Re: [Qemu-devel] [PATCH 04/29] 9pfs: introduce openat_nofollow() helper

[Greg Kurz]

On Thu, 23 Feb 2017 11:16:44 +0000
Stefan Hajnoczi <address@hidden> wrote:

> On Mon, Feb 20, 2017 at 03:39:51PM +0100, Greg Kurz wrote:
> > When using the passthrough security mode, symbolic links created by the
> > guest are actual symbolic links on the host file system.
> > 
> > Since the resolution of symbolic links during path walk is supposed to
> > occur on the client side. The server should hence never receive any path
> > pointing to an actual symbolic link. This isn't guaranteed by the protocol
> > though, and malicious code in the guest can trick the server to issue
> > various syscalls on paths whose one or more elements are symbolic links.
> > In the case of the "local" backend using the "passthrough" or "none"
> > security modes, the guest can directly create symbolic links to arbitrary
> > locations on the host (as per spec). The "mapped-xattr" and "mapped-file"
> > security modes are also affected to a lesser extent as they require some
> > help from an external entity to create actual symbolic links on the host,
> > i.e. anoter guest using "passthrough" mode for example.  
> 
> s/anoter/another/
> 
> > 
> > The current code hence relies on O_NOFOLLOW and "l*()" variants of system
> > calls. Unfortunately, this only applies to the rightmost path component.
> > A guest could maliciously replace any component in a trusted path with a
> > symbolic link. This could allow any guest to escape a virtfs shared folder.
> > 
> > This patch introduces a variant of the openat() syscall that successively
> > opens each path element with O_NOFOLLOW. When passing a file descriptor
> > pointing to a trusted directory, one is guaranteed to be returned a
> > file descriptor pointing to a path which is beneath the trusted directory.
> > This will be used by subsequent patches to implement symlink-safe path walk
> > for any access to the backend.
> > 
> > Suggested-by: Jann Horn <address@hidden>
> > Signed-off-by: Greg Kurz <address@hidden>
> > ---
> >  hw/9pfs/9p-util.c     |   69 
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> >  hw/9pfs/9p-util.h     |   25 ++++++++++++++++++
> >  hw/9pfs/Makefile.objs |    2 +
> >  3 files changed, 95 insertions(+), 1 deletion(-)
> >  create mode 100644 hw/9pfs/9p-util.c
> >  create mode 100644 hw/9pfs/9p-util.h
> > 
> > diff --git a/hw/9pfs/9p-util.c b/hw/9pfs/9p-util.c
> > new file mode 100644
> > index 000000000000..48292d948401
> > --- /dev/null
> > +++ b/hw/9pfs/9p-util.c
> > @@ -0,0 +1,69 @@
> > +/*
> > + * 9p utilities
> > + *
> > + * Copyright IBM, Corp. 2017
> > + *
> > + * Authors:
> > + *  Greg Kurz <address@hidden>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or 
> > later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "9p-util.h"
> > +
> > +int openat_nofollow(int dirfd, const char *path, int flags, mode_t mode)  
> 
> This function doesn't handle absolute paths?  It ignores leading '/' and
> therefore treats all paths as relative paths.
> 

Yes because any path coming from the client is supposed (*) to be relative to 
the
shared directory and openat(2) says:

If pathname is absolute, then dirfd is ignored.

(*) we make sure in the frontend that any path sent by the client doesn't
    contain '/'

> > +{
> > +    const char *tail = path;
> > +    const char *c;
> > +    int fd;
> > +
> > +    fd = dup(dirfd);
> > +    if (fd == -1) {
> > +        return -1;
> > +    }
> > +
> > +    while (*tail) {
> > +        int next_fd;
> > +        char *head;
> > +
> > +        while (*tail == '/') {
> > +            tail++;
> > +        }
> > +
> > +        if (!*tail) {
> > +            break;
> > +        }
> > +
> > +        head = g_strdup(tail);
> > +        c = strchr(tail, '/');
> > +        if (c) {
> > +            head[c - tail] = 0;
> > +            next_fd = openat(fd, head, O_DIRECTORY | O_RDONLY | 
> > O_NOFOLLOW);
> > +        } else {
> > +            /* We don't want bad things to happen like opening a file that
> > +             * sits outside the virtfs export, or hanging on a named pipe,
> > +             * or changing the controlling process of a terminal.
> > +             */
> > +            flags |= O_NOFOLLOW | O_NONBLOCK | O_NOCTTY;
> > +            next_fd = openat(fd, head, flags, mode);
> > +        }
> > +        g_free(head);
> > +        if (next_fd == -1) {
> > +            close_preserve_errno(fd);
> > +            return -1;
> > +        }
> > +        close(fd);
> > +        fd = next_fd;
> > +
> > +        if (!c) {
> > +            break;
> > +        }
> > +        tail = c + 1;
> > +    }
> > +    /* O_NONBLOCK was only needed to open the file. Let's drop it. */
> > +    assert(!fcntl(fd, F_SETFL, flags));  
> 
> If path="/" then we'll set flags on dirfd.  These flags are shared with
> other fds that have been duped.  Is this really what you want?
> 

You're right. This only makes sense if fd comes from openat() above...

Thanks for the catch! :)

> > +
> > +    return fd;
> > +}
> > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
> > new file mode 100644
> > index 000000000000..e19673d85222
> > --- /dev/null
> > +++ b/hw/9pfs/9p-util.h
> > @@ -0,0 +1,25 @@
> > +/*
> > + * 9p utilities
> > + *
> > + * Copyright IBM, Corp. 2017
> > + *
> > + * Authors:
> > + *  Greg Kurz <address@hidden>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or 
> > later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#ifndef QEMU_9P_UTIL_H
> > +#define QEMU_9P_UTIL_H
> > +
> > +static inline void close_preserve_errno(int fd)
> > +{
> > +    int serrno = errno;
> > +    close(fd);
> > +    errno = serrno;
> > +}
> > +
> > +int openat_nofollow(int dirfd, const char *path, int flags, mode_t mode);
> > +
> > +#endif
> > diff --git a/hw/9pfs/Makefile.objs b/hw/9pfs/Makefile.objs
> > index da0ae0cfdbae..32197e6671dd 100644
> > --- a/hw/9pfs/Makefile.objs
> > +++ b/hw/9pfs/Makefile.objs
> > @@ -1,4 +1,4 @@
> > -common-obj-y  = 9p.o
> > +common-obj-y  = 9p.o 9p-util.o
> >  common-obj-y += 9p-local.o 9p-xattr.o
> >  common-obj-y += 9p-xattr-user.o 9p-posix-acl.o
> >  common-obj-y += coth.o cofs.o codir.o cofile.o
> > 
> >

pgpf0mz9a1gCa.pgp
Description: OpenPGP digital signature