On Thu, 23 Feb 2017 11:16:44 +0000 Stefan Hajnoczi <stefa...@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 <ja...@google.com> > > Signed-off-by: Greg Kurz <gr...@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 <gr...@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 <gr...@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 > > > >
pgpK3UjuBIVD9.pgp
Description: OpenPGP digital signature