[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 04/29] 9pfs: introduce openat_nofollow() helper
From: |
Greg Kurz |
Subject: |
Re: [Qemu-devel] [PATCH 04/29] 9pfs: introduce openat_nofollow() helper |
Date: |
Thu, 23 Feb 2017 12:56:21 +0100 |
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
[Qemu-devel] [PATCH 05/29] 9pfs: local: keep a file descriptor on the shared folder, Greg Kurz, 2017/02/20
[Qemu-devel] [PATCH 06/29] 9pfs: local: open/opendir: don't follow symlinks, Greg Kurz, 2017/02/20
[Qemu-devel] [PATCH 07/29] 9pfs: local: introduce symlink-attack safe xattr helpers, Greg Kurz, 2017/02/20