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

[Rob Landley]

On 3/9/23 09:21, Vincent Donnefort wrote:
> On Wed, Mar 08, 2023 at 07:01:53PM -0600, Rob Landley wrote:
>> On 3/8/23 11:05, Vincent Donnefort via Toybox wrote:
>> > CPIOs archives might contain dev nodes which could also point to
>> > unavailable drivers. In that case, fopen would simply fail, making it
>> > impossible for cpio to change the ownership.
>> 
>> Did you have this issue, or are you fixing a theoretical?
> 
> I do have this issue. Although after further investigation, it is due to the
> "nodev" attribute of the mount point. The scenario described in the commit
> description is nonetheless still a problem.
> 
>> 
>> > Switch to the pathname version for chown and stat to circumvent this
>> > limitation.
>> > 
>> > Change-Id: I5f7202bf19f4c3d4f750cdd72ed0c467f9166da6
>> > 
>> > diff --git a/toys/posix/cpio.c b/toys/posix/cpio.c
>> > index 282a39b1..b1a20203 100644
>> > --- a/toys/posix/cpio.c
>> > +++ b/toys/posix/cpio.c
>> > @@ -226,14 +226,11 @@ void cpio_main(void)
>> >        if (!S_ISREG(mode) && !S_ISLNK(mode) && !geteuid()
>> >            && !FLAG(no_preserve_owner))
>> >        {
>> > -        int fd = open(name, O_RDONLY|O_NOFOLLOW);
>> >          struct stat st;
>> >  
>> > -        if (fd != -1 && !fstat(fd, &st) && (st.st_mode&S_IFMT) == (mode&S_IFMT))
>> > -          err = fchown(fd, uid, gid);
>> > +        if (!stat(name, &st) && (st.st_mode&S_IFMT) == (mode&S_IFMT))
>> > +          err = chown(name, uid, gid);
>> 
>> The race that stat() tries to defend against is making that the object we're
>> doing the fchown() on is at least the same _kind_ of object we placed there, so
>> it's not vulnerable to inotify/symlink attacks where you stick your own file
>> there and suddenly it belongs to somebody else. (I'd also like to compare the
>> timestamps but that's unfortunately fraught...) Your change has a race window
>> between the stat() and the chown() where there's no guarantee they're operating
>> on the same filesystem object.
> 
> I see. But I don't think, we can really open a dev node without expecting
> anything on the driver side. e.g. a watchdog node, might point to a driver with
> NO_WAY_OUT (and then reset the system), which sounds more risky than what the
> open is protecting against. Maybe instead I could relax that test only for the
> device node case?
> 
>> 
>> In theory the open should use O_PATH, and I think I tried that at one point, but
>> in practice the granularity is wrong: neither "can operate on the file
>> descriptor" and "can operate on the contents" are "only operate on the
>> metadata". (And then there's the fun of "does MacOS or FreeBSD even have that
>> flag", but I've handwaved past that before, it's what portability.h is for...)
> 
> Sadly O_PATH doesn't work for fchown. (returns -EBADFD)
> 
>> 
>> 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.

I didn't get to this yesterday because when I saw it on my phone I looked up c 1
3 on lanana and it's /dev/null which you can open for reading and writing just
fine with no side effects... 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...)

Rob

P.S. Ok, technically I didn't look it up on lanana because lanana.org no longer
has the actual device list (it not just went the way of the Linux Standard Base
and stopped being worked on when LF took it over and drove all hobbyists out of
linux development, but the link itself is 404 on the site. That's very linux
Foundation: the website it still up but the content it existed to host is not.)

Luckily it was mirrored on kernel.org:

https://kernel.org/pub/linux/docs/lanana/device-list/devices-2.6.txt