Re: [Qemu-devel] [for-2.10 PATCH] 9pfs: local: fix fchmodat_nofollow() limitations

[Greg Kurz]

On Tue, 8 Aug 2017 14:14:18 -0500
Eric Blake <address@hidden> wrote:

> On 08/08/2017 12:28 PM, Greg Kurz wrote:
> > This function has to ensure it doesn't follow a symlink that could be used
> > to escape the virtfs directory. This could be easily achieved if fchmodat()
> > on linux honored the AT_SYMLINK_NOFOLLOW flag as described in POSIX, but
> > it doesn't.
> > 
> > The current implementation covers most use-cases, but it notably fails if:
> > - the target path has access rights equal to 0000 (openat() returns EPERM), 
> >  
> >   => once you've done chmod(0000) on a file, you can never chmod() again  
> > - the target path is UNIX domain socket (openat() returns ENXIO)  
> >   => bind() of UNIX domain sockets fails if the file is on 9pfs  
> 
> Did your attempt at a kernel patch for AT_SYMLINK_NOFOLLOW ever get
> anywhere?
> 

No.

> > 
> > The solution is to use O_PATH: openat() now succeeds in both cases, and we
> > can ensure the path isn't a symlink with fstat(). The associated entry in
> > "/proc/self/fd" can hence be safely passed to the regular chmod() syscall.
> > 
> > Signed-off-by: Greg Kurz <address@hidden>
> > ---
> >  hw/9pfs/9p-local.c |   44 ++++++++++++++++++++++++++++----------------
> >  hw/9pfs/9p-util.h  |   10 +++++++---
> >  2 files changed, 35 insertions(+), 19 deletions(-)
> > 
> > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > index 6e478f4765ef..b178d627c764 100644
> > --- a/hw/9pfs/9p-local.c
> > +++ b/hw/9pfs/9p-local.c
> > @@ -333,30 +333,42 @@ update_map_file:
> >  
> >  static int fchmodat_nofollow(int dirfd, const char *name, mode_t mode)
> >  {
> > +    struct stat stbuf;
> >      int fd, ret;
> > +    char *proc_path;
> >  
> >      /* FIXME: this should be handled with fchmodat(AT_SYMLINK_NOFOLLOW).
> > -     * Unfortunately, the linux kernel doesn't implement it yet. As an
> > -     * alternative, let's open the file and use fchmod() instead. This
> > -     * may fail depending on the permissions of the file, but it is the
> > -     * best we can do to avoid TOCTTOU. We first try to open read-only
> > -     * in case name points to a directory. If that fails, we try write-only
> > -     * in case name doesn't point to a directory.
> > +     * Unfortunately, the linux kernel doesn't implement it yet.
> >       */
> > -    fd = openat_file(dirfd, name, O_RDONLY, 0);
> > -    if (fd == -1) {
> > -        /* In case the file is writable-only and isn't a directory. */
> > -        if (errno == EACCES) {
> > -            fd = openat_file(dirfd, name, O_WRONLY, 0);
> > -        }
> > -        if (fd == -1 && errno == EISDIR) {
> > -            errno = EACCES;
> > -        }
> > +    if (fstatat(dirfd, name, &stbuf, AT_SYMLINK_NOFOLLOW)) {
> > +        return -1;
> >      }  
> 
> Checking the file...
> 
> > +
> > +    if (S_ISLNK(stbuf.st_mode)) {
> > +        errno = ELOOP;
> > +        return -1;
> > +    }
> > +
> > +    fd = openat_file(dirfd, name, O_RDONLY | O_PATH, 0);  
> 
> ...and then opening the file is a TOCTTOU race (although it works most
> of the time and avoids the open where it is easy)...
> 

Exactly. It is globally assumed in the 9p code that symbolic links are
resolved by the client. The earlier we detect the file is a symbolic
link, the earlier we can error out. The fstat()+S_ISLNK() is a deliberate
fast error path actually :)

> >      if (fd == -1) {
> >          return -1;
> >      }
> > -    ret = fchmod(fd, mode);
> > +
> > +    ret = fstat(fd, &stbuf);
> > +    if (ret) {
> > +        goto out;
> > +    }  
> 
> ...so you are double-checking that you got a non-symlink after all (your
> insurance against the race having done the wrong thing [but see below])...
> 

Yes, this is the *real* check.

> > +
> > +    if (S_ISLNK(stbuf.st_mode)) {
> > +        errno = ELOOP;
> > +        ret = -1;
> > +        goto out;
> > +    }
> > +
> > +    proc_path = g_strdup_printf("/proc/self/fd/%d", fd);
> > +    ret = chmod(proc_path, mode);  
> 
> ...at which point you now have a valid file name that represents the
> file you wanted to chmod() in the first place (even if another rename
> occurs in the meantime, you are changing the mode tied to the
> non-symlink fd that you double-checked, which ends up behaving as if you
> had won the race and made the chmod() call before the rename).
> 

Yes.

> > +    g_free(proc_path);
> > +out:
> >      close_preserve_errno(fd);
> >      return ret;  
> 
> Might be worth littering some comments in the code explaining why you
> have to call both fstatat() and stat(), or perhaps you could drop the
> first fstatat() and just always do the open().
> 

I like the idea of erroring out right away when a symbolic link is
detected. I'll add comments.

> Reading 'man open', it looks like O_PATH will chase symlinks UNLESS you
> also use O_NOFOLLOW.  So I had to code up a simple test program to
> verify if things work...
> 
> =============
> #define _GNU_SOURCE 1
> #include <sys/stat.h>
> #include <stdio.h>
> #include <errno.h>
> #include <unistd.h>
> #include <fcntl.h>
> 
> int main(void) {
>     struct stat st;
>     int fd;
> 
>     remove("f");
>     remove("l");
>     if (creat("f", 0) < 0)
>         return 1;
>     if (symlink("f", "l") < 0)
>         return 2;
> 
>     fd = open("l", O_RDONLY | O_PATH);
>     if (fd < 0)
>         return 3;
>     if (fstat(fd, &st) < 0)
>         return 4;
>     if (S_ISLNK(st.st_mode)) {
>         printf("got a link\n");
>     } else {
>         printf("dereferenced\n");
>     }
>     close(fd);
> 
>     fd = open("l", O_RDONLY | O_PATH | O_NOFOLLOW);
>     if (fd < 0)
>         return 5;
>     if (fstat(fd, &st) < 0)
>         return 6;
>     if (S_ISLNK(st.st_mode)) {
>         printf("got a link\n");
>     } else {
>         printf("dereferenced\n");
>     }
>     close(fd);
> 
>     remove("f");
>     remove("l");
>     return 0;
> }
> ============
> $ ./foo
> dereferenced
> got a link
> 
> Ouch - you need to fix your patch to use open(O_NOFOLLOW | O_PATH).
> 

openat_file() adds O_NOFOLLOW... I'll rename it to openat_file_nofollow()
in a separate patch for clarity.

> Furthermore, I'm worried that your patch is regressing commit 918112c0,
> when compiling for older platforms where either O_PATH does not exist,
> or where libc exposes the constant but the kernel is too old to
> understand it.  (I guess the latter problem is already existing in our
> code base, so we really only need to worry about the former of compiling
> when the constant does not exist).  So if supporting old platforms is
> desired, you MUST keep BOTH approaches intact, gated by whether O_PATH
> is defined to a non-zero value, rather than just assuming O_PATH works.
> 

I don't necessarily want this to work for old platforms, but I'll fix the
potential build break like in commit 918112c0.

pgptGulNffZUd.pgp
Description: OpenPGP digital signature