[Qemu-devel] [PATCH 00/29] 9pfs: local: fix vulnerability to symlink attacks

[Qemu-devel] [PATCH 00/29] 9pfs: local: fix vulnerability to symlink attacks

[Greg Kurz]

This series tries to fix CVE-2016-9602 reported by Jann Horn of Google
Project Zero:

https://bugzilla.redhat.com/show_bug.cgi?id=1413929

This vulnerability affects all accesses to the underlying filesystem in
the "local" backend code.

If QEMU is started with:

-fsdev local,security_model=<passthrough|none>,path=/foo/bar

then the guest can cause QEMU to create symlinks in /foo/bar.

This causes accesses to any path /foo/bar/some/path to be unsafe, since
untrusted code within the guest (or in another guest sharing the same
virtfs folder) could change some/path to point to a random path of the
host filesystem.

The core problem is that the "local" backend relies on path-based syscalls
to access the underlying filesystem. All path-based syscalls are vulnerable
to this issue, even open(O_NOFOLLOW) or syscalls that explicitly don't
dereference symlinks, since the kernel only checks the rightmost element of
the path. Depending on the privilege level of the QEMU process, a guest can
end up opening, renaming, changing ACLs, unlinking... files on the host
filesystem.

The right way to address this is to use "at" variants of all syscalls in
the "local" backend code. This requires to open directories without
traversing any symlink in the intermediate path elements. There was a
tentative to introduce an O_BENEATH flag for openat() that would address
this:

https://patchwork.kernel.org/patch/7007181/

Unfortunately this never got merged. I shall contact the author and try
to revive this kernel patchset.

In the meantime, an alternative is to walk through all path elements
manually with openat(O_NOFOLLOW). This is likely to degrade performances,
but I don't see any better way to get the vulnerability fixed in 2.9.
I'll try to come up with some numbers later.

Stefan and Daniel, I've Cc'ed you because we talked about the issue
on irc already. Feel free to comment/review if you have some spare
cycles, it will be appreciated (but of course, I'll understand if
you don't :)

---

Greg Kurz (29):
      9pfs: local: move xattr security ops to 9p-xattr.c
      9pfs: remove side-effects in local_init()
      9pfs: remove side-effects in local_open() and local_opendir()
      9pfs: introduce openat_nofollow() helper
      9pfs: local: keep a file descriptor on the shared folder
      9pfs: local: open/opendir: don't follow symlinks
      9pfs: local: introduce symlink-attack safe xattr helpers
      9pfs: local: lgetxattr: don't follow symlinks
      9pfs: local: llistxattr: don't follow symlinks
      9pfs: local: lsetxattr: don't follow symlinks
      9pfs: local: lremovexattr: don't follow symlinks
      9pfs: local: unlinkat: don't follow symlinks
      9pfs: local: remove: don't follow symlinks
      9pfs: local: utimensat: don't follow symlinks
      9pfs: local: statfs: don't follow symlinks
      9pfs: local: truncate: don't follow symlinks
      9pfs: local: readlink: don't follow symlinks
      9pfs: local: lstat: don't follow symlinks
      9pfs: local: renameat: don't follow symlinks
      9pfs: local: rename: use renameat
      9pfs: local: improve error handling in link op
      9pfs: local: link: don't follow symlinks
      9pfs: local: chmod: don't follow symlinks
      9pfs: local: chown: don't follow symlinks
      9pfs: local: symlink: don't follow symlinks
      9pfs: local: mknod: don't follow symlinks
      9pfs: local: mkdir: don't follow symlinks
      9pfs: local: open2: don't follow symlinks
      9pfs: local: drop unused code


 hw/9pfs/9p-local.c      | 1020 ++++++++++++++++++++++++++---------------------
 hw/9pfs/9p-local.h      |   20 +
 hw/9pfs/9p-posix-acl.c  |   44 --
 hw/9pfs/9p-util.c       |   69 +++
 hw/9pfs/9p-util.h       |   25 +
 hw/9pfs/9p-xattr-user.c |   24 -
 hw/9pfs/9p-xattr.c      |  231 ++++++++++-
 hw/9pfs/9p-xattr.h      |   93 +---
 hw/9pfs/Makefile.objs   |    2 
 9 files changed, 934 insertions(+), 594 deletions(-)
 create mode 100644 hw/9pfs/9p-local.h
 create mode 100644 hw/9pfs/9p-util.c
 create mode 100644 hw/9pfs/9p-util.h

--
Greg

[Qemu-devel] [PATCH 01/29] 9pfs: local: move xattr security ops to 9p-xattr.c

[Greg Kurz]

These functions are always called indirectly. It really doesn't make sense
for them to sit in a header file.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-xattr.c |   61 ++++++++++++++++++++++++++++++++++++++++
 hw/9pfs/9p-xattr.h |   80 +++++++++-------------------------------------------
 2 files changed, 75 insertions(+), 66 deletions(-)

diff --git a/hw/9pfs/9p-xattr.c b/hw/9pfs/9p-xattr.c
index 5d8595ed932a..19a2daf02f5c 100644
--- a/hw/9pfs/9p-xattr.c
+++ b/hw/9pfs/9p-xattr.c
@@ -143,6 +143,67 @@ int v9fs_remove_xattr(FsContext *ctx,
 
 }
 
+ssize_t pt_getxattr(FsContext *ctx, const char *path, const char *name,
+                    void *value, size_t size)
+{
+    char *buffer;
+    ssize_t ret;
+
+    buffer = rpath(ctx, path);
+    ret = lgetxattr(buffer, name, value, size);
+    g_free(buffer);
+    return ret;
+}
+
+int pt_setxattr(FsContext *ctx, const char *path, const char *name, void *value,
+                size_t size, int flags)
+{
+    char *buffer;
+    int ret;
+
+    buffer = rpath(ctx, path);
+    ret = lsetxattr(buffer, name, value, size, flags);
+    g_free(buffer);
+    return ret;
+}
+
+int pt_removexattr(FsContext *ctx, const char *path, const char *name)
+{
+    char *buffer;
+    int ret;
+
+    buffer = rpath(ctx, path);
+    ret = lremovexattr(path, name);
+    g_free(buffer);
+    return ret;
+}
+
+ssize_t notsup_getxattr(FsContext *ctx, const char *path, const char *name,
+                        void *value, size_t size)
+{
+    errno = ENOTSUP;
+    return -1;
+}
+
+int notsup_setxattr(FsContext *ctx, const char *path, const char *name,
+                    void *value, size_t size, int flags)
+{
+    errno = ENOTSUP;
+    return -1;
+}
+
+ssize_t notsup_listxattr(FsContext *ctx, const char *path, char *name,
+                         void *value, size_t size)
+{
+    return 0;
+}
+
+int notsup_removexattr(FsContext *ctx, const char *path, const char *name)
+{
+    errno = ENOTSUP;
+    return -1;
+}
+
 XattrOperations *mapped_xattr_ops[] = {
     &mapped_user_xattr,
     &mapped_pacl_xattr,
diff --git a/hw/9pfs/9p-xattr.h b/hw/9pfs/9p-xattr.h
index a853ea641c0b..3f43f5153f3c 100644
--- a/hw/9pfs/9p-xattr.h
+++ b/hw/9pfs/9p-xattr.h
@@ -49,73 +49,21 @@ ssize_t v9fs_list_xattr(FsContext *ctx, const char *path, void *value,
 int v9fs_set_xattr(FsContext *ctx, const char *path, const char *name,
                           void *value, size_t size, int flags);
 int v9fs_remove_xattr(FsContext *ctx, const char *path, const char *name);
+
 ssize_t pt_listxattr(FsContext *ctx, const char *path, char *name, void *value,
                      size_t size);
-
-static inline ssize_t pt_getxattr(FsContext *ctx, const char *path,
-                                  const char *name, void *value, size_t size)
-{
-    char *buffer;
-    ssize_t ret;
-
-    buffer = rpath(ctx, path);
-    ret = lgetxattr(buffer, name, value, size);
-    g_free(buffer);
-    return ret;
-}
-
-static inline int pt_setxattr(FsContext *ctx, const char *path,
-                              const char *name, void *value,
-                              size_t size, int flags)
-{
-    char *buffer;
-    int ret;
-
-    buffer = rpath(ctx, path);
-    ret = lsetxattr(buffer, name, value, size, flags);
-    g_free(buffer);
-    return ret;
-}
-
-static inline int pt_removexattr(FsContext *ctx,
-                                 const char *path, const char *name)
-{
-    char *buffer;
-    int ret;
-
-    buffer = rpath(ctx, path);
-    ret = lremovexattr(path, name);
-    g_free(buffer);
-    return ret;
-}
-
-static inline ssize_t notsup_getxattr(FsContext *ctx, const char *path,
-                                      const char *name, void *value,
-                                      size_t size)
-{
-    errno = ENOTSUP;
-    return -1;
-}
-
-static inline int notsup_setxattr(FsContext *ctx, const char *path,
-                                  const char *name, void *value,
-                                  size_t size, int flags)
-{
-    errno = ENOTSUP;
-    return -1;
-}
-
-static inline ssize_t notsup_listxattr(FsContext *ctx, const char *path,
-                                       char *name, void *value, size_t size)
-{
-    return 0;
-}
-
-static inline int notsup_removexattr(FsContext *ctx,
-                                     const char *path, const char *name)
-{
-    errno = ENOTSUP;
-    return -1;
-}
+ssize_t pt_getxattr(FsContext *ctx, const char *path, const char *name,
+                    void *value, size_t size);
+int pt_setxattr(FsContext *ctx, const char *path, const char *name, void *value,
+                size_t size, int flags);
+int pt_removexattr(FsContext *ctx, const char *path, const char *name);
+
+ssize_t notsup_getxattr(FsContext *ctx, const char *path, const char *name,
+                        void *value, size_t size);
+int notsup_setxattr(FsContext *ctx, const char *path, const char *name,
+                    void *value, size_t size, int flags);
+ssize_t notsup_listxattr(FsContext *ctx, const char *path, char *name,
+                         void *value, size_t size);
+int notsup_removexattr(FsContext *ctx, const char *path, const char *name);
 
 #endif

[Qemu-devel] [PATCH 02/29] 9pfs: remove side-effects in local_init()

[Greg Kurz]

If this function fails, it should not modify *ctx.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 7de07e1ba67f..55903e5d7745 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1168,9 +1168,25 @@ static int local_ioc_getversion(FsContext *ctx, V9fsPath *path,
 
 static int local_init(FsContext *ctx)
 {
-    int err = 0;
     struct statfs stbuf;
 
+#ifdef FS_IOC_GETVERSION
+    /*
+     * use ioc_getversion only if the iocl is definied
+     */
+    if (statfs(ctx->fs_root, &stbuf) < 0) {
+        return -1;
+    }
+    switch (stbuf.f_type) {
+    case EXT2_SUPER_MAGIC:
+    case BTRFS_SUPER_MAGIC:
+    case REISERFS_SUPER_MAGIC:
+    case XFS_SUPER_MAGIC:
+        ctx->exops.get_st_gen = local_ioc_getversion;
+        break;
+    }
+#endif
+
     if (ctx->export_flags & V9FS_SM_PASSTHROUGH) {
         ctx->xops = passthrough_xattr_ops;
     } else if (ctx->export_flags & V9FS_SM_MAPPED) {
@@ -1185,23 +1201,8 @@ static int local_init(FsContext *ctx)
         ctx->xops = passthrough_xattr_ops;
     }
     ctx->export_flags |= V9FS_PATHNAME_FSCONTEXT;
-#ifdef FS_IOC_GETVERSION
-    /*
-     * use ioc_getversion only if the iocl is definied
-     */
-    err = statfs(ctx->fs_root, &stbuf);
-    if (!err) {
-        switch (stbuf.f_type) {
-        case EXT2_SUPER_MAGIC:
-        case BTRFS_SUPER_MAGIC:
-        case REISERFS_SUPER_MAGIC:
-        case XFS_SUPER_MAGIC:
-            ctx->exops.get_st_gen = local_ioc_getversion;
-            break;
-        }
-    }
-#endif
-    return err;
+
+    return 0;
 }
 
 static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse)

[Qemu-devel] [PATCH 03/29] 9pfs: remove side-effects in local_open() and local_opendir()

[Greg Kurz]

If these functions fail, they should not change *fs. Let's use local
variables to fix this.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 55903e5d7745..c2239bfafce4 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -356,10 +356,15 @@ static int local_open(FsContext *ctx, V9fsPath *fs_path,
 {
     char *buffer;
     char *path = fs_path->data;
+    int fd;
 
     buffer = rpath(ctx, path);
-    fs->fd = open(buffer, flags | O_NOFOLLOW);
+    fd = open(buffer, flags | O_NOFOLLOW);
     g_free(buffer);
+    if (fd == -1) {
+        return -1;
+    }
+    fs->fd = fd;
     return fs->fd;
 }
 
@@ -368,13 +373,15 @@ static int local_opendir(FsContext *ctx,
 {
     char *buffer;
     char *path = fs_path->data;
+    DIR *stream;
 
     buffer = rpath(ctx, path);
-    fs->dir.stream = opendir(buffer);
+    stream = opendir(buffer);
     g_free(buffer);
-    if (!fs->dir.stream) {
+    if (!stream) {
         return -1;
     }
+    fs->dir.stream = stream;
     return 0;
 }
 

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

[Greg Kurz]

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

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

[Qemu-devel] [PATCH 05/29] 9pfs: local: keep a file descriptor on the shared folder

[Greg Kurz]

This patch opens the shared folder and caches the file descriptor, so that
it can be used to do symlink-safe path walk.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index c2239bfafce4..8aee7f2516ed 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -14,6 +14,7 @@
 #include "qemu/osdep.h"
 #include "9p.h"
 #include "9p-xattr.h"
+#include "9p-util.h"
 #include "fsdev/qemu-fsdev.h"   /* local_ops */
 #include <arpa/inet.h>
 #include <pwd.h>
@@ -43,6 +44,10 @@
 #define BTRFS_SUPER_MAGIC 0x9123683E
 #endif
 
+struct local_data {
+    int mountfd;
+};
+
 #define VIRTFS_META_DIR ".virtfs_metadata"
 
 static char *local_mapped_attr_path(FsContext *ctx, const char *path)
@@ -1176,13 +1181,20 @@ static int local_ioc_getversion(FsContext *ctx, V9fsPath *path,
 static int local_init(FsContext *ctx)
 {
     struct statfs stbuf;
+    struct local_data *data = g_malloc(sizeof(*data));
+
+    data->mountfd = open(ctx->fs_root, O_DIRECTORY | O_RDONLY);
+    if (data->mountfd == -1) {
+        goto err;
+    }
 
 #ifdef FS_IOC_GETVERSION
     /*
      * use ioc_getversion only if the iocl is definied
      */
-    if (statfs(ctx->fs_root, &stbuf) < 0) {
-        return -1;
+    if (fstatfs(data->mountfd, &stbuf) < 0) {
+        close_preserve_errno(data->mountfd);
+        goto err;
     }
     switch (stbuf.f_type) {
     case EXT2_SUPER_MAGIC:
@@ -1209,7 +1221,20 @@ static int local_init(FsContext *ctx)
     }
     ctx->export_flags |= V9FS_PATHNAME_FSCONTEXT;
 
+    ctx->private = data;
     return 0;
+
+err:
+    g_free(data);
+    return -1;
+}
+
+static void local_cleanup(FsContext *ctx)
+{
+    struct local_data *data = ctx->private;
+
+    close(data->mountfd);
+    g_free(data);
 }
 
 static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse)
@@ -1252,6 +1277,7 @@ static int local_parse_opts(QemuOpts *opts, struct FsDriverEntry *fse)
 FileOperations local_ops = {
     .parse_opts = local_parse_opts,
     .init  = local_init,
+    .cleanup = local_cleanup,
     .lstat = local_lstat,
     .readlink = local_readlink,
     .close = local_close,

[Qemu-devel] [PATCH 06/29] 9pfs: local: open/opendir: don't follow symlinks

[Greg Kurz]

The local_open() and local_opendir() callbacks are vulnerable to symlink
attacks because they call:

(1) open(O_NOFOLLOW) which follows symbolic links in all path elements but
    the rightmost one
(2) opendir() which follows symbolic links in all path elements

This patch converts both callbacks to use new helpers based on
openat_nofollow() to only open files and directories if they are
below the virtfs shared folder

This partly fixes CVE-2016-9602.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   31 +++++++++++++++++++++----------
 hw/9pfs/9p-local.h |   20 ++++++++++++++++++++
 2 files changed, 41 insertions(+), 10 deletions(-)
 create mode 100644 hw/9pfs/9p-local.h

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 8aee7f2516ed..26ea72c66306 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -13,6 +13,7 @@
 
 #include "qemu/osdep.h"
 #include "9p.h"
+#include "9p-local.h"
 #include "9p-xattr.h"
 #include "9p-util.h"
 #include "fsdev/qemu-fsdev.h"   /* local_ops */
@@ -48,6 +49,18 @@ struct local_data {
     int mountfd;
 };
 
+int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags,
+                        mode_t mode)
+{
+    struct local_data *data = fs_ctx->private;
+    return openat_nofollow(data->mountfd, path, flags, mode);
+}
+
+int local_opendir_nofollow(FsContext *fs_ctx, const char *path)
+{
+    return local_open_nofollow(fs_ctx, path, O_DIRECTORY | O_RDONLY, 0);
+}
+
 #define VIRTFS_META_DIR ".virtfs_metadata"
 
 static char *local_mapped_attr_path(FsContext *ctx, const char *path)
@@ -359,13 +372,9 @@ static int local_closedir(FsContext *ctx, V9fsFidOpenState *fs)
 static int local_open(FsContext *ctx, V9fsPath *fs_path,
                       int flags, V9fsFidOpenState *fs)
 {
-    char *buffer;
-    char *path = fs_path->data;
     int fd;
 
-    buffer = rpath(ctx, path);
-    fd = open(buffer, flags | O_NOFOLLOW);
-    g_free(buffer);
+    fd = local_open_nofollow(ctx, fs_path->data, flags, 0);
     if (fd == -1) {
         return -1;
     }
@@ -376,13 +385,15 @@ static int local_open(FsContext *ctx, V9fsPath *fs_path,
 static int local_opendir(FsContext *ctx,
                          V9fsPath *fs_path, V9fsFidOpenState *fs)
 {
-    char *buffer;
-    char *path = fs_path->data;
+    int dirfd;
     DIR *stream;
 
-    buffer = rpath(ctx, path);
-    stream = opendir(buffer);
-    g_free(buffer);
+    dirfd = local_opendir_nofollow(ctx, fs_path->data);
+    if (dirfd == -1) {
+        return -1;
+    }
+
+    stream = fdopendir(dirfd);
     if (!stream) {
         return -1;
     }
diff --git a/hw/9pfs/9p-local.h b/hw/9pfs/9p-local.h
new file mode 100644
index 000000000000..32c72749d9df
--- /dev/null
+++ b/hw/9pfs/9p-local.h
@@ -0,0 +1,20 @@
+/*
+ * 9p local backend 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_LOCAL_H
+#define QEMU_9P_LOCAL_H
+
+int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags,
+                        mode_t mode);
+int local_opendir_nofollow(FsContext *fs_ctx, const char *path);
+
+#endif

[Qemu-devel] [PATCH 07/29] 9pfs: local: introduce symlink-attack safe xattr helpers

[Greg Kurz]

All operations dealing with extended attributes are vulnerable to symlink
attacks because they use path-based syscalls which can traverse symbolic
links while walking through the dirname part of the path.

The solution is to introduce helpers based on opendir_nofollow(). This
calls for "at" versions of the extended attribute syscalls, which don't
exist unfortunately. This patch implement them by simulating the "at"
behavior with fchdir(). Since the current working directory is process
wide, and we don't want to confuse another thread in QEMU, all the work
is done in a separate process.

The extended attributes code spreads over several files: all helpers
are hence declared with external linkage in 9p-xattr.h.

Note that the listxattr-based code is fully contained in 9p-xattr.c: the
flistxattrat_nofollow() helper is added in a subsequent patch.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-xattr.c |  158 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/9pfs/9p-xattr.h |   13 ++++
 2 files changed, 171 insertions(+)

diff --git a/hw/9pfs/9p-xattr.c b/hw/9pfs/9p-xattr.c
index 19a2daf02f5c..62993624ff64 100644
--- a/hw/9pfs/9p-xattr.c
+++ b/hw/9pfs/9p-xattr.c
@@ -15,7 +15,165 @@
 #include "9p.h"
 #include "fsdev/file-op-9p.h"
 #include "9p-xattr.h"
+#include "9p-util.h"
 
+enum {
+    XATTRAT_OP_GET = 0,
+    XATTRAT_OP_LIST,
+    XATTRAT_OP_SET,
+    XATTRAT_OP_REMOVE
+};
+
+struct xattrat_data {
+    ssize_t ret;
+    int serrno;
+    char value[0];
+};
+
+static void munmap_preserver_errno(void *addr, size_t length)
+{
+    int serrno = errno;
+    munmap(addr, length);
+    errno = serrno;
+}
+
+static ssize_t do_xattrat_op(int op_type, int dirfd, const char *path,
+                             const char *name, void *value, size_t size,
+                             int flags)
+{
+    struct xattrat_data *data;
+    pid_t pid;
+    ssize_t ret = -1;
+    int wstatus;
+
+    data = mmap(NULL, sizeof(*data) + size, PROT_READ | PROT_WRITE,
+                MAP_SHARED | MAP_ANONYMOUS, -1, 0);
+    if (data == MAP_FAILED) {
+        return -1;
+    }
+    data->ret = -1;
+
+    pid = fork();
+    if (pid < 0) {
+        goto err_out;
+    } else if (pid == 0) {
+        if (fchdir(dirfd) == 0) {
+            switch (op_type) {
+            case XATTRAT_OP_GET:
+                data->ret = lgetxattr(path, name, data->value, size);
+                break;
+            case XATTRAT_OP_LIST:
+                data->ret = llistxattr(path, data->value, size);
+                break;
+            case XATTRAT_OP_SET:
+                data->ret = lsetxattr(path, name, value, size, flags);
+                break;
+            case XATTRAT_OP_REMOVE:
+                data->ret = lremovexattr(path, name);
+                break;
+            default:
+                g_assert_not_reached();
+            }
+        }
+        data->serrno = errno;
+        _exit(0);
+    }
+    assert(waitpid(pid, &wstatus, 0) == pid && WIFEXITED(wstatus));
+
+    ret = data->ret;
+    if (ret < 0) {
+        errno = data->serrno;
+        goto err_out;
+    }
+    if (value) {
+        memcpy(value, data->value, data->ret);
+    }
+err_out:
+    munmap_preserver_errno(data, sizeof(*data) + size);
+    return ret;
+}
+
+ssize_t fgetxattrat_nofollow(int dirfd, const char *path, const char *name,
+                             void *value, size_t size)
+{
+    return do_xattrat_op(XATTRAT_OP_GET, dirfd, path, name, value, size, 0);
+}
+
+ssize_t local_getxattr_nofollow(FsContext *ctx, const char *path,
+                                const char *name, void *value, size_t size)
+{
+    char *dirpath = g_path_get_dirname(path);
+    char *filename = g_path_get_basename(path);
+    int dirfd;
+    ssize_t ret = -1;
+
+    dirfd = local_opendir_nofollow(ctx, dirpath);
+    if (dirfd == -1) {
+        goto out;
+    }
+
+    ret = fgetxattrat_nofollow(dirfd, filename, name, value, size);
+    close_preserve_errno(dirfd);
+out:
+    g_free(dirpath);
+    g_free(filename);
+    return ret;
+}
+
+int fsetxattrat_nofollow(int dirfd, const char *path, const char *name,
+                         void *value, size_t size, int flags)
+{
+    return do_xattrat_op(XATTRAT_OP_SET, dirfd, path, name, value, size, flags);
+}
+
+ssize_t local_setxattr_nofollow(FsContext *ctx, const char *path,
+                                const char *name, void *value, size_t size,
+                                int flags)
+{
+    char *dirpath = g_path_get_dirname(path);
+    char *filename = g_path_get_basename(path);
+    int dirfd;
+    ssize_t ret = -1;
+
+    dirfd = local_opendir_nofollow(ctx, dirpath);
+    if (dirfd == -1) {
+        goto out;
+    }
+
+    ret = fsetxattrat_nofollow(dirfd, filename, name, value, size, flags);
+    close_preserve_errno(dirfd);
+out:
+    g_free(dirpath);
+    g_free(filename);
+    return ret;
+}
+
+static ssize_t fremovexattrat_nofollow(int dirfd, const char *path,
+                                       const char *name)
+{
+    return do_xattrat_op(XATTRAT_OP_GET, dirfd, path, name, NULL, 0, 0);
+}
+
+ssize_t local_removexattr_nofollow(FsContext *ctx, const char *path,
+                                   const char *name)
+{
+    char *dirpath = g_path_get_dirname(path);
+    char *filename = g_path_get_basename(path);
+    int dirfd;
+    ssize_t ret = -1;
+
+    dirfd = local_opendir_nofollow(ctx, dirpath);
+    if (dirfd == -1) {
+        goto out;
+    }
+
+    ret = fremovexattrat_nofollow(dirfd, filename, name);
+    close_preserve_errno(dirfd);
+out:
+    g_free(dirpath);
+    g_free(filename);
+    return ret;
+}
 
 static XattrOperations *get_xattr_operations(XattrOperations **h,
                                              const char *name)
diff --git a/hw/9pfs/9p-xattr.h b/hw/9pfs/9p-xattr.h
index 3f43f5153f3c..986cb59b67f2 100644
--- a/hw/9pfs/9p-xattr.h
+++ b/hw/9pfs/9p-xattr.h
@@ -15,6 +15,7 @@
 #define QEMU_9P_XATTR_H
 
 #include "qemu/xattr.h"
+#include "9p-local.h"
 
 typedef struct xattr_operations
 {
@@ -29,6 +30,18 @@ typedef struct xattr_operations
                        const char *path, const char *name);
 } XattrOperations;
 
+ssize_t fgetxattrat_nofollow(int dirfd, const char *path, const char *name,
+                             void *value, size_t size);
+int fsetxattrat_nofollow(int dirfd, const char *path, const char *name,
+                         void *value, size_t size, int flags);
+
+ssize_t local_getxattr_nofollow(FsContext *ctx, const char *path,
+                                const char *name, void *value, size_t size);
+ssize_t local_setxattr_nofollow(FsContext *ctx, const char *path,
+                                const char *name, void *value, size_t size,
+                                int flags);
+ssize_t local_removexattr_nofollow(FsContext *ctx, const char *path,
+                                   const char *name);
 
 extern XattrOperations mapped_user_xattr;
 extern XattrOperations passthrough_user_xattr;

[Qemu-devel] [PATCH 08/29] 9pfs: local: lgetxattr: don't follow symlinks

[Greg Kurz]

The local_lgetxattr() callback is vulnerable to symlink attacks because
it calls lgetxattr() which follows symbolic links in all path elements but
the rightmost one.

This patch converts local_lgetxattr() to rely on opendir_nofollow() and
fgetxattrat_nofollow() instead.

This partly fixes CVE-2016-9602.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-posix-acl.c  |   16 ++--------------
 hw/9pfs/9p-xattr-user.c |    8 +-------
 hw/9pfs/9p-xattr.c      |    8 +-------
 3 files changed, 4 insertions(+), 28 deletions(-)

diff --git a/hw/9pfs/9p-posix-acl.c b/hw/9pfs/9p-posix-acl.c
index ec003181cd33..9435e27a368c 100644
--- a/hw/9pfs/9p-posix-acl.c
+++ b/hw/9pfs/9p-posix-acl.c
@@ -25,13 +25,7 @@
 static ssize_t mp_pacl_getxattr(FsContext *ctx, const char *path,
                                 const char *name, void *value, size_t size)
 {
-    char *buffer;
-    ssize_t ret;
-
-    buffer = rpath(ctx, path);
-    ret = lgetxattr(buffer, MAP_ACL_ACCESS, value, size);
-    g_free(buffer);
-    return ret;
+    return local_getxattr_nofollow(ctx, path, MAP_ACL_ACCESS, value, size);
 }
 
 static ssize_t mp_pacl_listxattr(FsContext *ctx, const char *path,
@@ -89,13 +83,7 @@ static int mp_pacl_removexattr(FsContext *ctx,
 static ssize_t mp_dacl_getxattr(FsContext *ctx, const char *path,
                                 const char *name, void *value, size_t size)
 {
-    char *buffer;
-    ssize_t ret;
-
-    buffer = rpath(ctx, path);
-    ret = lgetxattr(buffer, MAP_ACL_DEFAULT, value, size);
-    g_free(buffer);
-    return ret;
+    return local_getxattr_nofollow(ctx, path, MAP_ACL_DEFAULT, value, size);
 }
 
 static ssize_t mp_dacl_listxattr(FsContext *ctx, const char *path,
diff --git a/hw/9pfs/9p-xattr-user.c b/hw/9pfs/9p-xattr-user.c
index f87530c8b526..4071fbc4c086 100644
--- a/hw/9pfs/9p-xattr-user.c
+++ b/hw/9pfs/9p-xattr-user.c
@@ -20,9 +20,6 @@
 static ssize_t mp_user_getxattr(FsContext *ctx, const char *path,
                                 const char *name, void *value, size_t size)
 {
-    char *buffer;
-    ssize_t ret;
-
     if (strncmp(name, "user.virtfs.", 12) == 0) {
         /*
          * Don't allow fetch of user.virtfs namesapce
@@ -31,10 +28,7 @@ static ssize_t mp_user_getxattr(FsContext *ctx, const char *path,
         errno = ENOATTR;
         return -1;
     }
-    buffer = rpath(ctx, path);
-    ret = lgetxattr(buffer, name, value, size);
-    g_free(buffer);
-    return ret;
+    return local_getxattr_nofollow(ctx, path, name, value, size);
 }
 
 static ssize_t mp_user_listxattr(FsContext *ctx, const char *path,
diff --git a/hw/9pfs/9p-xattr.c b/hw/9pfs/9p-xattr.c
index 62993624ff64..4c3c0046bd47 100644
--- a/hw/9pfs/9p-xattr.c
+++ b/hw/9pfs/9p-xattr.c
@@ -304,13 +304,7 @@ int v9fs_remove_xattr(FsContext *ctx,
 ssize_t pt_getxattr(FsContext *ctx, const char *path, const char *name,
                     void *value, size_t size)
 {
-    char *buffer;
-    ssize_t ret;
-
-    buffer = rpath(ctx, path);
-    ret = lgetxattr(buffer, name, value, size);
-    g_free(buffer);
-    return ret;
+    return local_getxattr_nofollow(ctx, path, name, value, size);
 }
 
 int pt_setxattr(FsContext *ctx, const char *path, const char *name, void *value,

[Qemu-devel] [PATCH 09/29] 9pfs: local: llistxattr: don't follow symlinks

[Greg Kurz]

The local_llistxattr() callback is vulnerable to symlink attacks because
it calls llistxattr() which follows symbolic links in all path elements but
the rightmost one.

This patch converts local_llistxattr() to rely on opendir_nofollow() and
flistxattrat_nofollow() instead.

This partly fixes CVE-2016-9602.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-xattr.c |   30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/hw/9pfs/9p-xattr.c b/hw/9pfs/9p-xattr.c
index 4c3c0046bd47..803d4bbbc50b 100644
--- a/hw/9pfs/9p-xattr.c
+++ b/hw/9pfs/9p-xattr.c
@@ -216,6 +216,11 @@ ssize_t pt_listxattr(FsContext *ctx, const char *path,
     return name_size;
 }
 
+static ssize_t flistxattrat(int dirfd, const char *path, char *list,
+                            size_t size)
+{
+    return do_xattrat_op(XATTRAT_OP_LIST, dirfd, path, NULL, list, size, 0);
+}
 
 /*
  * Get the list and pass to each layer to find out whether
@@ -225,24 +230,37 @@ ssize_t v9fs_list_xattr(FsContext *ctx, const char *path,
                         void *value, size_t vsize)
 {
     ssize_t size = 0;
-    char *buffer;
     void *ovalue = value;
     XattrOperations *xops;
     char *orig_value, *orig_value_start;
     ssize_t xattr_len, parsed_len = 0, attr_len;
+    char *dirpath, *name;
+    int dirfd;
 
     /* Get the actual len */
-    buffer = rpath(ctx, path);
-    xattr_len = llistxattr(buffer, value, 0);
+    dirpath = g_path_get_dirname(path);
+    dirfd = local_opendir_nofollow(ctx, dirpath);
+    g_free(dirpath);
+    if (dirfd == -1) {
+        return -1;
+    }
+
+    name = g_path_get_basename(path);
+    xattr_len = flistxattrat(dirfd, name, value, 0);
     if (xattr_len <= 0) {
-        g_free(buffer);
+        g_free(name);
+        close_preserve_errno(dirfd);
         return xattr_len;
     }
 
     /* Now fetch the xattr and find the actual size */
     orig_value = g_malloc(xattr_len);
-    xattr_len = llistxattr(buffer, orig_value, xattr_len);
-    g_free(buffer);
+    xattr_len = flistxattrat(dirfd, name, orig_value, xattr_len);
+    g_free(name);
+    close_preserve_errno(dirfd);
+    if (xattr_len < 0) {
+        return -1;
+    }
 
     /* store the orig pointer */
     orig_value_start = orig_value;

[Qemu-devel] [PATCH 10/29] 9pfs: local: lsetxattr: don't follow symlinks

[Greg Kurz]

The local_lsetxattr() callback is vulnerable to symlink attacks because
it calls lsetxattr() which follows symbolic links in all path elements but
the rightmost one.

This patch converts local_lsetxattr() to rely on opendir_nofollow() and
fsetxattrat_nofollow() instead.

This partly fixes CVE-2016-9602.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-posix-acl.c  |   18 ++++--------------
 hw/9pfs/9p-xattr-user.c |    8 +-------
 hw/9pfs/9p-xattr.c      |    8 +-------
 3 files changed, 6 insertions(+), 28 deletions(-)

diff --git a/hw/9pfs/9p-posix-acl.c b/hw/9pfs/9p-posix-acl.c
index 9435e27a368c..0154e2a7605f 100644
--- a/hw/9pfs/9p-posix-acl.c
+++ b/hw/9pfs/9p-posix-acl.c
@@ -50,13 +50,8 @@ static ssize_t mp_pacl_listxattr(FsContext *ctx, const char *path,
 static int mp_pacl_setxattr(FsContext *ctx, const char *path, const char *name,
                             void *value, size_t size, int flags)
 {
-    char *buffer;
-    int ret;
-
-    buffer = rpath(ctx, path);
-    ret = lsetxattr(buffer, MAP_ACL_ACCESS, value, size, flags);
-    g_free(buffer);
-    return ret;
+    return local_setxattr_nofollow(ctx, path, MAP_ACL_ACCESS, value, size,
+                                   flags);
 }
 
 static int mp_pacl_removexattr(FsContext *ctx,
@@ -108,13 +103,8 @@ static ssize_t mp_dacl_listxattr(FsContext *ctx, const char *path,
 static int mp_dacl_setxattr(FsContext *ctx, const char *path, const char *name,
                             void *value, size_t size, int flags)
 {
-    char *buffer;
-    int ret;
-
-    buffer = rpath(ctx, path);
-    ret = lsetxattr(buffer, MAP_ACL_DEFAULT, value, size, flags);
-    g_free(buffer);
-    return ret;
+    return local_setxattr_nofollow(ctx, path, MAP_ACL_DEFAULT, value, size,
+                                   flags);
 }
 
 static int mp_dacl_removexattr(FsContext *ctx,
diff --git a/hw/9pfs/9p-xattr-user.c b/hw/9pfs/9p-xattr-user.c
index 4071fbc4c086..1840a5db66f3 100644
--- a/hw/9pfs/9p-xattr-user.c
+++ b/hw/9pfs/9p-xattr-user.c
@@ -67,9 +67,6 @@ static ssize_t mp_user_listxattr(FsContext *ctx, const char *path,
 static int mp_user_setxattr(FsContext *ctx, const char *path, const char *name,
                             void *value, size_t size, int flags)
 {
-    char *buffer;
-    int ret;
-
     if (strncmp(name, "user.virtfs.", 12) == 0) {
         /*
          * Don't allow fetch of user.virtfs namesapce
@@ -78,10 +75,7 @@ static int mp_user_setxattr(FsContext *ctx, const char *path, const char *name,
         errno = EACCES;
         return -1;
     }
-    buffer = rpath(ctx, path);
-    ret = lsetxattr(buffer, name, value, size, flags);
-    g_free(buffer);
-    return ret;
+    return local_setxattr_nofollow(ctx, path, name, value, size, flags);
 }
 
 static int mp_user_removexattr(FsContext *ctx,
diff --git a/hw/9pfs/9p-xattr.c b/hw/9pfs/9p-xattr.c
index 803d4bbbc50b..72ef820f16d7 100644
--- a/hw/9pfs/9p-xattr.c
+++ b/hw/9pfs/9p-xattr.c
@@ -328,13 +328,7 @@ ssize_t pt_getxattr(FsContext *ctx, const char *path, const char *name,
 int pt_setxattr(FsContext *ctx, const char *path, const char *name, void *value,
                 size_t size, int flags)
 {
-    char *buffer;
-    int ret;
-
-    buffer = rpath(ctx, path);
-    ret = lsetxattr(buffer, name, value, size, flags);
-    g_free(buffer);
-    return ret;
+    return local_setxattr_nofollow(ctx, path, name, value, size, flags);
 }
 
 int pt_removexattr(FsContext *ctx, const char *path, const char *name)

[Qemu-devel] [PATCH 11/29] 9pfs: local: lremovexattr: don't follow symlinks

[Greg Kurz]

The local_lremovexattr() callback is vulnerable to symlink attacks because
it calls lremovexattr() which follows symbolic links in all path elements but
the rightmost one.

This patch converts local_lremovexattr() to rely on opendir_nofollow() and
fremovexattrat_nofollow() instead.

This partly fixes CVE-2016-9602.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-posix-acl.c  |   10 ++--------
 hw/9pfs/9p-xattr-user.c |    8 +-------
 hw/9pfs/9p-xattr.c      |    8 +-------
 3 files changed, 4 insertions(+), 22 deletions(-)

diff --git a/hw/9pfs/9p-posix-acl.c b/hw/9pfs/9p-posix-acl.c
index 0154e2a7605f..c20409104135 100644
--- a/hw/9pfs/9p-posix-acl.c
+++ b/hw/9pfs/9p-posix-acl.c
@@ -58,10 +58,8 @@ static int mp_pacl_removexattr(FsContext *ctx,
                                const char *path, const char *name)
 {
     int ret;
-    char *buffer;
 
-    buffer = rpath(ctx, path);
-    ret  = lremovexattr(buffer, MAP_ACL_ACCESS);
+    ret = local_removexattr_nofollow(ctx, MAP_ACL_ACCESS, name);
     if (ret == -1 && errno == ENODATA) {
         /*
          * We don't get ENODATA error when trying to remove a
@@ -71,7 +69,6 @@ static int mp_pacl_removexattr(FsContext *ctx,
         errno = 0;
         ret = 0;
     }
-    g_free(buffer);
     return ret;
 }
 
@@ -111,10 +108,8 @@ static int mp_dacl_removexattr(FsContext *ctx,
                                const char *path, const char *name)
 {
     int ret;
-    char *buffer;
 
-    buffer = rpath(ctx, path);
-    ret  = lremovexattr(buffer, MAP_ACL_DEFAULT);
+    ret = local_removexattr_nofollow(ctx, MAP_ACL_DEFAULT, name);
     if (ret == -1 && errno == ENODATA) {
         /*
          * We don't get ENODATA error when trying to remove a
@@ -124,7 +119,6 @@ static int mp_dacl_removexattr(FsContext *ctx,
         errno = 0;
         ret = 0;
     }
-    g_free(buffer);
     return ret;
 }
 
diff --git a/hw/9pfs/9p-xattr-user.c b/hw/9pfs/9p-xattr-user.c
index 1840a5db66f3..2c90817b75a6 100644
--- a/hw/9pfs/9p-xattr-user.c
+++ b/hw/9pfs/9p-xattr-user.c
@@ -81,9 +81,6 @@ static int mp_user_setxattr(FsContext *ctx, const char *path, const char *name,
 static int mp_user_removexattr(FsContext *ctx,
                                const char *path, const char *name)
 {
-    char *buffer;
-    int ret;
-
     if (strncmp(name, "user.virtfs.", 12) == 0) {
         /*
          * Don't allow fetch of user.virtfs namesapce
@@ -92,10 +89,7 @@ static int mp_user_removexattr(FsContext *ctx,
         errno = EACCES;
         return -1;
     }
-    buffer = rpath(ctx, path);
-    ret = lremovexattr(buffer, name);
-    g_free(buffer);
-    return ret;
+    return local_removexattr_nofollow(ctx, path, name);
 }
 
 XattrOperations mapped_user_xattr = {
diff --git a/hw/9pfs/9p-xattr.c b/hw/9pfs/9p-xattr.c
index 72ef820f16d7..6fbfbca7e9a0 100644
--- a/hw/9pfs/9p-xattr.c
+++ b/hw/9pfs/9p-xattr.c
@@ -333,13 +333,7 @@ int pt_setxattr(FsContext *ctx, const char *path, const char *name, void *value,
 
 int pt_removexattr(FsContext *ctx, const char *path, const char *name)
 {
-    char *buffer;
-    int ret;
-
-    buffer = rpath(ctx, path);
-    ret = lremovexattr(path, name);
-    g_free(buffer);
-    return ret;
+    return local_removexattr_nofollow(ctx, path, name);
 }
 
 ssize_t notsup_getxattr(FsContext *ctx, const char *path, const char *name,

[Qemu-devel] [PATCH 12/29] 9pfs: local: unlinkat: don't follow symlinks

[Greg Kurz]

The local_unlinkat() callback is vulnerable to symlink attacks because it
calls remove() which follows symbolic links in all path elements but the
rightmost one.

This patch converts local_unlinkat() to rely on opendir_nofollow() and
unlinkat() instead.

Most of the code is moved to a separate local_unlinkat_common() helper
which will be reused in a subsequent patch to fix the same issue in
local_remove().

This partly fixes CVE-2016-9602.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |  100 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 57 insertions(+), 43 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 26ea72c66306..21522ca7de43 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -963,6 +963,57 @@ static int local_utimensat(FsContext *s, V9fsPath *fs_path,
     return ret;
 }
 
+static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name,
+                                 int flags)
+{
+    int ret = -1;
+
+    if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
+        int map_dirfd;
+
+        if (flags == AT_REMOVEDIR) {
+            int fd;
+
+            fd = openat(dirfd, name, O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
+            if (fd == -1) {
+                goto err_out;
+            }
+            /*
+             * If directory remove .virtfs_metadata contained in the
+             * directory
+             */
+            ret = unlinkat(fd, VIRTFS_META_DIR, AT_REMOVEDIR);
+            close_preserve_errno(fd);
+            if (ret < 0 && errno != ENOENT) {
+                /*
+                 * We didn't had the .virtfs_metadata file. May be file created
+                 * in non-mapped mode ?. Ignore ENOENT.
+                 */
+                goto err_out;
+            }
+        }
+        /*
+         * Now remove the name from parent directory
+         * .virtfs_metadata directory.
+         */
+        map_dirfd = openat(dirfd, VIRTFS_META_DIR,
+                           O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
+        ret = unlinkat(map_dirfd, name, 0);
+        close_preserve_errno(map_dirfd);
+        if (ret < 0 && errno != ENOENT) {
+            /*
+             * We didn't had the .virtfs_metadata file. May be file created
+             * in non-mapped mode ?. Ignore ENOENT.
+             */
+            goto err_out;
+        }
+    }
+
+    ret = unlinkat(dirfd, name, flags);
+err_out:
+    return ret;
+}
+
 static int local_remove(FsContext *ctx, const char *path)
 {
     int err;
@@ -1112,52 +1163,15 @@ static int local_unlinkat(FsContext *ctx, V9fsPath *dir,
                           const char *name, int flags)
 {
     int ret;
-    V9fsString fullname;
-    char *buffer;
-
-    v9fs_string_init(&fullname);
+    int dirfd;
 
-    v9fs_string_sprintf(&fullname, "%s/%s", dir->data, name);
-    if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-        if (flags == AT_REMOVEDIR) {
-            /*
-             * If directory remove .virtfs_metadata contained in the
-             * directory
-             */
-            buffer = g_strdup_printf("%s/%s/%s", ctx->fs_root,
-                                     fullname.data, VIRTFS_META_DIR);
-            ret = remove(buffer);
-            g_free(buffer);
-            if (ret < 0 && errno != ENOENT) {
-                /*
-                 * We didn't had the .virtfs_metadata file. May be file created
-                 * in non-mapped mode ?. Ignore ENOENT.
-                 */
-                goto err_out;
-            }
-        }
-        /*
-         * Now remove the name from parent directory
-         * .virtfs_metadata directory.
-         */
-        buffer = local_mapped_attr_path(ctx, fullname.data);
-        ret = remove(buffer);
-        g_free(buffer);
-        if (ret < 0 && errno != ENOENT) {
-            /*
-             * We didn't had the .virtfs_metadata file. May be file created
-             * in non-mapped mode ?. Ignore ENOENT.
-             */
-            goto err_out;
-        }
+    dirfd = local_opendir_nofollow(ctx, dir->data);
+    if (dirfd == -1) {
+        return -1;
     }
-    /* Remove the name finally */
-    buffer = rpath(ctx, fullname.data);
-    ret = remove(buffer);
-    g_free(buffer);
 
-err_out:
-    v9fs_string_free(&fullname);
+    ret = local_unlinkat_common(ctx, dirfd, name, flags);
+    close_preserve_errno(dirfd);
     return ret;
 }
 

[Qemu-devel] [PATCH 13/29] 9pfs: local: remove: don't follow symlinks

[Greg Kurz]

The local_remove() callback is vulnerable to symlink attacks because it
calls:

(1) lstat() which follows symbolic links in all path elements but the
    rightmost one
(2) remove() which follows symbolic links in all path elements but the
    rightmost one

This patch converts local_remove() to rely on opendir_nofollow(),
fstatat(AT_SYMLINK_NOFOLLOW) to fix (1) and unlinkat() to fix (2).

This partly fixes CVE-2016-9602.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   64 +++++++++++++++++-----------------------------------
 1 file changed, 21 insertions(+), 43 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 21522ca7de43..c6f4c8d95442 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1016,54 +1016,32 @@ err_out:
 
 static int local_remove(FsContext *ctx, const char *path)
 {
-    int err;
     struct stat stbuf;
-    char *buffer;
+    char *dirpath = g_path_get_dirname(path);
+    char *name = g_path_get_basename(path);
+    int flags = 0;
+    int dirfd;
+    int err = -1;
 
-    if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-        buffer = rpath(ctx, path);
-        err =  lstat(buffer, &stbuf);
-        g_free(buffer);
-        if (err) {
-            goto err_out;
-        }
-        /*
-         * If directory remove .virtfs_metadata contained in the
-         * directory
-         */
-        if (S_ISDIR(stbuf.st_mode)) {
-            buffer = g_strdup_printf("%s/%s/%s", ctx->fs_root,
-                                     path, VIRTFS_META_DIR);
-            err = remove(buffer);
-            g_free(buffer);
-            if (err < 0 && errno != ENOENT) {
-                /*
-                 * We didn't had the .virtfs_metadata file. May be file created
-                 * in non-mapped mode ?. Ignore ENOENT.
-                 */
-                goto err_out;
-            }
-        }
-        /*
-         * Now remove the name from parent directory
-         * .virtfs_metadata directory
-         */
-        buffer = local_mapped_attr_path(ctx, path);
-        err = remove(buffer);
-        g_free(buffer);
-        if (err < 0 && errno != ENOENT) {
-            /*
-             * We didn't had the .virtfs_metadata file. May be file created
-             * in non-mapped mode ?. Ignore ENOENT.
-             */
-            goto err_out;
-        }
+    dirfd = local_opendir_nofollow(ctx, dirpath);
+    if (dirfd) {
+        goto out;
     }
 
-    buffer = rpath(ctx, path);
-    err = remove(buffer);
-    g_free(buffer);
+    if (fstatat(dirfd, path, &stbuf, AT_SYMLINK_NOFOLLOW) < 0) {
+        goto err_out;
+    }
+
+    if (S_ISDIR(stbuf.st_mode)) {
+        flags |= AT_REMOVEDIR;
+    }
+
+    err = local_unlinkat_common(ctx, dirfd, name, flags);
 err_out:
+    close_preserve_errno(dirfd);
+out:
+    g_free(name);
+    g_free(dirpath);
     return err;
 }
 

[Qemu-devel] [PATCH 14/29] 9pfs: local: utimensat: don't follow symlinks

[Greg Kurz]

The local_utimensat() callback is vulnerable to symlink attacks because it
calls qemu_utimens()->utimensat(AT_SYMLINK_NOFOLLOW) which follows symbolic
links in all path elements but the rightmost one or qemu_utimens()->utimes()
which follows symbolic links for all path elements.

This patch converts local_utimensat() to rely on opendir_nofollow() and
utimensat(AT_SYMLINK_NOFOLLOW) directly instead of using qemu_utimens().
It is hence assumed that the OS supports utimensat(), i.e. has glibc 2.6
or higher and linux 2.6.22 or higher, which seems reasonable nowadays.

This partly fixes CVE-2016-9602.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index c6f4c8d95442..7f3d9dd9a499 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -953,13 +953,20 @@ static int local_chown(FsContext *fs_ctx, V9fsPath *fs_path, FsCred *credp)
 static int local_utimensat(FsContext *s, V9fsPath *fs_path,
                            const struct timespec *buf)
 {
-    char *buffer;
-    int ret;
-    char *path = fs_path->data;
+    char *dirpath = g_path_get_dirname(fs_path->data);
+    char *name = g_path_get_basename(fs_path->data);
+    int dirfd, ret = -1;
 
-    buffer = rpath(s, path);
-    ret = qemu_utimens(buffer, buf);
-    g_free(buffer);
+    dirfd = local_opendir_nofollow(s, dirpath);
+    if (dirfd == -1) {
+        goto out;
+    }
+
+    ret = utimensat(dirfd, name, buf, AT_SYMLINK_NOFOLLOW);
+    close_preserve_errno(dirfd);
+out:
+    g_free(dirpath);
+    g_free(name);
     return ret;
 }
 

[Qemu-devel] [PATCH 15/29] 9pfs: local: statfs: don't follow symlinks

[Greg Kurz]

The local_statfs() callback is vulnerable to symlink attacks because it
calls statfs() which follows symbolic links in all path elements.

This patch converts local_statfs() to rely on open_nofollow() and fstatfs()
instead.

This partly fixes CVE-2016-9602.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 7f3d9dd9a499..ebc12f3c0691 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1072,13 +1072,11 @@ static int local_fsync(FsContext *ctx, int fid_type,
 
 static int local_statfs(FsContext *s, V9fsPath *fs_path, struct statfs *stbuf)
 {
-    char *buffer;
-    int ret;
-    char *path = fs_path->data;
+    int fd, ret;
 
-    buffer = rpath(s, path);
-    ret = statfs(buffer, stbuf);
-    g_free(buffer);
+    fd = local_open_nofollow(s, fs_path->data, O_RDONLY, 0);
+    ret = fstatfs(fd, stbuf);
+    close_preserve_errno(fd);
     return ret;
 }
 

[Qemu-devel] [PATCH 16/29] 9pfs: local: truncate: don't follow symlinks

[Greg Kurz]

The local_truncate() callback is vulnerable to symlink attacks because
it calls truncate() which follows symbolic links in all path elements.

This patch converts local_truncate() to rely on open_nofollow() and
ftruncate() instead.

This partly fixes CVE-2016-9602.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index ebc12f3c0691..da233198d032 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -888,13 +888,14 @@ err_out:
 
 static int local_truncate(FsContext *ctx, V9fsPath *fs_path, off_t size)
 {
-    char *buffer;
-    int ret;
-    char *path = fs_path->data;
+    int fd, ret;
 
-    buffer = rpath(ctx, path);
-    ret = truncate(buffer, size);
-    g_free(buffer);
+    fd = local_open_nofollow(ctx, fs_path->data, O_WRONLY, 0);
+    if (fd == -1) {
+        return -1;
+    }
+    ret = ftruncate(fd, size);
+    close_preserve_errno(fd);
     return ret;
 }
 

[Qemu-devel] [PATCH 17/29] 9pfs: local: readlink: don't follow symlinks

[Greg Kurz]

The local_readlink() callback is vulnerable to symlink attacks because it
calls:

(1) open(O_NOFOLLOW) which follows symbolic links for all path elements but
    the rightmost one
(2) readlink() which follows symbolic links for all path elements but the
    rightmost one

This patch converts local_readlink() to rely on open_nofollow() to fix (1)
and opendir_nofollow(), readlinkat() to fix (2).

This partly fixes CVE-2016-9602.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index da233198d032..dbd5549283aa 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -334,27 +334,35 @@ static ssize_t local_readlink(FsContext *fs_ctx, V9fsPath *fs_path,
                               char *buf, size_t bufsz)
 {
     ssize_t tsize = -1;
-    char *buffer;
-    char *path = fs_path->data;
 
     if ((fs_ctx->export_flags & V9FS_SM_MAPPED) ||
         (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE)) {
         int fd;
-        buffer = rpath(fs_ctx, path);
-        fd = open(buffer, O_RDONLY | O_NOFOLLOW);
-        g_free(buffer);
+
+        fd = local_open_nofollow(fs_ctx, fs_path->data, O_RDONLY, 0);
         if (fd == -1) {
             return -1;
         }
         do {
             tsize = read(fd, (void *)buf, bufsz);
         } while (tsize == -1 && errno == EINTR);
-        close(fd);
+        close_preserve_errno(fd);
     } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) ||
                (fs_ctx->export_flags & V9FS_SM_NONE)) {
-        buffer = rpath(fs_ctx, path);
-        tsize = readlink(buffer, buf, bufsz);
-        g_free(buffer);
+        char *dirpath = g_path_get_dirname(fs_path->data);
+        char *name = g_path_get_basename(fs_path->data);
+        int dirfd;
+
+        dirfd = local_opendir_nofollow(fs_ctx, dirpath);
+        if (dirfd == -1) {
+            goto out;
+        }
+
+        tsize = readlinkat(dirfd, name, buf, bufsz);
+        close_preserve_errno(dirfd);
+    out:
+        g_free(name);
+        g_free(dirpath);
     }
     return tsize;
 }

[Qemu-devel] [PATCH 18/29] 9pfs: local: lstat: don't follow symlinks

[Greg Kurz]

The local_lstat() callback is vulnerable to symlink attacks because it
calls:

(1) lstat() which follows symbolic links in all path elements but the
    rightmost one
(2) getxattr() which follows symbolic links in all path elements
(3) local_mapped_file_attr()->local_fopen()->openat(O_NOFOLLOW) which
    follows symbolic links in all path elements but the rightmost
    one

This patch converts local_lstat() to rely on opendir_nofollow() and
fstatat(AT_SYMLINK_NOFOLLOW) to fix (1), fgetxattrat_nofollow() to
fix (2).

A new local_fopenat() helper is introduced as a replacement to
local_fopen() to fix (3). No effort is made to factor out code
because local_fopen() will be dropped when all users have been
converted to call local_fopenat().

This partly fixes CVE-2016-9602.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   78 +++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 61 insertions(+), 17 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index dbd5549283aa..b7bf1a11feb3 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -105,17 +105,49 @@ static FILE *local_fopen(const char *path, const char *mode)
     return fp;
 }
 
+static FILE *local_fopenat(int dirfd, const char *name, const char *mode)
+{
+    int fd, o_mode = 0;
+    FILE *fp;
+    int flags = O_NOFOLLOW;
+    /*
+     * only supports two modes
+     */
+    if (mode[0] == 'r') {
+        flags |= O_RDONLY;
+    } else if (mode[0] == 'w') {
+        flags |= O_WRONLY | O_TRUNC | O_CREAT;
+        o_mode = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH;
+    } else {
+        return NULL;
+    }
+    fd = openat(dirfd, name, flags, o_mode);
+    if (fd == -1) {
+        return NULL;
+    }
+    fp = fdopen(fd, mode);
+    if (!fp) {
+        close(fd);
+    }
+    return fp;
+}
+
 #define ATTR_MAX 100
-static void local_mapped_file_attr(FsContext *ctx, const char *path,
+static void local_mapped_file_attr(int dirfd, const char *name,
                                    struct stat *stbuf)
 {
     FILE *fp;
     char buf[ATTR_MAX];
-    char *attr_path;
+    int map_dirfd;
 
-    attr_path = local_mapped_attr_path(ctx, path);
-    fp = local_fopen(attr_path, "r");
-    g_free(attr_path);
+    map_dirfd = openat(dirfd, VIRTFS_META_DIR,
+                       O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
+    if (map_dirfd == -1) {
+        return;
+    }
+
+    fp = local_fopenat(map_dirfd, name, "r");
+    close_preserve_errno(map_dirfd);
     if (!fp) {
         return;
     }
@@ -137,12 +169,17 @@ static void local_mapped_file_attr(FsContext *ctx, const char *path,
 
 static int local_lstat(FsContext *fs_ctx, V9fsPath *fs_path, struct stat *stbuf)
 {
-    int err;
-    char *buffer;
-    char *path = fs_path->data;
+    int err = -1;
+    char *dirpath = g_path_get_dirname(fs_path->data);
+    char *name = g_path_get_basename(fs_path->data);
+    int dirfd;
 
-    buffer = rpath(fs_ctx, path);
-    err =  lstat(buffer, stbuf);
+    dirfd = local_opendir_nofollow(fs_ctx, dirpath);
+    if (dirfd == -1) {
+        goto out;
+    }
+
+    err = fstatat(dirfd, name, stbuf, AT_SYMLINK_NOFOLLOW);
     if (err) {
         goto err_out;
     }
@@ -152,25 +189,32 @@ static int local_lstat(FsContext *fs_ctx, V9fsPath *fs_path, struct stat *stbuf)
         gid_t tmp_gid;
         mode_t tmp_mode;
         dev_t tmp_dev;
-        if (getxattr(buffer, "user.virtfs.uid", &tmp_uid, sizeof(uid_t)) > 0) {
+
+        if (fgetxattrat_nofollow(dirfd, name, "user.virtfs.uid", &tmp_uid,
+                                 sizeof(uid_t)) > 0) {
             stbuf->st_uid = le32_to_cpu(tmp_uid);
         }
-        if (getxattr(buffer, "user.virtfs.gid", &tmp_gid, sizeof(gid_t)) > 0) {
+        if (fgetxattrat_nofollow(dirfd, name, "user.virtfs.gid", &tmp_gid,
+                                 sizeof(gid_t)) > 0) {
             stbuf->st_gid = le32_to_cpu(tmp_gid);
         }
-        if (getxattr(buffer, "user.virtfs.mode",
-                    &tmp_mode, sizeof(mode_t)) > 0) {
+        if (fgetxattrat_nofollow(dirfd, name, "user.virtfs.mode", &tmp_mode,
+                                 sizeof(mode_t)) > 0) {
             stbuf->st_mode = le32_to_cpu(tmp_mode);
         }
-        if (getxattr(buffer, "user.virtfs.rdev", &tmp_dev, sizeof(dev_t)) > 0) {
+        if (fgetxattrat_nofollow(dirfd, name, "user.virtfs.rdev", &tmp_dev,
+                                 sizeof(dev_t)) > 0) {
             stbuf->st_rdev = le64_to_cpu(tmp_dev);
         }
     } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-        local_mapped_file_attr(fs_ctx, path, stbuf);
+        local_mapped_file_attr(dirfd, name, stbuf);
     }
 
 err_out:
-    g_free(buffer);
+    close_preserve_errno(dirfd);
+out:
+    g_free(name);
+    g_free(dirpath);
     return err;
 }
 

[Qemu-devel] [PATCH 19/29] 9pfs: local: renameat: don't follow symlinks

[Greg Kurz]

The local_renameat() callback is currently a wrapper around local_rename()
which is vulnerable to symlink attacks.

This patch rewrites local_renameat() to have its own implementation, based
on local_opendir_nofollow() and renameat().

This partly fixes CVE-2016-9602.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   71 ++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 63 insertions(+), 8 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index b7bf1a11feb3..07b7110d87d7 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -61,6 +61,14 @@ int local_opendir_nofollow(FsContext *fs_ctx, const char *path)
     return local_open_nofollow(fs_ctx, path, O_DIRECTORY | O_RDONLY, 0);
 }
 
+static void renameat_preserve_errno(int odirfd, const char *opath, int ndirfd,
+                                    const char *npath)
+{
+    int serrno = errno;
+    renameat(odirfd, opath, ndirfd, npath);
+    errno = serrno;
+}
+
 #define VIRTFS_META_DIR ".virtfs_metadata"
 
 static char *local_mapped_attr_path(FsContext *ctx, const char *path)
@@ -1181,17 +1189,64 @@ static int local_renameat(FsContext *ctx, V9fsPath *olddir,
                           const char *new_name)
 {
     int ret;
-    V9fsString old_full_name, new_full_name;
+    int odirfd, ndirfd;
+
+    odirfd = local_opendir_nofollow(ctx, olddir->data);
+    if (odirfd == -1) {
+        return -1;
+    }
+
+    ndirfd = local_opendir_nofollow(ctx, newdir->data);
+    if (ndirfd == -1) {
+        close_preserve_errno(odirfd);
+        return -1;
+    }
+
+    ret = renameat(odirfd, old_name, ndirfd, new_name);
+    if (ret < 0) {
+        goto out;
+    }
 
-    v9fs_string_init(&old_full_name);
-    v9fs_string_init(&new_full_name);
+    if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
+        int omap_dirfd, nmap_dirfd;
 
-    v9fs_string_sprintf(&old_full_name, "%s/%s", olddir->data, old_name);
-    v9fs_string_sprintf(&new_full_name, "%s/%s", newdir->data, new_name);
+        ret = mkdirat(ndirfd, VIRTFS_META_DIR, 0700);
+        if (ret < 0 && errno != EEXIST) {
+            goto err_undo_rename;
+        }
 
-    ret = local_rename(ctx, old_full_name.data, new_full_name.data);
-    v9fs_string_free(&old_full_name);
-    v9fs_string_free(&new_full_name);
+        omap_dirfd = openat(odirfd, VIRTFS_META_DIR,
+                            O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
+        if (omap_dirfd == -1) {
+            goto err;
+        }
+
+        nmap_dirfd = openat(ndirfd, VIRTFS_META_DIR,
+                            O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
+        if (nmap_dirfd == -1) {
+            close_preserve_errno(omap_dirfd);
+            goto err;
+        }
+
+        /* rename the .virtfs_metadata files */
+        ret = renameat(omap_dirfd, old_name, nmap_dirfd, new_name);
+        close_preserve_errno(nmap_dirfd);
+        close_preserve_errno(omap_dirfd);
+        if (ret < 0 && errno != ENOENT) {
+            goto err_undo_rename;
+        }
+
+        ret = 0;
+    }
+    goto out;
+
+err:
+    ret = -1;
+err_undo_rename:
+    renameat_preserve_errno(ndirfd, new_name, odirfd, old_name);
+out:
+    close_preserve_errno(ndirfd);
+    close_preserve_errno(odirfd);
     return ret;
 }
 

[Qemu-devel] [PATCH 20/29] 9pfs: local: rename: use renameat

[Greg Kurz]

The local_rename() callback is vulnerable to symlink attacks because it
uses rename() which follows symbolic links in all path elements but the
rightmost one.

This patch simply transforms local_rename() into a wrapper around
local_renameat() which is symlink-attack safe.

This partly fixes CVE-2016-9602.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   57 +++++++++++++++++++++++++---------------------------
 1 file changed, 27 insertions(+), 30 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 07b7110d87d7..15e746ede86a 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -959,36 +959,6 @@ static int local_truncate(FsContext *ctx, V9fsPath *fs_path, off_t size)
     return ret;
 }
 
-static int local_rename(FsContext *ctx, const char *oldpath,
-                        const char *newpath)
-{
-    int err;
-    char *buffer, *buffer1;
-
-    if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-        err = local_create_mapped_attr_dir(ctx, newpath);
-        if (err < 0) {
-            return err;
-        }
-        /* rename the .virtfs_metadata files */
-        buffer = local_mapped_attr_path(ctx, oldpath);
-        buffer1 = local_mapped_attr_path(ctx, newpath);
-        err = rename(buffer, buffer1);
-        g_free(buffer);
-        g_free(buffer1);
-        if (err < 0 && errno != ENOENT) {
-            return err;
-        }
-    }
-
-    buffer = rpath(ctx, oldpath);
-    buffer1 = rpath(ctx, newpath);
-    err = rename(buffer, buffer1);
-    g_free(buffer);
-    g_free(buffer1);
-    return err;
-}
-
 static int local_chown(FsContext *fs_ctx, V9fsPath *fs_path, FsCred *credp)
 {
     char *buffer;
@@ -1250,6 +1220,33 @@ out:
     return ret;
 }
 
+static void v9fs_path_init_dirname(V9fsPath *path, const char *str)
+{
+    path->data = g_path_get_dirname(str);
+    path->size = strlen(path->data) + 1;
+}
+
+static int local_rename(FsContext *ctx, const char *oldpath,
+                        const char *newpath)
+{
+    int err;
+    char *oname = g_path_get_basename(oldpath);
+    char *nname = g_path_get_basename(newpath);
+    V9fsPath olddir, newdir;
+
+    v9fs_path_init_dirname(&olddir, oldpath);
+    v9fs_path_init_dirname(&newdir, newpath);
+
+    err = local_renameat(ctx, &olddir, oname, &newdir, nname);
+
+    v9fs_path_free(&newdir);
+    v9fs_path_free(&olddir);
+    g_free(nname);
+    g_free(oname);
+
+    return err;
+}
+
 static int local_unlinkat(FsContext *ctx, V9fsPath *dir,
                           const char *name, int flags)
 {

[Qemu-devel] [PATCH 21/29] 9pfs: local: improve error handling in link op

[Greg Kurz]

When using the mapped-file security model, we also have to create a link
for the metadata file if it exists. In case of failure, we should rollback.

That's what this patch does.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 15e746ede86a..9b9c39ee42ee 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -915,6 +915,7 @@ static int local_link(FsContext *ctx, V9fsPath *oldpath,
     int ret;
     V9fsString newpath;
     char *buffer, *buffer1;
+    int serrno;
 
     v9fs_string_init(&newpath);
     v9fs_string_sprintf(&newpath, "%s/%s", dirpath->data, name);
@@ -923,25 +924,36 @@ static int local_link(FsContext *ctx, V9fsPath *oldpath,
     buffer1 = rpath(ctx, newpath.data);
     ret = link(buffer, buffer1);
     g_free(buffer);
-    g_free(buffer1);
+    if (ret < 0) {
+        goto out;
+    }
 
     /* now link the virtfs_metadata files */
-    if (!ret && (ctx->export_flags & V9FS_SM_MAPPED_FILE)) {
+    if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
+        char *vbuffer, *vbuffer1;
+
         /* Link the .virtfs_metadata files. Create the metada directory */
         ret = local_create_mapped_attr_dir(ctx, newpath.data);
         if (ret < 0) {
             goto err_out;
         }
-        buffer = local_mapped_attr_path(ctx, oldpath->data);
-        buffer1 = local_mapped_attr_path(ctx, newpath.data);
-        ret = link(buffer, buffer1);
-        g_free(buffer);
-        g_free(buffer1);
+        vbuffer = local_mapped_attr_path(ctx, oldpath->data);
+        vbuffer1 = local_mapped_attr_path(ctx, newpath.data);
+        ret = link(vbuffer, vbuffer1);
+        g_free(vbuffer);
+        g_free(vbuffer1);
         if (ret < 0 && errno != ENOENT) {
             goto err_out;
         }
     }
+    goto out;
+
 err_out:
+    serrno = errno;
+    remove(buffer1);
+    errno = serrno;
+out:
+    g_free(buffer1);
     v9fs_string_free(&newpath);
     return ret;
 }

[Qemu-devel] [PATCH 22/29] 9pfs: local: link: don't follow symlinks

[Greg Kurz]

The local_link() callback is vulnerable to symlink attacks because it calls:

(1) link() which follows symbolic links for all path elements but the
    rightmost one
(2) local_create_mapped_attr_dir()->mkdir() which follows symbolic links
    for all path elements but the rightmost one

This patch converts local_link() to rely on opendir_nofollow() and linkat()
to fix (1), mkdirat() to fix (2).

This partly fixes CVE-2016-9602.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   86 ++++++++++++++++++++++++++++++++++------------------
 1 file changed, 57 insertions(+), 29 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 9b9c39ee42ee..fb536fdb3082 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -69,6 +69,13 @@ static void renameat_preserve_errno(int odirfd, const char *opath, int ndirfd,
     errno = serrno;
 }
 
+static void unlinkat_preserve_errno(int dirfd, const char *path, int flags)
+{
+    int serrno = errno;
+    unlinkat(dirfd, path, flags);
+    errno = serrno;
+}
+
 #define VIRTFS_META_DIR ".virtfs_metadata"
 
 static char *local_mapped_attr_path(FsContext *ctx, const char *path)
@@ -912,49 +919,70 @@ out:
 static int local_link(FsContext *ctx, V9fsPath *oldpath,
                       V9fsPath *dirpath, const char *name)
 {
-    int ret;
-    V9fsString newpath;
-    char *buffer, *buffer1;
-    int serrno;
+    char *odirpath = g_path_get_dirname(oldpath->data);
+    char *oname = g_path_get_basename(oldpath->data);
+    int ret = -1;
+    int odirfd, ndirfd;
 
-    v9fs_string_init(&newpath);
-    v9fs_string_sprintf(&newpath, "%s/%s", dirpath->data, name);
+    odirfd = local_opendir_nofollow(ctx, odirpath);
+    if (odirfd == -1) {
+        goto out;
+    }
 
-    buffer = rpath(ctx, oldpath->data);
-    buffer1 = rpath(ctx, newpath.data);
-    ret = link(buffer, buffer1);
-    g_free(buffer);
-    if (ret < 0) {
+    ndirfd = local_opendir_nofollow(ctx, dirpath->data);
+    if (ndirfd == -1) {
+        close_preserve_errno(odirfd);
         goto out;
     }
 
+    ret = linkat(odirfd, oname, ndirfd, name, 0);
+    if (ret < 0) {
+        goto out_close;
+    }
+
     /* now link the virtfs_metadata files */
     if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-        char *vbuffer, *vbuffer1;
+        int omap_dirfd, nmap_dirfd;
 
-        /* Link the .virtfs_metadata files. Create the metada directory */
-        ret = local_create_mapped_attr_dir(ctx, newpath.data);
-        if (ret < 0) {
-            goto err_out;
+        ret = mkdirat(ndirfd, VIRTFS_META_DIR, 0700);
+        if (ret < 0 && errno != EEXIST) {
+            goto err_undo_link;
         }
-        vbuffer = local_mapped_attr_path(ctx, oldpath->data);
-        vbuffer1 = local_mapped_attr_path(ctx, newpath.data);
-        ret = link(vbuffer, vbuffer1);
-        g_free(vbuffer);
-        g_free(vbuffer1);
+
+        omap_dirfd = openat(odirfd, VIRTFS_META_DIR,
+                            O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
+        if (omap_dirfd == -1) {
+            goto err;
+        }
+
+        nmap_dirfd = openat(ndirfd, VIRTFS_META_DIR,
+                            O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
+        if (nmap_dirfd == -1) {
+            close_preserve_errno(omap_dirfd);
+            goto err;
+        }
+
+        ret = linkat(omap_dirfd, oname, nmap_dirfd, name, 0);
+        close_preserve_errno(nmap_dirfd);
+        close_preserve_errno(omap_dirfd);
         if (ret < 0 && errno != ENOENT) {
-            goto err_out;
+            goto err_undo_link;
         }
+
+        ret = 0;
     }
-    goto out;
+    goto out_close;
 
-err_out:
-    serrno = errno;
-    remove(buffer1);
-    errno = serrno;
+err:
+    ret = -1;
+err_undo_link:
+    unlinkat_preserve_errno(ndirfd, name, 0);
+out_close:
+    close_preserve_errno(ndirfd);
+    close_preserve_errno(odirfd);
 out:
-    g_free(buffer1);
-    v9fs_string_free(&newpath);
+    g_free(oname);
+    g_free(odirpath);
     return ret;
 }
 

[Qemu-devel] [PATCH 23/29] 9pfs: local: chmod: don't follow symlinks

[Greg Kurz]

The local_chmod() callback is vulnerable to symlink attacks because it
calls:

(1) chmod() which follows symbolic links for all path elements
(2) local_set_xattr()->setxattr() which follows symbolic links for all
    path elements
(3) local_set_mapped_file_attr() which calls in turn local_fopen() and
    mkdir(), both functions following symbolic links for all path
    elements but the rightmost one

This patch converts local_chmod() to rely on open_nofollow() and
fchmod() to fix (1).

It introduces a local_set_xattrat() replacement to local_set_xattr()
based on fsetxattrat() to fix (2), and a local_set_mapped_file_attrat()
replacement to local_set_mapped_file_attr() based on local_fopenat()
and mkdirat() to fix (3). No effort is made to factor out code because
both local_set_xattr() and local_set_mapped_file_attr() will be dropped
when all users have been converted to use the "at" versions.

This partly fixes CVE-2016-9602.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |  163 ++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 150 insertions(+), 13 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index fb536fdb3082..56d7731f7ce1 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -327,6 +327,87 @@ err_out:
     return ret;
 }
 
+static int local_set_mapped_file_attrat(int dirfd, const char *name,
+                                        FsCred *credp)
+{
+    FILE *fp;
+    int ret;
+    char buf[ATTR_MAX];
+    int uid = -1, gid = -1, mode = -1, rdev = -1;
+    int map_dirfd;
+
+    ret = mkdirat(dirfd, VIRTFS_META_DIR, 0700);
+    if (ret < 0 && errno != EEXIST) {
+        return -1;
+    }
+
+    map_dirfd = openat(dirfd, VIRTFS_META_DIR,
+                       O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
+    if (map_dirfd == -1) {
+        return -1;
+    }
+
+    fp = local_fopenat(map_dirfd, name, "r");
+    if (!fp) {
+        if (errno == ENOENT) {
+            goto update_map_file;
+        } else {
+            close_preserve_errno(map_dirfd);
+            return -1;
+        }
+    }
+    memset(buf, 0, ATTR_MAX);
+    while (fgets(buf, ATTR_MAX, fp)) {
+        if (!strncmp(buf, "virtfs.uid", 10)) {
+            uid = atoi(buf + 11);
+        } else if (!strncmp(buf, "virtfs.gid", 10)) {
+            gid = atoi(buf + 11);
+        } else if (!strncmp(buf, "virtfs.mode", 11)) {
+            mode = atoi(buf + 12);
+        } else if (!strncmp(buf, "virtfs.rdev", 11)) {
+            rdev = atoi(buf + 12);
+        }
+        memset(buf, 0, ATTR_MAX);
+    }
+    fclose(fp);
+
+update_map_file:
+    fp = local_fopenat(map_dirfd, name, "w");
+    close_preserve_errno(map_dirfd);
+    if (!fp) {
+        return -1;
+    }
+
+    if (credp->fc_uid != -1) {
+        uid = credp->fc_uid;
+    }
+    if (credp->fc_gid != -1) {
+        gid = credp->fc_gid;
+    }
+    if (credp->fc_mode != -1) {
+        mode = credp->fc_mode;
+    }
+    if (credp->fc_rdev != -1) {
+        rdev = credp->fc_rdev;
+    }
+
+    if (uid != -1) {
+        fprintf(fp, "virtfs.uid=%d\n", uid);
+    }
+    if (gid != -1) {
+        fprintf(fp, "virtfs.gid=%d\n", gid);
+    }
+    if (mode != -1) {
+        fprintf(fp, "virtfs.mode=%d\n", mode);
+    }
+    if (rdev != -1) {
+        fprintf(fp, "virtfs.rdev=%d\n", rdev);
+    }
+    fclose(fp);
+
+    return 0;
+}
+
 static int local_set_xattr(const char *path, FsCred *credp)
 {
     int err;
@@ -362,6 +443,45 @@ static int local_set_xattr(const char *path, FsCred *credp)
     return 0;
 }
 
+static int local_set_xattrat(int dirfd, const char *path, FsCred *credp)
+{
+    int err;
+
+    if (credp->fc_uid != -1) {
+        uint32_t tmp_uid = cpu_to_le32(credp->fc_uid);
+        err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.uid", &tmp_uid,
+                                   sizeof(uid_t), 0);
+        if (err) {
+            return err;
+        }
+    }
+    if (credp->fc_gid != -1) {
+        uint32_t tmp_gid = cpu_to_le32(credp->fc_gid);
+        err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.gid", &tmp_gid,
+                                   sizeof(gid_t), 0);
+        if (err) {
+            return err;
+        }
+    }
+    if (credp->fc_mode != -1) {
+        uint32_t tmp_mode = cpu_to_le32(credp->fc_mode);
+        err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.mode", &tmp_mode,
+                                   sizeof(mode_t), 0);
+        if (err) {
+            return err;
+        }
+    }
+    if (credp->fc_rdev != -1) {
+        uint64_t tmp_rdev = cpu_to_le64(credp->fc_rdev);
+        err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.rdev", &tmp_rdev,
+                                   sizeof(dev_t), 0);
+        if (err) {
+            return err;
+        }
+    }
+    return 0;
+}
+
 static int local_post_create_passthrough(FsContext *fs_ctx, const char *path,
                                          FsCred *credp)
 {
@@ -553,21 +673,38 @@ static ssize_t local_pwritev(FsContext *ctx, V9fsFidOpenState *fs,
 
 static int local_chmod(FsContext *fs_ctx, V9fsPath *fs_path, FsCred *credp)
 {
-    char *buffer;
     int ret = -1;
-    char *path = fs_path->data;
+    int dirfd;
 
-    if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
-        buffer = rpath(fs_ctx, path);
-        ret = local_set_xattr(buffer, credp);
-        g_free(buffer);
-    } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-        return local_set_mapped_file_attr(fs_ctx, path, credp);
-    } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) ||
-               (fs_ctx->export_flags & V9FS_SM_NONE)) {
-        buffer = rpath(fs_ctx, path);
-        ret = chmod(buffer, credp->fc_mode);
-        g_free(buffer);
+    if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
+        fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
+        char *dirpath, *name;
+
+        dirpath = g_path_get_dirname(fs_path->data);
+        dirfd = local_opendir_nofollow(fs_ctx, dirpath);
+        g_free(dirpath);
+        if (dirfd == -1) {
+            return -1;
+        }
+
+        name = g_path_get_basename(fs_path->data);
+        if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
+            ret = local_set_xattrat(dirfd, name, credp);
+        } else {
+            ret = local_set_mapped_file_attrat(dirfd, name, credp);
+        }
+        g_free(name);
+        close_preserve_errno(dirfd);
+    } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
+               fs_ctx->export_flags & V9FS_SM_NONE) {
+        int fd;
+
+        fd = local_open_nofollow(fs_ctx, fs_path->data, O_RDONLY, 0);
+        if (fd == -1) {
+            return -1;
+        }
+        ret = fchmod(fd, credp->fc_mode);
+        close_preserve_errno(fd);
     }
     return ret;
 }

[Qemu-devel] [PATCH 24/29] 9pfs: local: chown: don't follow symlinks

[Greg Kurz]

The local_chown() callback is vulnerable to symlink attacks because it
calls:

(1) lchown() which follows symbolic links for all path elements but the
    rightmost one
(2) local_set_xattr()->setxattr() which follows symbolic links for all
    path elements
(3) local_set_mapped_file_attr() which calls in turn local_fopen() and
    mkdir(), both functions following symbolic links for all path
    elements but the rightmost one

This patch converts local_chown() to rely on open_nofollow() and
fchownat() to fix (1), as well as local_set_xattrat() and
local_set_mapped_file_attrat() to fix (2) and (3) respectively.

This partly fixes CVE-2016-9602.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 56d7731f7ce1..7f1e105d72aa 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1138,23 +1138,31 @@ static int local_truncate(FsContext *ctx, V9fsPath *fs_path, off_t size)
 
 static int local_chown(FsContext *fs_ctx, V9fsPath *fs_path, FsCred *credp)
 {
-    char *buffer;
+    char *dirpath = g_path_get_dirname(fs_path->data);
+    char *name = g_path_get_basename(fs_path->data);
     int ret = -1;
-    char *path = fs_path->data;
+    int dirfd;
+
+    dirfd = local_opendir_nofollow(fs_ctx, dirpath);
+    if (dirfd == -1) {
+        goto out;
+    }
 
     if ((credp->fc_uid == -1 && credp->fc_gid == -1) ||
         (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) ||
         (fs_ctx->export_flags & V9FS_SM_NONE)) {
-        buffer = rpath(fs_ctx, path);
-        ret = lchown(buffer, credp->fc_uid, credp->fc_gid);
-        g_free(buffer);
+        ret = fchownat(dirfd, name, credp->fc_uid, credp->fc_gid,
+                       AT_SYMLINK_NOFOLLOW);
     } else if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
-        buffer = rpath(fs_ctx, path);
-        ret = local_set_xattr(buffer, credp);
-        g_free(buffer);
+        ret = local_set_xattrat(dirfd, name, credp);
     } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-        return local_set_mapped_file_attr(fs_ctx, path, credp);
+        ret = local_set_mapped_file_attrat(dirfd, name, credp);
     }
+
+    close_preserve_errno(dirfd);
+out:
+    g_free(name);
+    g_free(dirpath);
     return ret;
 }
 

[Qemu-devel] [PATCH 25/29] 9pfs: local: symlink: don't follow symlinks

[Greg Kurz]

The local_symlink() callback is vulnerable to symlink attacks because it
calls:

(1) symlink() which follows symbolic links for all path elements but the
    rightmost one
(2) open(O_NOFOLLOW) which follows symbolic links for all path elements but
    the rightmost one
(3) local_set_xattr()->setxattr() which follows symbolic links for all
    path elements
(4) local_set_mapped_file_attr() which calls in turn local_fopen() and
    mkdir(), both functions following symbolic links for all path
    elements but the rightmost one

This patch converts local_symlink() to rely on opendir_nofollow() and
symlinkat() to fix (1), openat(O_NOFOLLOW) to fix (2), as well as
local_set_xattrat() and local_set_mapped_file_attrat() to fix (3) and
(4) respectively.

This partly fixes CVE-2016-9602.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   81 ++++++++++++++++------------------------------------
 1 file changed, 25 insertions(+), 56 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 7f1e105d72aa..9c4faae0b3a2 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -954,23 +954,22 @@ static int local_symlink(FsContext *fs_ctx, const char *oldpath,
                          V9fsPath *dir_path, const char *name, FsCred *credp)
 {
     int err = -1;
-    int serrno = 0;
-    char *newpath;
-    V9fsString fullname;
-    char *buffer = NULL;
+    int dirfd;
 
-    v9fs_string_init(&fullname);
-    v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
-    newpath = fullname.data;
+    dirfd = local_opendir_nofollow(fs_ctx, dir_path->data);
+    if (dirfd == -1) {
+        return -1;
+    }
 
     /* Determine the security model */
-    if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
+    if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
+        fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
         int fd;
         ssize_t oldpath_size, write_size;
-        buffer = rpath(fs_ctx, newpath);
-        fd = open(buffer, O_CREAT|O_EXCL|O_RDWR|O_NOFOLLOW, SM_LOCAL_MODE_BITS);
+
+        fd = openat(dirfd, name, O_CREAT | O_EXCL | O_RDWR | O_NOFOLLOW,
+                    SM_LOCAL_MODE_BITS);
         if (fd == -1) {
-            err = fd;
             goto out;
         }
         /* Write the oldpath (target) to the file. */
@@ -978,78 +977,48 @@ static int local_symlink(FsContext *fs_ctx, const char *oldpath,
         do {
             write_size = write(fd, (void *)oldpath, oldpath_size);
         } while (write_size == -1 && errno == EINTR);
+        close_preserve_errno(fd);
 
         if (write_size != oldpath_size) {
-            serrno = errno;
-            close(fd);
-            err = -1;
             goto err_end;
         }
-        close(fd);
         /* Set cleint credentials in symlink's xattr */
-        credp->fc_mode = credp->fc_mode|S_IFLNK;
-        err = local_set_xattr(buffer, credp);
-        if (err == -1) {
-            serrno = errno;
-            goto err_end;
-        }
-    } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-        int fd;
-        ssize_t oldpath_size, write_size;
-        buffer = rpath(fs_ctx, newpath);
-        fd = open(buffer, O_CREAT|O_EXCL|O_RDWR|O_NOFOLLOW, SM_LOCAL_MODE_BITS);
-        if (fd == -1) {
-            err = fd;
-            goto out;
-        }
-        /* Write the oldpath (target) to the file. */
-        oldpath_size = strlen(oldpath);
-        do {
-            write_size = write(fd, (void *)oldpath, oldpath_size);
-        } while (write_size == -1 && errno == EINTR);
+        credp->fc_mode = credp->fc_mode | S_IFLNK;
 
-        if (write_size != oldpath_size) {
-            serrno = errno;
-            close(fd);
-            err = -1;
-            goto err_end;
+        if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
+            err = local_set_xattrat(dirfd, name, credp);
+        } else {
+            err = local_set_mapped_file_attrat(dirfd, name, credp);
         }
-        close(fd);
-        /* Set cleint credentials in symlink's xattr */
-        credp->fc_mode = credp->fc_mode|S_IFLNK;
-        err = local_set_mapped_file_attr(fs_ctx, newpath, credp);
         if (err == -1) {
-            serrno = errno;
             goto err_end;
         }
-    } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) ||
-               (fs_ctx->export_flags & V9FS_SM_NONE)) {
-        buffer = rpath(fs_ctx, newpath);
-        err = symlink(oldpath, buffer);
+    } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
+               fs_ctx->export_flags & V9FS_SM_NONE) {
+        err = symlinkat(oldpath, dirfd, name);
         if (err) {
             goto out;
         }
-        err = lchown(buffer, credp->fc_uid, credp->fc_gid);
+        err = fchownat(dirfd, name, credp->fc_uid, credp->fc_gid,
+                       AT_SYMLINK_NOFOLLOW);
         if (err == -1) {
             /*
              * If we fail to change ownership and if we are
              * using security model none. Ignore the error
              */
             if ((fs_ctx->export_flags & V9FS_SEC_MASK) != V9FS_SM_NONE) {
-                serrno = errno;
                 goto err_end;
-            } else
+            } else {
                 err = 0;
+            }
         }
     }
     goto out;
 
 err_end:
-    remove(buffer);
-    errno = serrno;
+    unlinkat_preserve_errno(dirfd, name, 0);
 out:
-    g_free(buffer);
-    v9fs_string_free(&fullname);
+    close_preserve_errno(dirfd);
     return err;
 }
 

[Qemu-devel] [PATCH 26/29] 9pfs: local: mknod: don't follow symlinks

[Greg Kurz]

The local_mknod() callback is vulnerable to symlink attacks because it
calls:

(1) mknod() which follows symbolic links for all path elements but the
    rightmost one
(2) local_set_xattr()->setxattr() which follows symbolic links for all
    path elements
(3) local_set_mapped_file_attr() which calls in turn local_fopen() and
    mkdir(), both functions following symbolic links for all path
    elements but the rightmost one
(4) local_post_create_passthrough() which calls in turn lchown() and
    chmod(), both functions also following symbolic links

This patch converts local_mknod() to rely on opendir_nofollow() and
mknodat() to fix (1), as well as local_set_xattrat() and
local_set_mapped_file_attrat() to fix (2) and (3) respectively.

A new local_set_cred_passthrough() helper based on fchownat() and fchmod()
is introduced as a replacement to local_post_create_passthrough() to fix (4).
No effort is made to factor out code because local_post_create_passthrough()
will be dropped when all users have been converted to call the new helper.

The mapped and mapped-file security modes are supposed to be identical,
except for the place where credentials and file modes are stored. While
here, we also make that explicit by sharing the call to mknodat().

This partly fixes CVE-2016-9602.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   82 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 49 insertions(+), 33 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 9c4faae0b3a2..89357b9486e3 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -509,6 +509,37 @@ err:
     return -1;
 }
 
+static int local_set_cred_passthrough(FsContext *fs_ctx, int dirfd,
+                                      const char *name, FsCred *credp)
+{
+    int fd, err = -1;
+
+    if (fchownat(dirfd, name, credp->fc_uid, credp->fc_gid,
+                 AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH) < 0) {
+        /*
+         * If we fail to change ownership and if we are
+         * using security model none. Ignore the error
+         */
+        if ((fs_ctx->export_flags & V9FS_SEC_MASK) != V9FS_SM_NONE) {
+            return -1;
+        }
+    }
+
+    fd = openat_nofollow(dirfd, name, O_RDONLY, 0);
+    if (fd == -1) {
+        return -1;
+    }
+
+    if (fchmod(fd, credp->fc_mode & 07777) < 0) {
+        goto out;
+    }
+    err = 0;
+
+out:
+    close_preserve_errno(fd);
+    return err;
+}
+
 static ssize_t local_readlink(FsContext *fs_ctx, V9fsPath *fs_path,
                               char *buf, size_t bufsz)
 {
@@ -712,61 +743,46 @@ static int local_chmod(FsContext *fs_ctx, V9fsPath *fs_path, FsCred *credp)
 static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path,
                        const char *name, FsCred *credp)
 {
-    char *path;
     int err = -1;
-    int serrno = 0;
-    V9fsString fullname;
-    char *buffer = NULL;
+    int dirfd;
 
-    v9fs_string_init(&fullname);
-    v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
-    path = fullname.data;
+    dirfd = local_opendir_nofollow(fs_ctx, dir_path->data);
+    if (dirfd == -1) {
+        return -1;
+    }
 
-    /* Determine the security model */
-    if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
-        buffer = rpath(fs_ctx, path);
-        err = mknod(buffer, SM_LOCAL_MODE_BITS|S_IFREG, 0);
+    if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
+        fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
+        err = mknodat(dirfd, name, SM_LOCAL_MODE_BITS | S_IFREG, 0);
         if (err == -1) {
             goto out;
         }
-        err = local_set_xattr(buffer, credp);
-        if (err == -1) {
-            serrno = errno;
-            goto err_end;
-        }
-    } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
 
-        buffer = rpath(fs_ctx, path);
-        err = mknod(buffer, SM_LOCAL_MODE_BITS|S_IFREG, 0);
-        if (err == -1) {
-            goto out;
+        if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
+            err = local_set_xattrat(dirfd, name, credp);
+        } else {
+            err = local_set_mapped_file_attrat(dirfd, name, credp);
         }
-        err = local_set_mapped_file_attr(fs_ctx, path, credp);
         if (err == -1) {
-            serrno = errno;
             goto err_end;
         }
-    } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) ||
-               (fs_ctx->export_flags & V9FS_SM_NONE)) {
-        buffer = rpath(fs_ctx, path);
-        err = mknod(buffer, credp->fc_mode, credp->fc_rdev);
+    } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
+               fs_ctx->export_flags & V9FS_SM_NONE) {
+        err = mknodat(dirfd, name, credp->fc_mode, credp->fc_rdev);
         if (err == -1) {
             goto out;
         }
-        err = local_post_create_passthrough(fs_ctx, path, credp);
+        err = local_set_cred_passthrough(fs_ctx, dirfd, name, credp);
         if (err == -1) {
-            serrno = errno;
             goto err_end;
         }
     }
     goto out;
 
 err_end:
-    remove(buffer);
-    errno = serrno;
+    unlinkat_preserve_errno(dirfd, name, 0);
 out:
-    g_free(buffer);
-    v9fs_string_free(&fullname);
+    close_preserve_errno(dirfd);
     return err;
 }
 

[Qemu-devel] [PATCH 27/29] 9pfs: local: mkdir: don't follow symlinks

[Greg Kurz]

The local_mkdir() callback is vulnerable to symlink attacks because it
calls:

(1) mkdir() which follows symbolic links for all path elements but the
    rightmost one
(2) local_set_xattr()->setxattr() which follows symbolic links for all
    path elements
(3) local_set_mapped_file_attr() which calls in turn local_fopen() and
    mkdir(), both functions following symbolic links for all path
    elements but the rightmost one
(4) local_post_create_passthrough() which calls in turn lchown() and
    chmod(), both functions also following symbolic links

This patch converts local_mkdir() to rely on opendir_nofollow() and
mkdirat() to fix (1), as well as local_set_xattrat(),
local_set_mapped_file_attrat() and local_set_cred_passthrough() to
fix (2), (3) and (4) respectively.

The mapped and mapped-file security modes are supposed to be identical,
except for the place where credentials and file modes are stored. While
here, we also make that explicit by sharing the call to mkdirat().

This partly fixes CVE-2016-9602.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   55 +++++++++++++++++++---------------------------------
 1 file changed, 20 insertions(+), 35 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 89357b9486e3..85c0ea813baf 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -789,62 +789,47 @@ out:
 static int local_mkdir(FsContext *fs_ctx, V9fsPath *dir_path,
                        const char *name, FsCred *credp)
 {
-    char *path;
     int err = -1;
-    int serrno = 0;
-    V9fsString fullname;
-    char *buffer = NULL;
+    int dirfd;
 
-    v9fs_string_init(&fullname);
-    v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
-    path = fullname.data;
+    dirfd = local_opendir_nofollow(fs_ctx, dir_path->data);
+    if (dirfd == -1) {
+        return -1;
+    }
 
-    /* Determine the security model */
-    if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
-        buffer = rpath(fs_ctx, path);
-        err = mkdir(buffer, SM_LOCAL_DIR_MODE_BITS);
+    if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
+        fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
+        err = mkdirat(dirfd, name, SM_LOCAL_DIR_MODE_BITS);
         if (err == -1) {
             goto out;
         }
-        credp->fc_mode = credp->fc_mode|S_IFDIR;
-        err = local_set_xattr(buffer, credp);
-        if (err == -1) {
-            serrno = errno;
-            goto err_end;
-        }
-    } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-        buffer = rpath(fs_ctx, path);
-        err = mkdir(buffer, SM_LOCAL_DIR_MODE_BITS);
-        if (err == -1) {
-            goto out;
+        credp->fc_mode = credp->fc_mode | S_IFDIR;
+
+        if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
+            err = local_set_xattrat(dirfd, name, credp);
+        } else {
+            err = local_set_mapped_file_attrat(dirfd, name, credp);
         }
-        credp->fc_mode = credp->fc_mode|S_IFDIR;
-        err = local_set_mapped_file_attr(fs_ctx, path, credp);
         if (err == -1) {
-            serrno = errno;
             goto err_end;
         }
-    } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) ||
-               (fs_ctx->export_flags & V9FS_SM_NONE)) {
-        buffer = rpath(fs_ctx, path);
-        err = mkdir(buffer, credp->fc_mode);
+    } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
+               fs_ctx->export_flags & V9FS_SM_NONE) {
+        err = mkdirat(dirfd, name, credp->fc_mode);
         if (err == -1) {
             goto out;
         }
-        err = local_post_create_passthrough(fs_ctx, path, credp);
+        err = local_set_cred_passthrough(fs_ctx, dirfd, name, credp);
         if (err == -1) {
-            serrno = errno;
             goto err_end;
         }
     }
     goto out;
 
 err_end:
-    remove(buffer);
-    errno = serrno;
+    unlinkat_preserve_errno(dirfd, name, AT_REMOVEDIR);
 out:
-    g_free(buffer);
-    v9fs_string_free(&fullname);
+    close_preserve_errno(dirfd);
     return err;
 }
 

[Qemu-devel] [PATCH 28/29] 9pfs: local: open2: don't follow symlinks

[Greg Kurz]

The local_open2() callback is vulnerable to symlink attacks because it
calls:

(1) open() which follows symbolic links for all path elements but the
    rightmost one
(2) local_set_xattr()->setxattr() which follows symbolic links for all
    path elements
(3) local_set_mapped_file_attr() which calls in turn local_fopen() and
    mkdir(), both functions following symbolic links for all path
    elements but the rightmost one
(4) local_post_create_passthrough() which calls in turn lchown() and
    chmod(), both functions also following symbolic links

This patch converts local_open2() to rely on opendir_nofollow() and
mkdirat() to fix (1), as well as local_set_xattrat(),
local_set_mapped_file_attrat() and local_set_cred_passthrough() to
fix (2), (3) and (4) respectively. Since local_open2() already opens
a descriptor to the target file, local_set_cred_passthrough() is
modified to reuse it instead of opening a new one.

The mapped and mapped-file security modes are supposed to be identical,
except for the place where credentials and file modes are stored. While
here, we also make that explicit by sharing the call to openat().

This partly fixes CVE-2016-9602.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |   70 ++++++++++++++++++++++------------------------------
 1 file changed, 29 insertions(+), 41 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 85c0ea813baf..730cc37b765f 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -525,9 +525,13 @@ static int local_set_cred_passthrough(FsContext *fs_ctx, int dirfd,
         }
     }
 
-    fd = openat_nofollow(dirfd, name, O_RDONLY, 0);
-    if (fd == -1) {
-        return -1;
+    if (name[0]) {
+        fd = openat_nofollow(dirfd, name, O_RDONLY, 0);
+        if (fd == -1) {
+            return -1;
+        }
+    } else {
+        fd = dirfd;
     }
 
     if (fchmod(fd, credp->fc_mode & 07777) < 0) {
@@ -536,7 +540,9 @@ static int local_set_cred_passthrough(FsContext *fs_ctx, int dirfd,
     err = 0;
 
 out:
-    close_preserve_errno(fd);
+    if (name[0]) {
+        close_preserve_errno(fd);
+    }
     return err;
 }
 
@@ -877,62 +883,45 @@ static int local_fstat(FsContext *fs_ctx, int fid_type,
 static int local_open2(FsContext *fs_ctx, V9fsPath *dir_path, const char *name,
                        int flags, FsCred *credp, V9fsFidOpenState *fs)
 {
-    char *path;
     int fd = -1;
     int err = -1;
-    int serrno = 0;
-    V9fsString fullname;
-    char *buffer = NULL;
+    int dirfd;
 
     /*
      * Mark all the open to not follow symlinks
      */
     flags |= O_NOFOLLOW;
 
-    v9fs_string_init(&fullname);
-    v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name);
-    path = fullname.data;
+    dirfd = local_opendir_nofollow(fs_ctx, dir_path->data);
+    if (dirfd == -1) {
+        return -1;
+    }
 
     /* Determine the security model */
-    if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
-        buffer = rpath(fs_ctx, path);
-        fd = open(buffer, flags, SM_LOCAL_MODE_BITS);
+    if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
+        fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
+        fd = openat(dirfd, name, flags, SM_LOCAL_MODE_BITS);
         if (fd == -1) {
-            err = fd;
             goto out;
         }
         credp->fc_mode = credp->fc_mode|S_IFREG;
-        /* Set cleint credentials in xattr */
-        err = local_set_xattr(buffer, credp);
-        if (err == -1) {
-            serrno = errno;
-            goto err_end;
-        }
-    } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-        buffer = rpath(fs_ctx, path);
-        fd = open(buffer, flags, SM_LOCAL_MODE_BITS);
-        if (fd == -1) {
-            err = fd;
-            goto out;
+        if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
+            /* Set cleint credentials in xattr */
+            err = local_set_xattrat(dirfd, name, credp);
+        } else {
+            err = local_set_mapped_file_attrat(dirfd, name, credp);
         }
-        credp->fc_mode = credp->fc_mode|S_IFREG;
-        /* Set client credentials in .virtfs_metadata directory files */
-        err = local_set_mapped_file_attr(fs_ctx, path, credp);
         if (err == -1) {
-            serrno = errno;
             goto err_end;
         }
     } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) ||
                (fs_ctx->export_flags & V9FS_SM_NONE)) {
-        buffer = rpath(fs_ctx, path);
-        fd = open(buffer, flags, credp->fc_mode);
+        fd = openat(dirfd, name, flags, credp->fc_mode);
         if (fd == -1) {
-            err = fd;
             goto out;
         }
-        err = local_post_create_passthrough(fs_ctx, path, credp);
+        err = local_set_cred_passthrough(fs_ctx, fd, "", credp);
         if (err == -1) {
-            serrno = errno;
             goto err_end;
         }
     }
@@ -941,12 +930,11 @@ static int local_open2(FsContext *fs_ctx, V9fsPath *dir_path, const char *name,
     goto out;
 
 err_end:
-    close(fd);
-    remove(buffer);
-    errno = serrno;
+    unlinkat_preserve_errno(dirfd, name,
+                            flags & O_DIRECTORY ? AT_REMOVEDIR : 0);
+    close_preserve_errno(fd);
 out:
-    g_free(buffer);
-    v9fs_string_free(&fullname);
+    close_preserve_errno(dirfd);
     return err;
 }
 

[Qemu-devel] [PATCH 29/29] 9pfs: local: drop unused code

[Greg Kurz]

Now that the all callbacks have been converted to use "at" syscalls, we
can drop this code.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c |  198 ----------------------------------------------------
 1 file changed, 198 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 730cc37b765f..6d7d1d35bb6b 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -78,48 +78,6 @@ static void unlinkat_preserve_errno(int dirfd, const char *path, int flags)
 
 #define VIRTFS_META_DIR ".virtfs_metadata"
 
-static char *local_mapped_attr_path(FsContext *ctx, const char *path)
-{
-    int dirlen;
-    const char *name = strrchr(path, '/');
-    if (name) {
-        dirlen = name - path;
-        ++name;
-    } else {
-        name = path;
-        dirlen = 0;
-    }
-    return g_strdup_printf("%s/%.*s/%s/%s", ctx->fs_root,
-                           dirlen, path, VIRTFS_META_DIR, name);
-}
-
-static FILE *local_fopen(const char *path, const char *mode)
-{
-    int fd, o_mode = 0;
-    FILE *fp;
-    int flags = O_NOFOLLOW;
-    /*
-     * only supports two modes
-     */
-    if (mode[0] == 'r') {
-        flags |= O_RDONLY;
-    } else if (mode[0] == 'w') {
-        flags |= O_WRONLY | O_TRUNC | O_CREAT;
-        o_mode = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH;
-    } else {
-        return NULL;
-    }
-    fd = open(path, flags, o_mode);
-    if (fd == -1) {
-        return NULL;
-    }
-    fp = fdopen(fd, mode);
-    if (!fp) {
-        close(fd);
-    }
-    return fp;
-}
-
 static FILE *local_fopenat(int dirfd, const char *name, const char *mode)
 {
     int fd, o_mode = 0;
@@ -233,100 +191,6 @@ out:
     return err;
 }
 
-static int local_create_mapped_attr_dir(FsContext *ctx, const char *path)
-{
-    int err;
-    char *attr_dir;
-    char *tmp_path = g_strdup(path);
-
-    attr_dir = g_strdup_printf("%s/%s/%s",
-             ctx->fs_root, dirname(tmp_path), VIRTFS_META_DIR);
-
-    err = mkdir(attr_dir, 0700);
-    if (err < 0 && errno == EEXIST) {
-        err = 0;
-    }
-    g_free(attr_dir);
-    g_free(tmp_path);
-    return err;
-}
-
-static int local_set_mapped_file_attr(FsContext *ctx,
-                                      const char *path, FsCred *credp)
-{
-    FILE *fp;
-    int ret = 0;
-    char buf[ATTR_MAX];
-    char *attr_path;
-    int uid = -1, gid = -1, mode = -1, rdev = -1;
-
-    attr_path = local_mapped_attr_path(ctx, path);
-    fp = local_fopen(attr_path, "r");
-    if (!fp) {
-        goto create_map_file;
-    }
-    memset(buf, 0, ATTR_MAX);
-    while (fgets(buf, ATTR_MAX, fp)) {
-        if (!strncmp(buf, "virtfs.uid", 10)) {
-            uid = atoi(buf+11);
-        } else if (!strncmp(buf, "virtfs.gid", 10)) {
-            gid = atoi(buf+11);
-        } else if (!strncmp(buf, "virtfs.mode", 11)) {
-            mode = atoi(buf+12);
-        } else if (!strncmp(buf, "virtfs.rdev", 11)) {
-            rdev = atoi(buf+12);
-        }
-        memset(buf, 0, ATTR_MAX);
-    }
-    fclose(fp);
-    goto update_map_file;
-
-create_map_file:
-    ret = local_create_mapped_attr_dir(ctx, path);
-    if (ret < 0) {
-        goto err_out;
-    }
-
-update_map_file:
-    fp = local_fopen(attr_path, "w");
-    if (!fp) {
-        ret = -1;
-        goto err_out;
-    }
-
-    if (credp->fc_uid != -1) {
-        uid = credp->fc_uid;
-    }
-    if (credp->fc_gid != -1) {
-        gid = credp->fc_gid;
-    }
-    if (credp->fc_mode != -1) {
-        mode = credp->fc_mode;
-    }
-    if (credp->fc_rdev != -1) {
-        rdev = credp->fc_rdev;
-    }
-
-
-    if (uid != -1) {
-        fprintf(fp, "virtfs.uid=%d\n", uid);
-    }
-    if (gid != -1) {
-        fprintf(fp, "virtfs.gid=%d\n", gid);
-    }
-    if (mode != -1) {
-        fprintf(fp, "virtfs.mode=%d\n", mode);
-    }
-    if (rdev != -1) {
-        fprintf(fp, "virtfs.rdev=%d\n", rdev);
-    }
-    fclose(fp);
-
-err_out:
-    g_free(attr_path);
-    return ret;
-}
-
 static int local_set_mapped_file_attrat(int dirfd, const char *name,
                                         FsCred *credp)
 {
@@ -408,41 +272,6 @@ update_map_file:
     return 0;
 }
 
-static int local_set_xattr(const char *path, FsCred *credp)
-{
-    int err;
-
-    if (credp->fc_uid != -1) {
-        uint32_t tmp_uid = cpu_to_le32(credp->fc_uid);
-        err = setxattr(path, "user.virtfs.uid", &tmp_uid, sizeof(uid_t), 0);
-        if (err) {
-            return err;
-        }
-    }
-    if (credp->fc_gid != -1) {
-        uint32_t tmp_gid = cpu_to_le32(credp->fc_gid);
-        err = setxattr(path, "user.virtfs.gid", &tmp_gid, sizeof(gid_t), 0);
-        if (err) {
-            return err;
-        }
-    }
-    if (credp->fc_mode != -1) {
-        uint32_t tmp_mode = cpu_to_le32(credp->fc_mode);
-        err = setxattr(path, "user.virtfs.mode", &tmp_mode, sizeof(mode_t), 0);
-        if (err) {
-            return err;
-        }
-    }
-    if (credp->fc_rdev != -1) {
-        uint64_t tmp_rdev = cpu_to_le64(credp->fc_rdev);
-        err = setxattr(path, "user.virtfs.rdev", &tmp_rdev, sizeof(dev_t), 0);
-        if (err) {
-            return err;
-        }
-    }
-    return 0;
-}
-
 static int local_set_xattrat(int dirfd, const char *path, FsCred *credp)
 {
     int err;
@@ -482,33 +311,6 @@ static int local_set_xattrat(int dirfd, const char *path, FsCred *credp)
     return 0;
 }
 
-static int local_post_create_passthrough(FsContext *fs_ctx, const char *path,
-                                         FsCred *credp)
-{
-    char *buffer;
-
-    buffer = rpath(fs_ctx, path);
-    if (lchown(buffer, credp->fc_uid, credp->fc_gid) < 0) {
-        /*
-         * If we fail to change ownership and if we are
-         * using security model none. Ignore the error
-         */
-        if ((fs_ctx->export_flags & V9FS_SEC_MASK) != V9FS_SM_NONE) {
-            goto err;
-        }
-    }
-
-    if (chmod(buffer, credp->fc_mode & 07777) < 0) {
-        goto err;
-    }
-
-    g_free(buffer);
-    return 0;
-err:
-    g_free(buffer);
-    return -1;
-}
-
 static int local_set_cred_passthrough(FsContext *fs_ctx, int dirfd,
                                       const char *name, FsCred *credp)
 {

Re: [Qemu-devel] [PATCH 01/29] 9pfs: local: move xattr security ops to 9p-xattr.c

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 494 bytes --]

On Mon, Feb 20, 2017 at 03:39:26PM +0100, Greg Kurz wrote:
> These functions are always called indirectly. It really doesn't make sense
> for them to sit in a header file.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-xattr.c |   61 ++++++++++++++++++++++++++++++++++++++++
>  hw/9pfs/9p-xattr.h |   80 +++++++++-------------------------------------------
>  2 files changed, 75 insertions(+), 66 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 02/29] 9pfs: remove side-effects in local_init()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 831 bytes --]

On Mon, Feb 20, 2017 at 03:39:34PM +0100, Greg Kurz wrote:
> If this function fails, it should not modify *ctx.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-local.c |   37 +++++++++++++++++++------------------
>  1 file changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 7de07e1ba67f..55903e5d7745 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -1168,9 +1168,25 @@ static int local_ioc_getversion(FsContext *ctx, V9fsPath *path,
>  
>  static int local_init(FsContext *ctx)
>  {
> -    int err = 0;
>      struct statfs stbuf;
>  
> +#ifdef FS_IOC_GETVERSION
> +    /*
> +     * use ioc_getversion only if the iocl is definied

s/iocl/ioctl/

Otherwise:
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 03/29] 9pfs: remove side-effects in local_open() and local_opendir()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 365 bytes --]

On Mon, Feb 20, 2017 at 03:39:42PM +0100, Greg Kurz wrote:
> If these functions fail, they should not change *fs. Let's use local
> variables to fix this.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-local.c |   13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 5632 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 05/29] 9pfs: local: keep a file descriptor on the shared folder

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1001 bytes --]

On Mon, Feb 20, 2017 at 03:39:59PM +0100, Greg Kurz wrote:
> This patch opens the shared folder and caches the file descriptor, so that
> it can be used to do symlink-safe path walk.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-local.c |   30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index c2239bfafce4..8aee7f2516ed 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -14,6 +14,7 @@
>  #include "qemu/osdep.h"
>  #include "9p.h"
>  #include "9p-xattr.h"
> +#include "9p-util.h"
>  #include "fsdev/qemu-fsdev.h"   /* local_ops */
>  #include <arpa/inet.h>
>  #include <pwd.h>
> @@ -43,6 +44,10 @@
>  #define BTRFS_SUPER_MAGIC 0x9123683E
>  #endif
>  
> +struct local_data {

QEMU coding style would requires:

  typedef struct {
      int mountfd;
  } LocalData;

Otherwise:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 6433 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH 06/29] 9pfs: local: open/opendir: don't follow symlinks

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 872 bytes --]

On Mon, Feb 20, 2017 at 03:40:07PM +0100, Greg Kurz wrote:
> The local_open() and local_opendir() callbacks are vulnerable to symlink
> attacks because they call:
> 
> (1) open(O_NOFOLLOW) which follows symbolic links in all path elements but
>     the rightmost one
> (2) opendir() which follows symbolic links in all path elements
> 
> This patch converts both callbacks to use new helpers based on
> openat_nofollow() to only open files and directories if they are
> below the virtfs shared folder
> 
> This partly fixes CVE-2016-9602.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-local.c |   31 +++++++++++++++++++++----------
>  hw/9pfs/9p-local.h |   20 ++++++++++++++++++++
>  2 files changed, 41 insertions(+), 10 deletions(-)
>  create mode 100644 hw/9pfs/9p-local.h

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 07/29] 9pfs: local: introduce symlink-attack safe xattr helpers

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1966 bytes --]

On Mon, Feb 20, 2017 at 03:40:15PM +0100, Greg Kurz wrote:
> +static ssize_t do_xattrat_op(int op_type, int dirfd, const char *path,
> +                             const char *name, void *value, size_t size,
> +                             int flags)
> +{
> +    struct xattrat_data *data;
> +    pid_t pid;
> +    ssize_t ret = -1;
> +    int wstatus;
> +
> +    data = mmap(NULL, sizeof(*data) + size, PROT_READ | PROT_WRITE,
> +                MAP_SHARED | MAP_ANONYMOUS, -1, 0);
> +    if (data == MAP_FAILED) {
> +        return -1;
> +    }
> +    data->ret = -1;
> +
> +    pid = fork();
> +    if (pid < 0) {
> +        goto err_out;
> +    } else if (pid == 0) {
> +        if (fchdir(dirfd) == 0) {
> +            switch (op_type) {
> +            case XATTRAT_OP_GET:
> +                data->ret = lgetxattr(path, name, data->value, size);
> +                break;
> +            case XATTRAT_OP_LIST:
> +                data->ret = llistxattr(path, data->value, size);
> +                break;
> +            case XATTRAT_OP_SET:
> +                data->ret = lsetxattr(path, name, value, size, flags);
> +                break;
> +            case XATTRAT_OP_REMOVE:
> +                data->ret = lremovexattr(path, name);
> +                break;
> +            default:
> +                g_assert_not_reached();
> +            }
> +        }
> +        data->serrno = errno;
> +        _exit(0);
> +    }
> +    assert(waitpid(pid, &wstatus, 0) == pid && WIFEXITED(wstatus));
> +
> +    ret = data->ret;
> +    if (ret < 0) {
> +        errno = data->serrno;
> +        goto err_out;
> +    }
> +    if (value) {
> +        memcpy(value, data->value, data->ret);
> +    }
> +err_out:
> +    munmap_preserver_errno(data, sizeof(*data) + size);
> +    return ret;
> +}

Forking is ugly since QEMU is a multi-threaded program.  We brainstormed
alternatives on IRC like using /proc/self/fd/$fd to work around the
missing getxattrat() API.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 08/29] 9pfs: local: lgetxattr: don't follow symlinks

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 693 bytes --]

On Mon, Feb 20, 2017 at 03:40:23PM +0100, Greg Kurz wrote:
> The local_lgetxattr() callback is vulnerable to symlink attacks because
> it calls lgetxattr() which follows symbolic links in all path elements but
> the rightmost one.
> 
> This patch converts local_lgetxattr() to rely on opendir_nofollow() and
> fgetxattrat_nofollow() instead.
> 
> This partly fixes CVE-2016-9602.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-posix-acl.c  |   16 ++--------------
>  hw/9pfs/9p-xattr-user.c |    8 +-------
>  hw/9pfs/9p-xattr.c      |    8 +-------
>  3 files changed, 4 insertions(+), 28 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 09/29] 9pfs: local: llistxattr: don't follow symlinks

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 617 bytes --]

On Mon, Feb 20, 2017 at 03:40:30PM +0100, Greg Kurz wrote:
> The local_llistxattr() callback is vulnerable to symlink attacks because
> it calls llistxattr() which follows symbolic links in all path elements but
> the rightmost one.
> 
> This patch converts local_llistxattr() to rely on opendir_nofollow() and
> flistxattrat_nofollow() instead.
> 
> This partly fixes CVE-2016-9602.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-xattr.c |   30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 10/29] 9pfs: local: lsetxattr: don't follow symlinks

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 695 bytes --]

On Mon, Feb 20, 2017 at 03:40:38PM +0100, Greg Kurz wrote:
> The local_lsetxattr() callback is vulnerable to symlink attacks because
> it calls lsetxattr() which follows symbolic links in all path elements but
> the rightmost one.
> 
> This patch converts local_lsetxattr() to rely on opendir_nofollow() and
> fsetxattrat_nofollow() instead.
> 
> This partly fixes CVE-2016-9602.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-posix-acl.c  |   18 ++++--------------
>  hw/9pfs/9p-xattr-user.c |    8 +-------
>  hw/9pfs/9p-xattr.c      |    8 +-------
>  3 files changed, 6 insertions(+), 28 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 11/29] 9pfs: local: lremovexattr: don't follow symlinks

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 699 bytes --]

On Mon, Feb 20, 2017 at 03:40:45PM +0100, Greg Kurz wrote:
> The local_lremovexattr() callback is vulnerable to symlink attacks because
> it calls lremovexattr() which follows symbolic links in all path elements but
> the rightmost one.
> 
> This patch converts local_lremovexattr() to rely on opendir_nofollow() and
> fremovexattrat_nofollow() instead.
> 
> This partly fixes CVE-2016-9602.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-posix-acl.c  |   10 ++--------
>  hw/9pfs/9p-xattr-user.c |    8 +-------
>  hw/9pfs/9p-xattr.c      |    8 +-------
>  3 files changed, 4 insertions(+), 22 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 12/29] 9pfs: local: unlinkat: don't follow symlinks

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 787 bytes --]

On Mon, Feb 20, 2017 at 03:40:53PM +0100, Greg Kurz wrote:
> The local_unlinkat() callback is vulnerable to symlink attacks because it
> calls remove() which follows symbolic links in all path elements but the
> rightmost one.
> 
> This patch converts local_unlinkat() to rely on opendir_nofollow() and
> unlinkat() instead.
> 
> Most of the code is moved to a separate local_unlinkat_common() helper
> which will be reused in a subsequent patch to fix the same issue in
> local_remove().
> 
> This partly fixes CVE-2016-9602.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-local.c |  100 ++++++++++++++++++++++++++++++----------------------
>  1 file changed, 57 insertions(+), 43 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 13/29] 9pfs: local: remove: don't follow symlinks

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 766 bytes --]

On Mon, Feb 20, 2017 at 03:41:00PM +0100, Greg Kurz wrote:
> +    dirfd = local_opendir_nofollow(ctx, dirpath);
> +    if (dirfd) {
> +        goto out;
>      }
>  
> -    buffer = rpath(ctx, path);
> -    err = remove(buffer);
> -    g_free(buffer);
> +    if (fstatat(dirfd, path, &stbuf, AT_SYMLINK_NOFOLLOW) < 0) {
> +        goto err_out;
> +    }
> +
> +    if (S_ISDIR(stbuf.st_mode)) {
> +        flags |= AT_REMOVEDIR;
> +    }
> +
> +    err = local_unlinkat_common(ctx, dirfd, name, flags);

The alternative is optimistically skip fstat but then do:

  if (err == EISDIR) {
      err = local_unlinkat_common(ctx, dirfd, name, flags | AT_REMOVEDIR);
  }

It might be faster.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 14/29] 9pfs: local: utimensat: don't follow symlinks

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 910 bytes --]

On Mon, Feb 20, 2017 at 03:41:08PM +0100, Greg Kurz wrote:
> The local_utimensat() callback is vulnerable to symlink attacks because it
> calls qemu_utimens()->utimensat(AT_SYMLINK_NOFOLLOW) which follows symbolic
> links in all path elements but the rightmost one or qemu_utimens()->utimes()
> which follows symbolic links for all path elements.
> 
> This patch converts local_utimensat() to rely on opendir_nofollow() and
> utimensat(AT_SYMLINK_NOFOLLOW) directly instead of using qemu_utimens().
> It is hence assumed that the OS supports utimensat(), i.e. has glibc 2.6
> or higher and linux 2.6.22 or higher, which seems reasonable nowadays.
> 
> This partly fixes CVE-2016-9602.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-local.c |   19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 15/29] 9pfs: local: statfs: don't follow symlinks

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 542 bytes --]

On Mon, Feb 20, 2017 at 03:41:16PM +0100, Greg Kurz wrote:
> The local_statfs() callback is vulnerable to symlink attacks because it
> calls statfs() which follows symbolic links in all path elements.
> 
> This patch converts local_statfs() to rely on open_nofollow() and fstatfs()
> instead.
> 
> This partly fixes CVE-2016-9602.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-local.c |   10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 16/29] 9pfs: local: truncate: don't follow symlinks

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 553 bytes --]

On Mon, Feb 20, 2017 at 03:41:24PM +0100, Greg Kurz wrote:
> The local_truncate() callback is vulnerable to symlink attacks because
> it calls truncate() which follows symbolic links in all path elements.
> 
> This patch converts local_truncate() to rely on open_nofollow() and
> ftruncate() instead.
> 
> This partly fixes CVE-2016-9602.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-local.c |   13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 17/29] 9pfs: local: readlink: don't follow symlinks

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 747 bytes --]

On Mon, Feb 20, 2017 at 03:41:32PM +0100, Greg Kurz wrote:
> The local_readlink() callback is vulnerable to symlink attacks because it
> calls:
> 
> (1) open(O_NOFOLLOW) which follows symbolic links for all path elements but
>     the rightmost one
> (2) readlink() which follows symbolic links for all path elements but the
>     rightmost one
> 
> This patch converts local_readlink() to rely on open_nofollow() to fix (1)
> and opendir_nofollow(), readlinkat() to fix (2).
> 
> This partly fixes CVE-2016-9602.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-local.c |   26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 18/29] 9pfs: local: lstat: don't follow symlinks

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1147 bytes --]

On Mon, Feb 20, 2017 at 03:41:41PM +0100, Greg Kurz wrote:
> The local_lstat() callback is vulnerable to symlink attacks because it
> calls:
> 
> (1) lstat() which follows symbolic links in all path elements but the
>     rightmost one
> (2) getxattr() which follows symbolic links in all path elements
> (3) local_mapped_file_attr()->local_fopen()->openat(O_NOFOLLOW) which
>     follows symbolic links in all path elements but the rightmost
>     one
> 
> This patch converts local_lstat() to rely on opendir_nofollow() and
> fstatat(AT_SYMLINK_NOFOLLOW) to fix (1), fgetxattrat_nofollow() to
> fix (2).
> 
> A new local_fopenat() helper is introduced as a replacement to
> local_fopen() to fix (3). No effort is made to factor out code
> because local_fopen() will be dropped when all users have been
> converted to call local_fopenat().
> 
> This partly fixes CVE-2016-9602.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-local.c |   78 +++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 61 insertions(+), 17 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 19/29] 9pfs: local: renameat: don't follow symlinks

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 596 bytes --]

On Mon, Feb 20, 2017 at 03:41:49PM +0100, Greg Kurz wrote:
> The local_renameat() callback is currently a wrapper around local_rename()
> which is vulnerable to symlink attacks.
> 
> This patch rewrites local_renameat() to have its own implementation, based
> on local_opendir_nofollow() and renameat().
> 
> This partly fixes CVE-2016-9602.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-local.c |   71 ++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 63 insertions(+), 8 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 20/29] 9pfs: local: rename: use renameat

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 638 bytes --]

On Mon, Feb 20, 2017 at 03:41:56PM +0100, Greg Kurz wrote:
> The local_rename() callback is vulnerable to symlink attacks because it
> uses rename() which follows symbolic links in all path elements but the
> rightmost one.
> 
> This patch simply transforms local_rename() into a wrapper around
> local_renameat() which is symlink-attack safe.
> 
> This partly fixes CVE-2016-9602.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-local.c |   57 +++++++++++++++++++++++++---------------------------
>  1 file changed, 27 insertions(+), 30 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 21/29] 9pfs: local: improve error handling in link op

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 471 bytes --]

On Mon, Feb 20, 2017 at 03:42:04PM +0100, Greg Kurz wrote:
> When using the mapped-file security model, we also have to create a link
> for the metadata file if it exists. In case of failure, we should rollback.
> 
> That's what this patch does.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-local.c |   26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 22/29] 9pfs: local: link: don't follow symlinks

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 772 bytes --]

On Mon, Feb 20, 2017 at 03:42:11PM +0100, Greg Kurz wrote:
> The local_link() callback is vulnerable to symlink attacks because it calls:
> 
> (1) link() which follows symbolic links for all path elements but the
>     rightmost one
> (2) local_create_mapped_attr_dir()->mkdir() which follows symbolic links
>     for all path elements but the rightmost one
> 
> This patch converts local_link() to rely on opendir_nofollow() and linkat()
> to fix (1), mkdirat() to fix (2).
> 
> This partly fixes CVE-2016-9602.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-local.c |   86 ++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 57 insertions(+), 29 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 07/29] 9pfs: local: introduce symlink-attack safe xattr helpers

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1074 bytes --]

On 02/20/2017 08:40 AM, Greg Kurz wrote:
> All operations dealing with extended attributes are vulnerable to symlink
> attacks because they use path-based syscalls which can traverse symbolic
> links while walking through the dirname part of the path.
> 
> The solution is to introduce helpers based on opendir_nofollow(). This
> calls for "at" versions of the extended attribute syscalls, which don't
> exist unfortunately. This patch implement them by simulating the "at"
> behavior with fchdir(). Since the current working directory is process
> wide, and we don't want to confuse another thread in QEMU, all the work
> is done in a separate process.

Can you emulate *at using /proc/fd/nnn/xyz?  Coreutils was one of the
early adopters of the power of *at functions, and found that emulation
of *at via procfs was a LOT more efficient than emulation via fchdir
(although both emulations still exist in gnulib, since procfs is not
universal).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 07/29] 9pfs: local: introduce symlink-attack safe xattr helpers

[Jann Horn]

On Thu, Feb 23, 2017 at 4:02 PM, Eric Blake <eblake@redhat.com> wrote:
> On 02/20/2017 08:40 AM, Greg Kurz wrote:
>> All operations dealing with extended attributes are vulnerable to symlink
>> attacks because they use path-based syscalls which can traverse symbolic
>> links while walking through the dirname part of the path.
>>
>> The solution is to introduce helpers based on opendir_nofollow(). This
>> calls for "at" versions of the extended attribute syscalls, which don't
>> exist unfortunately. This patch implement them by simulating the "at"
>> behavior with fchdir(). Since the current working directory is process
>> wide, and we don't want to confuse another thread in QEMU, all the work
>> is done in a separate process.
>
> Can you emulate *at using /proc/fd/nnn/xyz?

I don't know much about QEMU internals, but QEMU supports running in a
chroot using the -chroot option, right? Does that already require procfs to be
mounted inside the chroot?

Re: [Qemu-devel] [PATCH 23/29] 9pfs: local: chmod: don't follow symlinks

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 7621 bytes --]

On Mon, Feb 20, 2017 at 03:42:19PM +0100, Greg Kurz wrote:
> The local_chmod() callback is vulnerable to symlink attacks because it
> calls:
> 
> (1) chmod() which follows symbolic links for all path elements
> (2) local_set_xattr()->setxattr() which follows symbolic links for all
>     path elements
> (3) local_set_mapped_file_attr() which calls in turn local_fopen() and
>     mkdir(), both functions following symbolic links for all path
>     elements but the rightmost one
> 
> This patch converts local_chmod() to rely on open_nofollow() and
> fchmod() to fix (1).
> 
> It introduces a local_set_xattrat() replacement to local_set_xattr()
> based on fsetxattrat() to fix (2), and a local_set_mapped_file_attrat()
> replacement to local_set_mapped_file_attr() based on local_fopenat()
> and mkdirat() to fix (3). No effort is made to factor out code because
> both local_set_xattr() and local_set_mapped_file_attr() will be dropped
> when all users have been converted to use the "at" versions.
> 
> This partly fixes CVE-2016-9602.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-local.c |  163 ++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 150 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index fb536fdb3082..56d7731f7ce1 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -327,6 +327,87 @@ err_out:
>      return ret;
>  }
>  
> +static int local_set_mapped_file_attrat(int dirfd, const char *name,
> +                                        FsCred *credp)
> +{
> +    FILE *fp;
> +    int ret;
> +    char buf[ATTR_MAX];
> +    int uid = -1, gid = -1, mode = -1, rdev = -1;
> +    int map_dirfd;
> +
> +    ret = mkdirat(dirfd, VIRTFS_META_DIR, 0700);
> +    if (ret < 0 && errno != EEXIST) {
> +        return -1;
> +    }
> +
> +    map_dirfd = openat(dirfd, VIRTFS_META_DIR,
> +                       O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
> +    if (map_dirfd == -1) {
> +        return -1;
> +    }
> +
> +    fp = local_fopenat(map_dirfd, name, "r");
> +    if (!fp) {
> +        if (errno == ENOENT) {
> +            goto update_map_file;
> +        } else {
> +            close_preserve_errno(map_dirfd);
> +            return -1;
> +        }
> +    }
> +    memset(buf, 0, ATTR_MAX);
> +    while (fgets(buf, ATTR_MAX, fp)) {
> +        if (!strncmp(buf, "virtfs.uid", 10)) {
> +            uid = atoi(buf + 11);
> +        } else if (!strncmp(buf, "virtfs.gid", 10)) {
> +            gid = atoi(buf + 11);
> +        } else if (!strncmp(buf, "virtfs.mode", 11)) {
> +            mode = atoi(buf + 12);
> +        } else if (!strncmp(buf, "virtfs.rdev", 11)) {
> +            rdev = atoi(buf + 12);
> +        }
> +        memset(buf, 0, ATTR_MAX);
> +    }
> +    fclose(fp);
> +
> +update_map_file:
> +    fp = local_fopenat(map_dirfd, name, "w");
> +    close_preserve_errno(map_dirfd);
> +    if (!fp) {
> +        return -1;
> +    }
> +
> +    if (credp->fc_uid != -1) {
> +        uid = credp->fc_uid;
> +    }
> +    if (credp->fc_gid != -1) {
> +        gid = credp->fc_gid;
> +    }
> +    if (credp->fc_mode != -1) {
> +        mode = credp->fc_mode;
> +    }
> +    if (credp->fc_rdev != -1) {
> +        rdev = credp->fc_rdev;
> +    }
> +
> +    if (uid != -1) {
> +        fprintf(fp, "virtfs.uid=%d\n", uid);
> +    }
> +    if (gid != -1) {
> +        fprintf(fp, "virtfs.gid=%d\n", gid);
> +    }
> +    if (mode != -1) {
> +        fprintf(fp, "virtfs.mode=%d\n", mode);
> +    }
> +    if (rdev != -1) {
> +        fprintf(fp, "virtfs.rdev=%d\n", rdev);
> +    }
> +    fclose(fp);
> +
> +    return 0;
> +}
> +
>  static int local_set_xattr(const char *path, FsCred *credp)
>  {
>      int err;
> @@ -362,6 +443,45 @@ static int local_set_xattr(const char *path, FsCred *credp)
>      return 0;
>  }
>  
> +static int local_set_xattrat(int dirfd, const char *path, FsCred *credp)
> +{
> +    int err;
> +
> +    if (credp->fc_uid != -1) {
> +        uint32_t tmp_uid = cpu_to_le32(credp->fc_uid);
> +        err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.uid", &tmp_uid,
> +                                   sizeof(uid_t), 0);
> +        if (err) {
> +            return err;
> +        }
> +    }
> +    if (credp->fc_gid != -1) {
> +        uint32_t tmp_gid = cpu_to_le32(credp->fc_gid);
> +        err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.gid", &tmp_gid,
> +                                   sizeof(gid_t), 0);
> +        if (err) {
> +            return err;
> +        }
> +    }
> +    if (credp->fc_mode != -1) {
> +        uint32_t tmp_mode = cpu_to_le32(credp->fc_mode);
> +        err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.mode", &tmp_mode,
> +                                   sizeof(mode_t), 0);
> +        if (err) {
> +            return err;
> +        }
> +    }
> +    if (credp->fc_rdev != -1) {
> +        uint64_t tmp_rdev = cpu_to_le64(credp->fc_rdev);
> +        err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.rdev", &tmp_rdev,
> +                                   sizeof(dev_t), 0);
> +        if (err) {
> +            return err;
> +        }
> +    }
> +    return 0;
> +}
> +
>  static int local_post_create_passthrough(FsContext *fs_ctx, const char *path,
>                                           FsCred *credp)
>  {
> @@ -553,21 +673,38 @@ static ssize_t local_pwritev(FsContext *ctx, V9fsFidOpenState *fs,
>  
>  static int local_chmod(FsContext *fs_ctx, V9fsPath *fs_path, FsCred *credp)
>  {
> -    char *buffer;
>      int ret = -1;
> -    char *path = fs_path->data;
> +    int dirfd;
>  
> -    if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
> -        buffer = rpath(fs_ctx, path);
> -        ret = local_set_xattr(buffer, credp);
> -        g_free(buffer);
> -    } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
> -        return local_set_mapped_file_attr(fs_ctx, path, credp);
> -    } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) ||
> -               (fs_ctx->export_flags & V9FS_SM_NONE)) {
> -        buffer = rpath(fs_ctx, path);
> -        ret = chmod(buffer, credp->fc_mode);
> -        g_free(buffer);
> +    if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
> +        fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
> +        char *dirpath, *name;
> +
> +        dirpath = g_path_get_dirname(fs_path->data);
> +        dirfd = local_opendir_nofollow(fs_ctx, dirpath);
> +        g_free(dirpath);
> +        if (dirfd == -1) {
> +            return -1;
> +        }
> +
> +        name = g_path_get_basename(fs_path->data);
> +        if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
> +            ret = local_set_xattrat(dirfd, name, credp);
> +        } else {
> +            ret = local_set_mapped_file_attrat(dirfd, name, credp);
> +        }
> +        g_free(name);
> +        close_preserve_errno(dirfd);
> +    } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
> +               fs_ctx->export_flags & V9FS_SM_NONE) {
> +        int fd;
> +
> +        fd = local_open_nofollow(fs_ctx, fs_path->data, O_RDONLY, 0);
> +        if (fd == -1) {
> +            return -1;
> +        }
> +        ret = fchmod(fd, credp->fc_mode);
> +        close_preserve_errno(fd);

As mentioned on IRC, not sure this is workable since chmod(2) must not
require read permission on the file.  This patch introduces failures
when the file isn't readable.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 24/29] 9pfs: local: chown: don't follow symlinks

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 980 bytes --]

On Mon, Feb 20, 2017 at 03:42:26PM +0100, Greg Kurz wrote:
> The local_chown() callback is vulnerable to symlink attacks because it
> calls:
> 
> (1) lchown() which follows symbolic links for all path elements but the
>     rightmost one
> (2) local_set_xattr()->setxattr() which follows symbolic links for all
>     path elements
> (3) local_set_mapped_file_attr() which calls in turn local_fopen() and
>     mkdir(), both functions following symbolic links for all path
>     elements but the rightmost one
> 
> This patch converts local_chown() to rely on open_nofollow() and
> fchownat() to fix (1), as well as local_set_xattrat() and
> local_set_mapped_file_attrat() to fix (2) and (3) respectively.
> 
> This partly fixes CVE-2016-9602.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-local.c |   26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 25/29] 9pfs: local: symlink: don't follow symlinks

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1154 bytes --]

On Mon, Feb 20, 2017 at 03:42:34PM +0100, Greg Kurz wrote:
> The local_symlink() callback is vulnerable to symlink attacks because it
> calls:
> 
> (1) symlink() which follows symbolic links for all path elements but the
>     rightmost one
> (2) open(O_NOFOLLOW) which follows symbolic links for all path elements but
>     the rightmost one
> (3) local_set_xattr()->setxattr() which follows symbolic links for all
>     path elements
> (4) local_set_mapped_file_attr() which calls in turn local_fopen() and
>     mkdir(), both functions following symbolic links for all path
>     elements but the rightmost one
> 
> This patch converts local_symlink() to rely on opendir_nofollow() and
> symlinkat() to fix (1), openat(O_NOFOLLOW) to fix (2), as well as
> local_set_xattrat() and local_set_mapped_file_attrat() to fix (3) and
> (4) respectively.
> 
> This partly fixes CVE-2016-9602.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-local.c |   81 ++++++++++++++++------------------------------------
>  1 file changed, 25 insertions(+), 56 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 26/29] 9pfs: local: mknod: don't follow symlinks

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1686 bytes --]

On Mon, Feb 20, 2017 at 03:42:42PM +0100, Greg Kurz wrote:
> The local_mknod() callback is vulnerable to symlink attacks because it
> calls:
> 
> (1) mknod() which follows symbolic links for all path elements but the
>     rightmost one
> (2) local_set_xattr()->setxattr() which follows symbolic links for all
>     path elements
> (3) local_set_mapped_file_attr() which calls in turn local_fopen() and
>     mkdir(), both functions following symbolic links for all path
>     elements but the rightmost one
> (4) local_post_create_passthrough() which calls in turn lchown() and
>     chmod(), both functions also following symbolic links
> 
> This patch converts local_mknod() to rely on opendir_nofollow() and
> mknodat() to fix (1), as well as local_set_xattrat() and
> local_set_mapped_file_attrat() to fix (2) and (3) respectively.
> 
> A new local_set_cred_passthrough() helper based on fchownat() and fchmod()
> is introduced as a replacement to local_post_create_passthrough() to fix (4).
> No effort is made to factor out code because local_post_create_passthrough()
> will be dropped when all users have been converted to call the new helper.
> 
> The mapped and mapped-file security modes are supposed to be identical,
> except for the place where credentials and file modes are stored. While
> here, we also make that explicit by sharing the call to mknodat().
> 
> This partly fixes CVE-2016-9602.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-local.c |   82 +++++++++++++++++++++++++++++++---------------------
>  1 file changed, 49 insertions(+), 33 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 27/29] 9pfs: local: mkdir: don't follow symlinks

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1403 bytes --]

On Mon, Feb 20, 2017 at 03:42:50PM +0100, Greg Kurz wrote:
> The local_mkdir() callback is vulnerable to symlink attacks because it
> calls:
> 
> (1) mkdir() which follows symbolic links for all path elements but the
>     rightmost one
> (2) local_set_xattr()->setxattr() which follows symbolic links for all
>     path elements
> (3) local_set_mapped_file_attr() which calls in turn local_fopen() and
>     mkdir(), both functions following symbolic links for all path
>     elements but the rightmost one
> (4) local_post_create_passthrough() which calls in turn lchown() and
>     chmod(), both functions also following symbolic links
> 
> This patch converts local_mkdir() to rely on opendir_nofollow() and
> mkdirat() to fix (1), as well as local_set_xattrat(),
> local_set_mapped_file_attrat() and local_set_cred_passthrough() to
> fix (2), (3) and (4) respectively.
> 
> The mapped and mapped-file security modes are supposed to be identical,
> except for the place where credentials and file modes are stored. While
> here, we also make that explicit by sharing the call to mkdirat().
> 
> This partly fixes CVE-2016-9602.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-local.c |   55 +++++++++++++++++++---------------------------------
>  1 file changed, 20 insertions(+), 35 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 28/29] 9pfs: local: open2: don't follow symlinks

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1557 bytes --]

On Mon, Feb 20, 2017 at 03:42:58PM +0100, Greg Kurz wrote:
> The local_open2() callback is vulnerable to symlink attacks because it
> calls:
> 
> (1) open() which follows symbolic links for all path elements but the
>     rightmost one
> (2) local_set_xattr()->setxattr() which follows symbolic links for all
>     path elements
> (3) local_set_mapped_file_attr() which calls in turn local_fopen() and
>     mkdir(), both functions following symbolic links for all path
>     elements but the rightmost one
> (4) local_post_create_passthrough() which calls in turn lchown() and
>     chmod(), both functions also following symbolic links
> 
> This patch converts local_open2() to rely on opendir_nofollow() and
> mkdirat() to fix (1), as well as local_set_xattrat(),
> local_set_mapped_file_attrat() and local_set_cred_passthrough() to
> fix (2), (3) and (4) respectively. Since local_open2() already opens
> a descriptor to the target file, local_set_cred_passthrough() is
> modified to reuse it instead of opening a new one.
> 
> The mapped and mapped-file security modes are supposed to be identical,
> except for the place where credentials and file modes are stored. While
> here, we also make that explicit by sharing the call to openat().
> 
> This partly fixes CVE-2016-9602.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-local.c |   70 ++++++++++++++++++++++------------------------------
>  1 file changed, 29 insertions(+), 41 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 29/29] 9pfs: local: drop unused code

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 388 bytes --]

On Mon, Feb 20, 2017 at 03:43:07PM +0100, Greg Kurz wrote:
> Now that the all callbacks have been converted to use "at" syscalls, we
> can drop this code.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-local.c |  198 ----------------------------------------------------
>  1 file changed, 198 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 07/29] 9pfs: local: introduce symlink-attack safe xattr helpers

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 1231 bytes --]

On Thu, 23 Feb 2017 16:05:02 +0100
Jann Horn <jannh@google.com> wrote:

> On Thu, Feb 23, 2017 at 4:02 PM, Eric Blake <eblake@redhat.com> wrote:
> > On 02/20/2017 08:40 AM, Greg Kurz wrote:  
> >> All operations dealing with extended attributes are vulnerable to symlink
> >> attacks because they use path-based syscalls which can traverse symbolic
> >> links while walking through the dirname part of the path.
> >>
> >> The solution is to introduce helpers based on opendir_nofollow(). This
> >> calls for "at" versions of the extended attribute syscalls, which don't
> >> exist unfortunately. This patch implement them by simulating the "at"
> >> behavior with fchdir(). Since the current working directory is process
> >> wide, and we don't want to confuse another thread in QEMU, all the work
> >> is done in a separate process.  
> >
> > Can you emulate *at using /proc/fd/nnn/xyz?  
> 
> I don't know much about QEMU internals, but QEMU supports running in a
> chroot using the -chroot option, right? Does that already require procfs to be
> mounted inside the chroot?

Calling chroot() requires CAP_SYS_CHROOT and QEMU shouldn't rely on that to
provide a secure and isolated environment to run VMs.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH 07/29] 9pfs: local: introduce symlink-attack safe xattr helpers

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 2728 bytes --]

On Thu, 23 Feb 2017 13:44:41 +0000
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Mon, Feb 20, 2017 at 03:40:15PM +0100, Greg Kurz wrote:
> > +static ssize_t do_xattrat_op(int op_type, int dirfd, const char *path,
> > +                             const char *name, void *value, size_t size,
> > +                             int flags)
> > +{
> > +    struct xattrat_data *data;
> > +    pid_t pid;
> > +    ssize_t ret = -1;
> > +    int wstatus;
> > +
> > +    data = mmap(NULL, sizeof(*data) + size, PROT_READ | PROT_WRITE,
> > +                MAP_SHARED | MAP_ANONYMOUS, -1, 0);
> > +    if (data == MAP_FAILED) {
> > +        return -1;
> > +    }
> > +    data->ret = -1;
> > +
> > +    pid = fork();
> > +    if (pid < 0) {
> > +        goto err_out;
> > +    } else if (pid == 0) {
> > +        if (fchdir(dirfd) == 0) {
> > +            switch (op_type) {
> > +            case XATTRAT_OP_GET:
> > +                data->ret = lgetxattr(path, name, data->value, size);
> > +                break;
> > +            case XATTRAT_OP_LIST:
> > +                data->ret = llistxattr(path, data->value, size);
> > +                break;
> > +            case XATTRAT_OP_SET:
> > +                data->ret = lsetxattr(path, name, value, size, flags);
> > +                break;
> > +            case XATTRAT_OP_REMOVE:
> > +                data->ret = lremovexattr(path, name);
> > +                break;
> > +            default:
> > +                g_assert_not_reached();
> > +            }
> > +        }
> > +        data->serrno = errno;
> > +        _exit(0);
> > +    }
> > +    assert(waitpid(pid, &wstatus, 0) == pid && WIFEXITED(wstatus));
> > +
> > +    ret = data->ret;
> > +    if (ret < 0) {
> > +        errno = data->serrno;
> > +        goto err_out;
> > +    }
> > +    if (value) {
> > +        memcpy(value, data->value, data->ret);
> > +    }
> > +err_out:
> > +    munmap_preserver_errno(data, sizeof(*data) + size);
> > +    return ret;
> > +}  
> 
> Forking is ugly since QEMU is a multi-threaded program.  We brainstormed

Yeah, forking is ugly and it completely ruins metadata performance (x30
slower in passthrough mode and x300 slower in mapped-xattr mode).

> alternatives on IRC like using /proc/self/fd/$fd to work around the
> missing getxattrat() API.
> 

This should do the trick indeed. If we have to call getxattr()
on some untrusted $path that may be modified by the guest. We
can do:

dirfd = openat_nofollow($mount_fd, dirname($path))
filename = basename($path)

and then we can safely call:

lgetxattr("/proc/self/fd/$dirfd/$filename")

since "/proc/self/fd/$dirfd" is trusted.

> Stefan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH 07/29] 9pfs: local: introduce symlink-attack safe xattr helpers

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 1352 bytes --]

On Thu, 23 Feb 2017 09:02:39 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 02/20/2017 08:40 AM, Greg Kurz wrote:
> > All operations dealing with extended attributes are vulnerable to symlink
> > attacks because they use path-based syscalls which can traverse symbolic
> > links while walking through the dirname part of the path.
> > 
> > The solution is to introduce helpers based on opendir_nofollow(). This
> > calls for "at" versions of the extended attribute syscalls, which don't
> > exist unfortunately. This patch implement them by simulating the "at"
> > behavior with fchdir(). Since the current working directory is process
> > wide, and we don't want to confuse another thread in QEMU, all the work
> > is done in a separate process.  
> 
> Can you emulate *at using /proc/fd/nnn/xyz?  Coreutils was one of the
> early adopters of the power of *at functions, and found that emulation
> of *at via procfs was a LOT more efficient than emulation via fchdir
> (although both emulations still exist in gnulib, since procfs is not
> universal).
> 

Yeah, Stefan suggested this on irc. I had also found a tentative patchset to
implement genuine f*xattrat() calls in the kernel 3 yrs ago, that never got
merged. The author, Florian Weimer, also told me /proc was the way to go.

It looks like we have a consensus :)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH 13/29] 9pfs: local: remove: don't follow symlinks

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 1375 bytes --]

On Thu, 23 Feb 2017 14:23:15 +0000
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Mon, Feb 20, 2017 at 03:41:00PM +0100, Greg Kurz wrote:
> > +    dirfd = local_opendir_nofollow(ctx, dirpath);
> > +    if (dirfd) {
> > +        goto out;
> >      }
> >  
> > -    buffer = rpath(ctx, path);
> > -    err = remove(buffer);
> > -    g_free(buffer);
> > +    if (fstatat(dirfd, path, &stbuf, AT_SYMLINK_NOFOLLOW) < 0) {
> > +        goto err_out;
> > +    }
> > +
> > +    if (S_ISDIR(stbuf.st_mode)) {
> > +        flags |= AT_REMOVEDIR;
> > +    }
> > +
> > +    err = local_unlinkat_common(ctx, dirfd, name, flags);  
> 
> The alternative is optimistically skip fstat but then do:
> 
>   if (err == EISDIR) {

It would be err == -1 && errno == EISDIR actually.

>       err = local_unlinkat_common(ctx, dirfd, name, flags | AT_REMOVEDIR);
>   }
> 
> It might be faster.
> 

This would work for passthrough and mapped modes indeed. For mapped-file
mode, things are more complicated though. If path points to a directory
and we call local_unlinkat_common() without AT_REMOVEDIR, it won't unlink
the metadata directory and unlinkat() will fail with ENOENT because
the directory isn't empty... I'd rather try to optimize in a followup
patch later to avoid the extra complexity.

> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH 23/29] 9pfs: local: chmod: don't follow symlinks

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 8390 bytes --]

On Thu, 23 Feb 2017 15:10:42 +0000
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Mon, Feb 20, 2017 at 03:42:19PM +0100, Greg Kurz wrote:
> > The local_chmod() callback is vulnerable to symlink attacks because it
> > calls:
> > 
> > (1) chmod() which follows symbolic links for all path elements
> > (2) local_set_xattr()->setxattr() which follows symbolic links for all
> >     path elements
> > (3) local_set_mapped_file_attr() which calls in turn local_fopen() and
> >     mkdir(), both functions following symbolic links for all path
> >     elements but the rightmost one
> > 
> > This patch converts local_chmod() to rely on open_nofollow() and
> > fchmod() to fix (1).
> > 
> > It introduces a local_set_xattrat() replacement to local_set_xattr()
> > based on fsetxattrat() to fix (2), and a local_set_mapped_file_attrat()
> > replacement to local_set_mapped_file_attr() based on local_fopenat()
> > and mkdirat() to fix (3). No effort is made to factor out code because
> > both local_set_xattr() and local_set_mapped_file_attr() will be dropped
> > when all users have been converted to use the "at" versions.
> > 
> > This partly fixes CVE-2016-9602.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/9pfs/9p-local.c |  163 ++++++++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 150 insertions(+), 13 deletions(-)
> > 
> > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > index fb536fdb3082..56d7731f7ce1 100644
> > --- a/hw/9pfs/9p-local.c
> > +++ b/hw/9pfs/9p-local.c
> > @@ -327,6 +327,87 @@ err_out:
> >      return ret;
> >  }
> >  
> > +static int local_set_mapped_file_attrat(int dirfd, const char *name,
> > +                                        FsCred *credp)
> > +{
> > +    FILE *fp;
> > +    int ret;
> > +    char buf[ATTR_MAX];
> > +    int uid = -1, gid = -1, mode = -1, rdev = -1;
> > +    int map_dirfd;
> > +
> > +    ret = mkdirat(dirfd, VIRTFS_META_DIR, 0700);
> > +    if (ret < 0 && errno != EEXIST) {
> > +        return -1;
> > +    }
> > +
> > +    map_dirfd = openat(dirfd, VIRTFS_META_DIR,
> > +                       O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
> > +    if (map_dirfd == -1) {
> > +        return -1;
> > +    }
> > +
> > +    fp = local_fopenat(map_dirfd, name, "r");
> > +    if (!fp) {
> > +        if (errno == ENOENT) {
> > +            goto update_map_file;
> > +        } else {
> > +            close_preserve_errno(map_dirfd);
> > +            return -1;
> > +        }
> > +    }
> > +    memset(buf, 0, ATTR_MAX);
> > +    while (fgets(buf, ATTR_MAX, fp)) {
> > +        if (!strncmp(buf, "virtfs.uid", 10)) {
> > +            uid = atoi(buf + 11);
> > +        } else if (!strncmp(buf, "virtfs.gid", 10)) {
> > +            gid = atoi(buf + 11);
> > +        } else if (!strncmp(buf, "virtfs.mode", 11)) {
> > +            mode = atoi(buf + 12);
> > +        } else if (!strncmp(buf, "virtfs.rdev", 11)) {
> > +            rdev = atoi(buf + 12);
> > +        }
> > +        memset(buf, 0, ATTR_MAX);
> > +    }
> > +    fclose(fp);
> > +
> > +update_map_file:
> > +    fp = local_fopenat(map_dirfd, name, "w");
> > +    close_preserve_errno(map_dirfd);
> > +    if (!fp) {
> > +        return -1;
> > +    }
> > +
> > +    if (credp->fc_uid != -1) {
> > +        uid = credp->fc_uid;
> > +    }
> > +    if (credp->fc_gid != -1) {
> > +        gid = credp->fc_gid;
> > +    }
> > +    if (credp->fc_mode != -1) {
> > +        mode = credp->fc_mode;
> > +    }
> > +    if (credp->fc_rdev != -1) {
> > +        rdev = credp->fc_rdev;
> > +    }
> > +
> > +    if (uid != -1) {
> > +        fprintf(fp, "virtfs.uid=%d\n", uid);
> > +    }
> > +    if (gid != -1) {
> > +        fprintf(fp, "virtfs.gid=%d\n", gid);
> > +    }
> > +    if (mode != -1) {
> > +        fprintf(fp, "virtfs.mode=%d\n", mode);
> > +    }
> > +    if (rdev != -1) {
> > +        fprintf(fp, "virtfs.rdev=%d\n", rdev);
> > +    }
> > +    fclose(fp);
> > +
> > +    return 0;
> > +}
> > +
> >  static int local_set_xattr(const char *path, FsCred *credp)
> >  {
> >      int err;
> > @@ -362,6 +443,45 @@ static int local_set_xattr(const char *path, FsCred *credp)
> >      return 0;
> >  }
> >  
> > +static int local_set_xattrat(int dirfd, const char *path, FsCred *credp)
> > +{
> > +    int err;
> > +
> > +    if (credp->fc_uid != -1) {
> > +        uint32_t tmp_uid = cpu_to_le32(credp->fc_uid);
> > +        err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.uid", &tmp_uid,
> > +                                   sizeof(uid_t), 0);
> > +        if (err) {
> > +            return err;
> > +        }
> > +    }
> > +    if (credp->fc_gid != -1) {
> > +        uint32_t tmp_gid = cpu_to_le32(credp->fc_gid);
> > +        err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.gid", &tmp_gid,
> > +                                   sizeof(gid_t), 0);
> > +        if (err) {
> > +            return err;
> > +        }
> > +    }
> > +    if (credp->fc_mode != -1) {
> > +        uint32_t tmp_mode = cpu_to_le32(credp->fc_mode);
> > +        err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.mode", &tmp_mode,
> > +                                   sizeof(mode_t), 0);
> > +        if (err) {
> > +            return err;
> > +        }
> > +    }
> > +    if (credp->fc_rdev != -1) {
> > +        uint64_t tmp_rdev = cpu_to_le64(credp->fc_rdev);
> > +        err = fsetxattrat_nofollow(dirfd, path, "user.virtfs.rdev", &tmp_rdev,
> > +                                   sizeof(dev_t), 0);
> > +        if (err) {
> > +            return err;
> > +        }
> > +    }
> > +    return 0;
> > +}
> > +
> >  static int local_post_create_passthrough(FsContext *fs_ctx, const char *path,
> >                                           FsCred *credp)
> >  {
> > @@ -553,21 +673,38 @@ static ssize_t local_pwritev(FsContext *ctx, V9fsFidOpenState *fs,
> >  
> >  static int local_chmod(FsContext *fs_ctx, V9fsPath *fs_path, FsCred *credp)
> >  {
> > -    char *buffer;
> >      int ret = -1;
> > -    char *path = fs_path->data;
> > +    int dirfd;
> >  
> > -    if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
> > -        buffer = rpath(fs_ctx, path);
> > -        ret = local_set_xattr(buffer, credp);
> > -        g_free(buffer);
> > -    } else if (fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
> > -        return local_set_mapped_file_attr(fs_ctx, path, credp);
> > -    } else if ((fs_ctx->export_flags & V9FS_SM_PASSTHROUGH) ||
> > -               (fs_ctx->export_flags & V9FS_SM_NONE)) {
> > -        buffer = rpath(fs_ctx, path);
> > -        ret = chmod(buffer, credp->fc_mode);
> > -        g_free(buffer);
> > +    if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
> > +        fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
> > +        char *dirpath, *name;
> > +
> > +        dirpath = g_path_get_dirname(fs_path->data);
> > +        dirfd = local_opendir_nofollow(fs_ctx, dirpath);
> > +        g_free(dirpath);
> > +        if (dirfd == -1) {
> > +            return -1;
> > +        }
> > +
> > +        name = g_path_get_basename(fs_path->data);
> > +        if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
> > +            ret = local_set_xattrat(dirfd, name, credp);
> > +        } else {
> > +            ret = local_set_mapped_file_attrat(dirfd, name, credp);
> > +        }
> > +        g_free(name);
> > +        close_preserve_errno(dirfd);
> > +    } else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
> > +               fs_ctx->export_flags & V9FS_SM_NONE) {
> > +        int fd;
> > +
> > +        fd = local_open_nofollow(fs_ctx, fs_path->data, O_RDONLY, 0);
> > +        if (fd == -1) {
> > +            return -1;
> > +        }
> > +        ret = fchmod(fd, credp->fc_mode);
> > +        close_preserve_errno(fd);  
> 
> As mentioned on IRC, not sure this is workable since chmod(2) must not
> require read permission on the file.  This patch introduces failures
> when the file isn't readable.
> 

Yeah :-\ This one is the more painful issue to address. I need a
chmod() variant that would fail on symlinks instead of following
them... and I'm kinda suffering to implement this in a race-free
manner.

Cc'ing Eric for insights.

> Stefan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH 23/29] 9pfs: local: chmod: don't follow symlinks

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3506 bytes --]

On 02/24/2017 04:34 AM, Greg Kurz wrote:
> On Thu, 23 Feb 2017 15:10:42 +0000
> Stefan Hajnoczi <stefanha@gmail.com> wrote:
> 
>> On Mon, Feb 20, 2017 at 03:42:19PM +0100, Greg Kurz wrote:
>>> The local_chmod() callback is vulnerable to symlink attacks because it
>>> calls:
>>>
>>> (1) chmod() which follows symbolic links for all path elements
>>> (2) local_set_xattr()->setxattr() which follows symbolic links for all
>>>     path elements
>>> (3) local_set_mapped_file_attr() which calls in turn local_fopen() and
>>>     mkdir(), both functions following symbolic links for all path
>>>     elements but the rightmost one
>>>

>>> +        fd = local_open_nofollow(fs_ctx, fs_path->data, O_RDONLY, 0);
>>> +        if (fd == -1) {
>>> +            return -1;
>>> +        }
>>> +        ret = fchmod(fd, credp->fc_mode);
>>> +        close_preserve_errno(fd);  
>>
>> As mentioned on IRC, not sure this is workable since chmod(2) must not
>> require read permission on the file.  This patch introduces failures
>> when the file isn't readable.
>>
> 
> Yeah :-\ This one is the more painful issue to address. I need a
> chmod() variant that would fail on symlinks instead of following
> them... and I'm kinda suffering to implement this in a race-free
> manner.

BSD has lchmod(), but Linux does not.  And Linux does not yet properly
implement AT_SYMLINK_NOFOLLOW.  (The BSD semantics are pretty cool:
restricting mode bits on a symlink can create scenarios where some users
can dereference the symlink but others get ENOENT failures - but I don't
know of any effort to add those semantics to the Linux kernel)

POSIX says support for AT_SYMLINK_NOFOLLOW is optional, precisely
because POSIX does not require permissions on symlinks to matter, but
the way it requires it is that AT_SYMLINK_NOFOLLOW is supposed to be a
no-op for regular files, and cause an EOPNOTSUPP failure for symlinks.

Unfortunately, the Linux man page says that Linux fails with ENOTSUP
(which is identical to EOPNOTSUPP) any time AT_SYMLINK_NOFOLLOW is set,
and not just if it is attempted on a symlink.  And I just verified that
poor behavior as follows:

$ cat foo.c
#include <stdio.h>
#include <fcntl.h>
#include <sys/stat.h>
#include <unistd.h>
#include <errno.h>

int main(int argc, char **argv)
{
    int ret = 1;
    if (symlink("a", "l") < 0)
        goto cleanup;
    if (fchmodat(AT_FDCWD, "l", 0600, AT_SYMLINK_NOFOLLOW) == 0)
        printf("kernel supports mode bits on symlinks\n");
    else if (errno != EOPNOTSUPP) {
        printf("Oops, kernel failed with %m on symlink\n");
        goto cleanup;
    }
    if (creat("f", 0600) < 0)
        goto cleanup;
    if (fchmodat(AT_FDCWD, "f", 0600, AT_SYMLINK_NOFOLLOW) < 0) {
        printf("Oops, kernel failed with %m on regular file\n");
        goto cleanup;
    }
    ret = 0;
 cleanup:
    remove("l");
    remove("f");
    return ret;
}
$ ./foo
Oops, kernel failed with Operation not supported on regular file

If you were to get that kernel bug fixed, then you could use fchmodat()
to do a safe chmod() which does not chase through a final symlink, and
relative to a safe directory fd that you obtain for the rest of the path
(as done elsewhere in the series).  Use the CVE nature of the problem as
leverage on the kernel developers, if that's what it takes.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 23/29] 9pfs: local: chmod: don't follow symlinks

[Jann Horn]

On Fri, Feb 24, 2017 at 4:23 PM, Eric Blake <eblake@redhat.com> wrote:
> On 02/24/2017 04:34 AM, Greg Kurz wrote:
>> On Thu, 23 Feb 2017 15:10:42 +0000
>> Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>
>>> On Mon, Feb 20, 2017 at 03:42:19PM +0100, Greg Kurz wrote:
>>>> The local_chmod() callback is vulnerable to symlink attacks because it
>>>> calls:
>>>>
>>>> (1) chmod() which follows symbolic links for all path elements
>>>> (2) local_set_xattr()->setxattr() which follows symbolic links for all
>>>>     path elements
>>>> (3) local_set_mapped_file_attr() which calls in turn local_fopen() and
>>>>     mkdir(), both functions following symbolic links for all path
>>>>     elements but the rightmost one
>>>>
>
>>>> +        fd = local_open_nofollow(fs_ctx, fs_path->data, O_RDONLY, 0);
>>>> +        if (fd == -1) {
>>>> +            return -1;
>>>> +        }
>>>> +        ret = fchmod(fd, credp->fc_mode);
>>>> +        close_preserve_errno(fd);
>>>
>>> As mentioned on IRC, not sure this is workable since chmod(2) must not
>>> require read permission on the file.  This patch introduces failures
>>> when the file isn't readable.
>>>
>>
>> Yeah :-\ This one is the more painful issue to address. I need a
>> chmod() variant that would fail on symlinks instead of following
>> them... and I'm kinda suffering to implement this in a race-free
>> manner.
>
> BSD has lchmod(), but Linux does not.  And Linux does not yet properly
> implement AT_SYMLINK_NOFOLLOW.  (The BSD semantics are pretty cool:
> restricting mode bits on a symlink can create scenarios where some users
> can dereference the symlink but others get ENOENT failures - but I don't
> know of any effort to add those semantics to the Linux kernel)
>
> POSIX says support for AT_SYMLINK_NOFOLLOW is optional, precisely
> because POSIX does not require permissions on symlinks to matter, but
> the way it requires it is that AT_SYMLINK_NOFOLLOW is supposed to be a
> no-op for regular files, and cause an EOPNOTSUPP failure for symlinks.
>
> Unfortunately, the Linux man page says that Linux fails with ENOTSUP
> (which is identical to EOPNOTSUPP) any time AT_SYMLINK_NOFOLLOW is set,
> and not just if it is attempted on a symlink.

And unfortunately, that flags argument is not actually present in the
real syscall.
See this glibc code:

int
fchmodat (int fd, const char *file, mode_t mode, int flag)
{
  if (flag & ~AT_SYMLINK_NOFOLLOW)
    return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
#ifndef __NR_lchmod /* Linux so far has no lchmod syscall.  */
  if (flag & AT_SYMLINK_NOFOLLOW)
    return INLINE_SYSCALL_ERROR_RETURN_VALUE (ENOTSUP);
#endif

  return INLINE_SYSCALL (fchmodat, 3, fd, file, mode);
}

and this kernel code:

SYSCALL_DEFINE3(fchmodat, int, dfd, const char __user *, filename,
umode_t, mode)
{
  [...]
}

So to fix this, you'll probably have to add a new syscall fchmodat2()
to the kernel,
wire it up for all the architectures and get the various libc
implementations to adopt
that. That's going to be quite tedious. :(

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3612 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 23/29] 9pfs: local: chmod: don't follow symlinks

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 1525 bytes --]

On Fri, 24 Feb 2017 17:22:19 +0100
Jann Horn <jannh@google.com> wrote:
> [...]
> And unfortunately, that flags argument is not actually present in the
> real syscall.
> See this glibc code:
> 
> int
> fchmodat (int fd, const char *file, mode_t mode, int flag)
> {
>   if (flag & ~AT_SYMLINK_NOFOLLOW)
>     return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
> #ifndef __NR_lchmod /* Linux so far has no lchmod syscall.  */
>   if (flag & AT_SYMLINK_NOFOLLOW)
>     return INLINE_SYSCALL_ERROR_RETURN_VALUE (ENOTSUP);
> #endif
> 
>   return INLINE_SYSCALL (fchmodat, 3, fd, file, mode);
> }
> 
> and this kernel code:
> 
> SYSCALL_DEFINE3(fchmodat, int, dfd, const char __user *, filename,
> umode_t, mode)
> {
>   [...]
> }
> 
> So to fix this, you'll probably have to add a new syscall fchmodat2()
> to the kernel,
> wire it up for all the architectures and get the various libc
> implementations to adopt
> that. That's going to be quite tedious. :(

Yeah, Eric and I had a discussion about that on irc. I'll start to
work on the kernel part, at least. Indeed, adoption by the various
libc is likely to take some time... When the syscalls are available
in the kernel, maybe it is possible to implement something in the
9pfs code with the syscall() function.

In the meantime, we'll have to live with a degraded version of
fchmodat() based on openat()+fchmod(). This will fail if the
file isn't accessible but it is better than allowing the guest
to chmod() any file on the host.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH 11/29] 9pfs: local: lremovexattr: don't follow symlinks

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 3930 bytes --]

On Mon, 20 Feb 2017 15:40:45 +0100
Greg Kurz <groug@kaod.org> wrote:

> The local_lremovexattr() callback is vulnerable to symlink attacks because
> it calls lremovexattr() which follows symbolic links in all path elements but
> the rightmost one.
> 
> This patch converts local_lremovexattr() to rely on opendir_nofollow() and
> fremovexattrat_nofollow() instead.
> 
> This partly fixes CVE-2016-9602.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/9p-posix-acl.c  |   10 ++--------
>  hw/9pfs/9p-xattr-user.c |    8 +-------
>  hw/9pfs/9p-xattr.c      |    8 +-------
>  3 files changed, 4 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/9pfs/9p-posix-acl.c b/hw/9pfs/9p-posix-acl.c
> index 0154e2a7605f..c20409104135 100644
> --- a/hw/9pfs/9p-posix-acl.c
> +++ b/hw/9pfs/9p-posix-acl.c
> @@ -58,10 +58,8 @@ static int mp_pacl_removexattr(FsContext *ctx,
>                                 const char *path, const char *name)
>  {
>      int ret;
> -    char *buffer;
>  
> -    buffer = rpath(ctx, path);
> -    ret  = lremovexattr(buffer, MAP_ACL_ACCESS);
> +    ret = local_removexattr_nofollow(ctx, MAP_ACL_ACCESS, name);

While reworking on a new implementation fremovexattrat(), I realized the
above should be:

    ret = local_removexattr_nofollow(ctx, path, MAP_ACL_ACCESS);

>      if (ret == -1 && errno == ENODATA) {
>          /*
>           * We don't get ENODATA error when trying to remove a
> @@ -71,7 +69,6 @@ static int mp_pacl_removexattr(FsContext *ctx,
>          errno = 0;
>          ret = 0;
>      }
> -    g_free(buffer);
>      return ret;
>  }
>  
> @@ -111,10 +108,8 @@ static int mp_dacl_removexattr(FsContext *ctx,
>                                 const char *path, const char *name)
>  {
>      int ret;
> -    char *buffer;
>  
> -    buffer = rpath(ctx, path);
> -    ret  = lremovexattr(buffer, MAP_ACL_DEFAULT);
> +    ret = local_removexattr_nofollow(ctx, MAP_ACL_DEFAULT, name);

Same error here.

>      if (ret == -1 && errno == ENODATA) {
>          /*
>           * We don't get ENODATA error when trying to remove a
> @@ -124,7 +119,6 @@ static int mp_dacl_removexattr(FsContext *ctx,
>          errno = 0;
>          ret = 0;
>      }
> -    g_free(buffer);
>      return ret;
>  }
>  
> diff --git a/hw/9pfs/9p-xattr-user.c b/hw/9pfs/9p-xattr-user.c
> index 1840a5db66f3..2c90817b75a6 100644
> --- a/hw/9pfs/9p-xattr-user.c
> +++ b/hw/9pfs/9p-xattr-user.c
> @@ -81,9 +81,6 @@ static int mp_user_setxattr(FsContext *ctx, const char *path, const char *name,
>  static int mp_user_removexattr(FsContext *ctx,
>                                 const char *path, const char *name)
>  {
> -    char *buffer;
> -    int ret;
> -
>      if (strncmp(name, "user.virtfs.", 12) == 0) {
>          /*
>           * Don't allow fetch of user.virtfs namesapce
> @@ -92,10 +89,7 @@ static int mp_user_removexattr(FsContext *ctx,
>          errno = EACCES;
>          return -1;
>      }
> -    buffer = rpath(ctx, path);
> -    ret = lremovexattr(buffer, name);
> -    g_free(buffer);
> -    return ret;
> +    return local_removexattr_nofollow(ctx, path, name);
>  }
>  
>  XattrOperations mapped_user_xattr = {
> diff --git a/hw/9pfs/9p-xattr.c b/hw/9pfs/9p-xattr.c
> index 72ef820f16d7..6fbfbca7e9a0 100644
> --- a/hw/9pfs/9p-xattr.c
> +++ b/hw/9pfs/9p-xattr.c
> @@ -333,13 +333,7 @@ int pt_setxattr(FsContext *ctx, const char *path, const char *name, void *value,
>  
>  int pt_removexattr(FsContext *ctx, const char *path, const char *name)
>  {
> -    char *buffer;
> -    int ret;
> -
> -    buffer = rpath(ctx, path);
> -    ret = lremovexattr(path, name);
> -    g_free(buffer);
> -    return ret;
> +    return local_removexattr_nofollow(ctx, path, name);
>  }
>  
>  ssize_t notsup_getxattr(FsContext *ctx, const char *path, const char *name,
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 688 bytes --]

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 ?

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 966 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]