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. > +{ > + 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 <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 > >
signature.asc
Description: PGP signature