[Toybox] [PATCH] cpio: Use chown pathname versiong

[Rob Landley]

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