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? > 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. 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...) 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. 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