yeahuh / rpms / qemu-kvm

Forked from rpms/qemu-kvm 2 years ago
Clone

Blame SOURCES/kvm-virtiofsd-prevent-opening-of-special-files-CVE-2020-.patch

f45498
From 0c394cf4347c49ac6d7d2ee5013009f41a59118f Mon Sep 17 00:00:00 2001
f45498
From: Jon Maloy <jmaloy@redhat.com>
f45498
Date: Wed, 10 Feb 2021 00:02:17 -0500
f45498
Subject: [PATCH 3/3] virtiofsd: prevent opening of special files
f45498
 (CVE-2020-35517)
f45498
f45498
RH-Author: Jon Maloy <jmaloy@redhat.com>
f45498
Message-id: <20210210000217.1561306-4-jmaloy@redhat.com>
f45498
Patchwork-id: 101028
f45498
O-Subject: [RHEL-8.3.0.z qemu-kvm PATCH 3/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
f45498
Bugzilla: 1919109
f45498
RH-Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
f45498
RH-Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
f45498
RH-Acked-by: Greg Kurz <gkurz@redhat.com>
f45498
f45498
From: Stefan Hajnoczi <stefanha@redhat.com>
f45498
f45498
A well-behaved FUSE client does not attempt to open special files with
f45498
FUSE_OPEN because they are handled on the client side (e.g. device nodes
f45498
are handled by client-side device drivers).
f45498
f45498
The check to prevent virtiofsd from opening special files is missing in
f45498
a few cases, most notably FUSE_OPEN. A malicious client can cause
f45498
virtiofsd to open a device node, potentially allowing the guest to
f45498
escape. This can be exploited by a modified guest device driver. It is
f45498
not exploitable from guest userspace since the guest kernel will handle
f45498
special files inside the guest instead of sending FUSE requests.
f45498
f45498
This patch fixes this issue by introducing the lo_inode_open() function
f45498
to check the file type before opening it. This is a short-term solution
f45498
because it does not prevent a compromised virtiofsd process from opening
f45498
device nodes on the host.
f45498
f45498
Restructure lo_create() to try O_CREAT | O_EXCL first. Note that O_CREAT
f45498
| O_EXCL does not follow symlinks, so O_NOFOLLOW masking is not
f45498
necessary here. If the file exists and the user did not specify O_EXCL,
f45498
open it via lo_do_open().
f45498
f45498
Reported-by: Alex Xu <alex@alxu.ca>
f45498
Fixes: CVE-2020-35517
f45498
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
f45498
Reviewed-by: Vivek Goyal <vgoyal@redhat.com>
f45498
Reviewed-by: Greg Kurz <groug@kaod.org>
f45498
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
f45498
Message-Id: <20210204150208.367837-4-stefanha@redhat.com>
f45498
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
f45498
f45498
(cherry picked from commit a3fdbbc7f271bff7d53d0501b29d910ece0b3789)
f45498
Signed-off-by: Jon Maloy <jmaloy@redhat.com>
f45498
Signed-off-by: Jon Maloy <jmaloy.redhat.com>
f45498
---
f45498
 tools/virtiofsd/passthrough_ll.c | 144 ++++++++++++++++++++-----------
f45498
 1 file changed, 92 insertions(+), 52 deletions(-)
f45498
f45498
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
f45498
index e5bd3d73e4..cb0992f2db 100644
f45498
--- a/tools/virtiofsd/passthrough_ll.c
f45498
+++ b/tools/virtiofsd/passthrough_ll.c
f45498
@@ -535,6 +535,38 @@ static int lo_fd(fuse_req_t req, fuse_ino_t ino)
f45498
     return fd;
f45498
 }
f45498
 
f45498
+/*
f45498
+ * Open a file descriptor for an inode. Returns -EBADF if the inode is not a
f45498
+ * regular file or a directory.
f45498
+ *
f45498
+ * Use this helper function instead of raw openat(2) to prevent security issues
f45498
+ * when a malicious client opens special files such as block device nodes.
f45498
+ * Symlink inodes are also rejected since symlinks must already have been
f45498
+ * traversed on the client side.
f45498
+ */
f45498
+static int lo_inode_open(struct lo_data *lo, struct lo_inode *inode,
f45498
+                         int open_flags)
f45498
+{
f45498
+    g_autofree char *fd_str = g_strdup_printf("%d", inode->fd);
f45498
+    int fd;
f45498
+
f45498
+    if (!S_ISREG(inode->filetype) && !S_ISDIR(inode->filetype)) {
f45498
+        return -EBADF;
f45498
+    }
f45498
+
f45498
+    /*
f45498
+     * The file is a symlink so O_NOFOLLOW must be ignored. We checked earlier
f45498
+     * that the inode is not a special file but if an external process races
f45498
+     * with us then symlinks are traversed here. It is not possible to escape
f45498
+     * the shared directory since it is mounted as "/" though.
f45498
+     */
f45498
+    fd = openat(lo->proc_self_fd, fd_str, open_flags & ~O_NOFOLLOW);
f45498
+    if (fd < 0) {
f45498
+        return -errno;
f45498
+    }
f45498
+    return fd;
f45498
+}
f45498
+
f45498
 static void lo_init(void *userdata, struct fuse_conn_info *conn)
f45498
 {
f45498
     struct lo_data *lo = (struct lo_data *)userdata;
f45498
@@ -788,9 +820,9 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
f45498
         if (fi) {
f45498
             truncfd = fd;
f45498
         } else {
f45498
-            sprintf(procname, "%i", ifd);
f45498
-            truncfd = openat(lo->proc_self_fd, procname, O_RDWR);
f45498
+            truncfd = lo_inode_open(lo, inode, O_RDWR);
f45498
             if (truncfd < 0) {
f45498
+                errno = -truncfd;
f45498
                 goto out_err;
f45498
             }
f45498
         }
f45498
@@ -894,7 +926,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
f45498
     struct lo_inode *dir = lo_inode(req, parent);
f45498
 
f45498
     if (inodep) {
f45498
-        *inodep = NULL;
f45498
+        *inodep = NULL; /* in case there is an error */
f45498
     }
f45498
 
f45498
     /*
f45498
@@ -1725,19 +1757,26 @@ static void update_open_flags(int writeback, struct fuse_file_info *fi)
f45498
     fi->flags &= ~O_DIRECT;
f45498
 }
f45498
 
f45498
+/*
f45498
+ * Open a regular file, set up an fd mapping, and fill out the struct
f45498
+ * fuse_file_info for it. If existing_fd is not negative, use that fd instead
f45498
+ * opening a new one. Takes ownership of existing_fd.
f45498
+ *
f45498
+ * Returns 0 on success or a positive errno.
f45498
+ */
f45498
 static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
f45498
-                      struct fuse_file_info *fi)
f45498
+                      int existing_fd, struct fuse_file_info *fi)
f45498
 {
f45498
-    char buf[64];
f45498
     ssize_t fh;
f45498
-    int fd;
f45498
+    int fd = existing_fd;
f45498
 
f45498
     update_open_flags(lo->writeback, fi);
f45498
 
f45498
-    sprintf(buf, "%i", inode->fd);
f45498
-    fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
f45498
-    if (fd == -1) {
f45498
-        return errno;
f45498
+    if (fd < 0) {
f45498
+        fd = lo_inode_open(lo, inode, fi->flags);
f45498
+        if (fd < 0) {
f45498
+            return -fd;
f45498
+        }
f45498
     }
f45498
 
f45498
     pthread_mutex_lock(&lo->mutex);
f45498
@@ -1760,9 +1799,10 @@ static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
f45498
 static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
f45498
                       mode_t mode, struct fuse_file_info *fi)
f45498
 {
f45498
-    int fd;
f45498
+    int fd = -1;
f45498
     struct lo_data *lo = lo_data(req);
f45498
     struct lo_inode *parent_inode;
f45498
+    struct lo_inode *inode = NULL;
f45498
     struct fuse_entry_param e;
f45498
     int err;
f45498
     struct lo_cred old = {};
f45498
@@ -1788,36 +1828,38 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
f45498
 
f45498
     update_open_flags(lo->writeback, fi);
f45498
 
f45498
-    fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW,
f45498
-                mode);
f45498
+    /* Try to create a new file but don't open existing files */
f45498
+    fd = openat(parent_inode->fd, name, fi->flags | O_CREAT | O_EXCL, mode);
f45498
     err = fd == -1 ? errno : 0;
f45498
-    lo_restore_cred(&old;;
f45498
 
f45498
-    if (!err) {
f45498
-        ssize_t fh;
f45498
+    lo_restore_cred(&old;;
f45498
 
f45498
-        pthread_mutex_lock(&lo->mutex);
f45498
-        fh = lo_add_fd_mapping(lo, fd);
f45498
-        pthread_mutex_unlock(&lo->mutex);
f45498
-        if (fh == -1) {
f45498
-            close(fd);
f45498
-            err = ENOMEM;
f45498
-            goto out;
f45498
-        }
f45498
+    /* Ignore the error if file exists and O_EXCL was not given */
f45498
+    if (err && (err != EEXIST || (fi->flags & O_EXCL))) {
f45498
+        goto out;
f45498
+    }
f45498
 
f45498
-        fi->fh = fh;
f45498
-        err = lo_do_lookup(req, parent, name, &e, NULL);
f45498
+    err = lo_do_lookup(req, parent, name, &e, &inode;;
f45498
+    if (err) {
f45498
+        goto out;
f45498
     }
f45498
-    if (lo->cache == CACHE_NONE) {
f45498
-        fi->direct_io = 1;
f45498
-    } else if (lo->cache == CACHE_ALWAYS) {
f45498
-        fi->keep_cache = 1;
f45498
+
f45498
+    err = lo_do_open(lo, inode, fd, fi);
f45498
+    fd = -1; /* lo_do_open() takes ownership of fd */
f45498
+    if (err) {
f45498
+        /* Undo lo_do_lookup() nlookup ref */
f45498
+        unref_inode_lolocked(lo, inode, 1);
f45498
     }
f45498
 
f45498
 out:
f45498
+    lo_inode_put(lo, &inode;;
f45498
     lo_inode_put(lo, &parent_inode);
f45498
 
f45498
     if (err) {
f45498
+        if (fd >= 0) {
f45498
+            close(fd);
f45498
+        }
f45498
+
f45498
         fuse_reply_err(req, err);
f45498
     } else {
f45498
         fuse_reply_create(req, &e, fi);
f45498
@@ -1831,7 +1873,6 @@ static struct lo_inode_plock *lookup_create_plock_ctx(struct lo_data *lo,
f45498
                                                       pid_t pid, int *err)
f45498
 {
f45498
     struct lo_inode_plock *plock;
f45498
-    char procname[64];
f45498
     int fd;
f45498
 
f45498
     plock =
f45498
@@ -1848,12 +1889,10 @@ static struct lo_inode_plock *lookup_create_plock_ctx(struct lo_data *lo,
f45498
     }
f45498
 
f45498
     /* Open another instance of file which can be used for ofd locks. */
f45498
-    sprintf(procname, "%i", inode->fd);
f45498
-
f45498
     /* TODO: What if file is not writable? */
f45498
-    fd = openat(lo->proc_self_fd, procname, O_RDWR);
f45498
-    if (fd == -1) {
f45498
-        *err = errno;
f45498
+    fd = lo_inode_open(lo, inode, O_RDWR);
f45498
+    if (fd < 0) {
f45498
+        *err = -fd;
f45498
         free(plock);
f45498
         return NULL;
f45498
     }
f45498
@@ -2000,7 +2039,7 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
f45498
         return;
f45498
     }
f45498
 
f45498
-    err = lo_do_open(lo, inode, fi);
f45498
+    err = lo_do_open(lo, inode, -1, fi);
f45498
     lo_inode_put(lo, &inode;;
f45498
     if (err) {
f45498
         fuse_reply_err(req, err);
f45498
@@ -2056,39 +2095,40 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
f45498
 static void lo_fsync(fuse_req_t req, fuse_ino_t ino, int datasync,
f45498
                      struct fuse_file_info *fi)
f45498
 {
f45498
+    struct lo_inode *inode = lo_inode(req, ino);
f45498
+    struct lo_data *lo = lo_data(req);
f45498
     int res;
f45498
     int fd;
f45498
-    char *buf;
f45498
 
f45498
     fuse_log(FUSE_LOG_DEBUG, "lo_fsync(ino=%" PRIu64 ", fi=0x%p)\n", ino,
f45498
              (void *)fi);
f45498
 
f45498
-    if (!fi) {
f45498
-        struct lo_data *lo = lo_data(req);
f45498
-
f45498
-        res = asprintf(&buf, "%i", lo_fd(req, ino));
f45498
-        if (res == -1) {
f45498
-            return (void)fuse_reply_err(req, errno);
f45498
-        }
f45498
+    if (!inode) {
f45498
+        fuse_reply_err(req, EBADF);
f45498
+        return;
f45498
+    }
f45498
 
f45498
-        fd = openat(lo->proc_self_fd, buf, O_RDWR);
f45498
-        free(buf);
f45498
-        if (fd == -1) {
f45498
-            return (void)fuse_reply_err(req, errno);
f45498
+    if (!fi) {
f45498
+        fd = lo_inode_open(lo, inode, O_RDWR);
f45498
+        if (fd < 0) {
f45498
+            res = -fd;
f45498
+            goto out;
f45498
         }
f45498
     } else {
f45498
         fd = lo_fi_fd(req, fi);
f45498
     }
f45498
 
f45498
     if (datasync) {
f45498
-        res = fdatasync(fd);
f45498
+        res = fdatasync(fd) == -1 ? errno : 0;
f45498
     } else {
f45498
-        res = fsync(fd);
f45498
+        res = fsync(fd) == -1 ? errno : 0;
f45498
     }
f45498
     if (!fi) {
f45498
         close(fd);
f45498
     }
f45498
-    fuse_reply_err(req, res == -1 ? errno : 0);
f45498
+out:
f45498
+    lo_inode_put(lo, &inode;;
f45498
+    fuse_reply_err(req, res);
f45498
 }
f45498
 
f45498
 static void lo_read(fuse_req_t req, fuse_ino_t ino, size_t size, off_t offset,
f45498
-- 
f45498
2.18.2
f45498