On 3/10/23 14:59, Rob Landley wrote: >>> That said there's more options: the reason eject.c has open(O_NONBLOCK) is >>> so it >>> doesn't try to spin up the CD in the drive before the open returns, and in >>> theory I could do similar kinds of tricks here... but not without a test >>> case to >>> see what specifically I'm trying to fix. >> >> You should be able to reproduce the issue quite easily with `mknod c 1 3` in >> a >> cpio archive and to extract it on a mount point with nodev. > > but rereading this you meant "with nodev" is your > reproduction sequence, got it. (The fact you can mknod on a nodev mount seems > kind of strange, but...)
Didn't send the email ruminating about the design issues I worked through for commit 4e68a9268c3b, so for posterity: On the one hand, being unable to chown a node on a nodev mount seems... acceptable? I honestly did not expect the mknod to work there at all, and followup actions failing (with warning not error) because of it isn't exactly unexpected. That said, somebody could have a really weird use case of creating a new root filesystem image (container setup?) with root access to do a nodev mount but caring about security in ways that I can't really mentally mentally model... (What would they be trying to accomplish and/or defending against? I suppose "clueless management imposed an insane selinux regime on it they're awkwardly dancing around" is most likely. Sigh. I hate trying to program for a use case I can't mentally model.) Anyway, there IS another chown codepath in there that isn't entirely secure, and that one uses lchown(). So it protects against symlink attacks but not hardlinks, but if you can hardlink to something important you've already got some large cracks in your security model. (Although dnotify() -> replace expected name with an executable that's about to be chown(0) and suid is still concerning, and lchown() does not protect against that, but why would you set the suid bit on a device node? Also, the symlink path doesn't do a chmod() because a symlink's permissions are hardwired. On MacOS, they're hardwired wrong. :) Anyway, making the other path use lchown() isn't much MORE insecure? The permission bits are an argument to mknod() so that doesn't have to be done later (modulo restoring the suid bit after chown which we don't support for device nodes). Presumably I can just merge the mknod path with the symlink path and then have the other path JUST be S_ISDIR() which we should always be able to open... Well, not if the old one was chmod 000, which it should never be since we just created it except... should cpio or tar have TOYFLAG_UMASK? They're both working ok so far and "umask 777" breaking stuff is kinda to be expected... Ahem. Tangent! Trying to fix up the test suite so TEST_HOST doesn't spew record counts all over the place and... one of the existing tests doesn't work on my devuan. Commit 95a15d238120 added "skip runs of NUL bytes" because the kernel's extractor does and somebody needed that here (works with collated gzip instances) and... the one in debian is failing that test. It warns that it skipped 512 bytes, and then doesn't output the contents of the second concatenated instance of the archive. Mine does, debian's doesn't, does that make mine's right? Eh, check it in anyway. No worse than what's there... Rob _______________________________________________ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
