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@lists.landley.net
> http://lists.landley.net/listinfo.cgi/toybox-landley.net
_______________________________________________
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net
