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

[Stefan Hajnoczi]

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.

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

signature.asc
Description: PGP signature