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

[Vincent Donnefort]

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.

Thanks for your prompt reply to this patch!

-- 
Vincent

> 
> Rob
> 
> P.S. Nope, O_DIRECTORY isn't the right granularity either. You'd think "can do
> metadata but not data" would be an obvious level since that's what half the
> existing syscalls that then got an openat() version already do, but so far...
> _______________________________________________
> Toybox mailing list
> Toybox at lists.landley.net
> http://lists.landley.net/listinfo.cgi/toybox-landley.net