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

[Rob Landley]

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