diff mbox

[v2,08/28] 9pfs: local: llistxattr: don't follow symlinks

Message ID 148814895464.28146.14337427309543597613.stgit@bahia
State New
Headers show

Commit Message

Greg Kurz Feb. 26, 2017, 10:42 p.m. UTC
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 introduces a helper to emulate the non-existing flistxattrat()
function: it is implemented with /proc/self/fd which provides a trusted
path that can be safely passed to llistxattr().

local_llistxattr() is converted to use this helper and opendir_nofollow().

This partly fixes CVE-2016-9602.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
v2: - introduce /proc based flistxattrat_nofollow()
---
 hw/9pfs/9p-xattr.c |   35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

Comments

Stefan Hajnoczi Feb. 27, 2017, 1:08 p.m. UTC | #1
On Sun, Feb 26, 2017 at 11:42:34PM +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 introduces a helper to emulate the non-existing flistxattrat()
> function: it is implemented with /proc/self/fd which provides a trusted
> path that can be safely passed to llistxattr().
> 
> local_llistxattr() is converted to use this helper and opendir_nofollow().
> 
> This partly fixes CVE-2016-9602.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> v2: - introduce /proc based flistxattrat_nofollow()
> ---
>  hw/9pfs/9p-xattr.c |   35 +++++++++++++++++++++++++++++------
>  1 file changed, 29 insertions(+), 6 deletions(-)

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

Patch

diff --git a/hw/9pfs/9p-xattr.c b/hw/9pfs/9p-xattr.c
index aa4391e6b317..54193c630c9d 100644
--- a/hw/9pfs/9p-xattr.c
+++ b/hw/9pfs/9p-xattr.c
@@ -60,6 +60,16 @@  ssize_t pt_listxattr(FsContext *ctx, const char *path,
     return name_size;
 }
 
+static ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
+                                     char *list, size_t size)
+{
+    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
+    int ret;
+
+    ret = llistxattr(proc_path, list, size);
+    g_free(proc_path);
+    return ret;
+}
 
 /*
  * Get the list and pass to each layer to find out whether
@@ -69,24 +79,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_nofollow(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_nofollow(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;