[04/29] 9pfs: introduce openat_nofollow() helper

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.

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 <jannh@google.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 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

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 <jannh@google.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  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 <groug@kaod.org>
> + *
> + * 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.

> +{
> +    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?

> +
> +    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 <groug@kaod.org>
> + *
> + * 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
> 
>

On Thu, 23 Feb 2017 11:16:44 +0000
Stefan Hajnoczi <stefanha@gmail.com> 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 <jannh@google.com>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  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 <groug@kaod.org>
> > + *
> > + * 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 <groug@kaod.org>
> > + *
> > + * 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
> > 
> >

On Thu, Feb 23, 2017 at 12:56:21PM +0100, Greg Kurz wrote:
> On Thu, 23 Feb 2017 11:16:44 +0000
> Stefan Hajnoczi <stefanha@gmail.com> 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 <jannh@google.com>
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > >  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 <groug@kaod.org>
> > > + *
> > > + * 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:

Please change the function name since this isn't openat with nofollow
behavior, it's a subset of openat that only takes relative paths with
nofollow behavior.

On Fri, 24 Feb 2017 17:17:30 +0000
Stefan Hajnoczi <stefanha@redhat.com> wrote:
[...]
> > > 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:  
> 
> Please change the function name since this isn't openat with nofollow
> behavior, it's a subset of openat that only takes relative paths with
> nofollow behavior.

In the v2, this function is only called by local_open_nofollow() actually.
Maybe I should move the stripping of leading '/' characters there ?

On Fri, Feb 24, 2017 at 11:17:44PM +0100, Greg Kurz wrote:
> On Fri, 24 Feb 2017 17:17:30 +0000
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> [...]
> > > > 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:  
> > 
> > Please change the function name since this isn't openat with nofollow
> > behavior, it's a subset of openat that only takes relative paths with
> > nofollow behavior.
> 
> In the v2, this function is only called by local_open_nofollow() actually.
> Maybe I should move the stripping of leading '/' characters there ?

As long as the function name is clear then I'm happy.  If it has
different semantics from openat() then it should have a different name
(e.g. relative_openat_nofollow()).

Stefan

On Mon, 27 Feb 2017 10:20:58 +0000
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Fri, Feb 24, 2017 at 11:17:44PM +0100, Greg Kurz wrote:
> > On Fri, 24 Feb 2017 17:17:30 +0000
> > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > [...]  
> > > > > 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:    
> > > 
> > > Please change the function name since this isn't openat with nofollow
> > > behavior, it's a subset of openat that only takes relative paths with
> > > nofollow behavior.  
> > 
> > In the v2, this function is only called by local_open_nofollow() actually.
> > Maybe I should move the stripping of leading '/' characters there ?  
> 
> As long as the function name is clear then I'm happy.  If it has
> different semantics from openat() then it should have a different name
> (e.g. relative_openat_nofollow()).
> 

I've moved the stripping to the caller. This makes the code simpler.

> Stefan