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

[Greg Kurz]

On Wed, 9 Aug 2017 09:55:32 -0500
Eric Blake <address@hidden> wrote:

> On 08/09/2017 09:23 AM, 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.  
> 
> Might be worth including a URL of the LKML discussion on the last
> version of that patch attempt.
> 

Ok.

> > 
> > 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  
> > 
> > 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.  
> 
> Hey - should we point this out as a viable solution to the glibc folks,
> since their current user-space emulation of AT_SYMLINK_NOFOLLOW is broken?
> 

Probably. What's the best way to do that ?

> > 
> > The previous behavior is kept for older systems that don't have O_PATH.
> > 
> > Signed-off-by: Greg Kurz <address@hidden>
> > ---
> > v2: - renamed OPENAT_DIR_O_PATH to O_PATH_9P_UTIL and use it as a 
> > replacement
> >       for O_PATH to avoid build breaks on O_PATH-less systems
> >     - keep current behavior for O_PATH-less systems
> >     - added comments
> >     - TODO in 2.11: add _nofollow suffix to openat_dir() and openat_file()
> > ---
> >  hw/9pfs/9p-local.c |   41 +++++++++++++++++++++++++++++++++--------
> >  hw/9pfs/9p-util.h  |   24 +++++++++++++++---------
> >  2 files changed, 48 insertions(+), 17 deletions(-)
> > 
> > +     /* First, we clear non-racing symlinks out of the way. */
> > +    if (fstatat(dirfd, name, &stbuf, AT_SYMLINK_NOFOLLOW)) {
> > +        return -1;
> > +    }
> > +    if (S_ISLNK(stbuf.st_mode)) {
> > +        errno = ELOOP;  
> 
> I don't know if ELOOP is the best errno value to use here, but I don't
> have any better suggestions so I'm okay with it.
> 
> > +        return -1;
> > +    }
> > +
> > +    /* Access modes are ignored when O_PATH is supported. We try O_RDONLY 
> > and
> > +     * O_WRONLY for old-systems that don't support O_PATH.
> >       */
> > -    fd = openat_file(dirfd, name, O_RDONLY, 0);
> > +    fd = openat_file(dirfd, name, O_RDONLY | O_PATH_9P_UTIL, 0);
> >      if (fd == -1) {
> >          /* In case the file is writable-only and isn't a directory. */
> >          if (errno == EACCES) {
> > @@ -356,7 +366,22 @@ static int fchmodat_nofollow(int dirfd, const char 
> > *name, mode_t mode)
> >      if (fd == -1) {
> >          return -1;
> >      }
> > -    ret = fchmod(fd, mode);
> > +
> > +    /* Now we handle racing symlinks. */
> > +    ret = fstat(fd, &stbuf);
> > +    if (ret) {
> > +        goto out;
> > +    }
> > +    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);  
> 
> Nope, unsafe when O_PATH_9P_UTIL is 0.  This needs to be more like:
> 
> /* Now we handle racing symlinks.  On kernels without O_PATH, we will
>  * fail on some corner cases, but that's better than dereferencing a
>  * symlink that was injected during the TOCTTOU between our initial
>  * fstatat() and openat_file().
>  */
> if (O_PATH_9P_UTIL) {
>     fstat, S_ISLINK, proc_path, chmod()
> } else {
>     fchmod()
> }
> 

Oops, you're right. I'll fix that.

pgpyabJZ0uXd6.pgp
Description: OpenPGP digital signature