xorl %eax, %eax

CVE-2013-2007: QEMU Guest Agent Insecure File Permissions

leave a comment »

This vulnerability was reported by Laszlo Ersek of Red Hat and it allows guest privilege escalation when started in daemon mode. As he mentioned, QEMU guest agent creates files with incorrect file permissions. Specifically, at least the files that are affected are the ones created with ‘guest-file-open’ QMP command, shell output redirection, or when invoked by the fsfreeze script.

To overcome this he first updated the umask(2) of become_daemon() routine in qga/main.c as shown below.

     }
 
-    umask(0);
+    umask(S_IRWXG | S_IRWXO);
     sid = setsid();

The find_open_flag() and safe_open_or_create() functions were added in qga/commands-posix.c as you can see here.

+typedef const char * const ccpc;
+
+/* http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html */
+static const struct {
+    ccpc *forms;
+    int oflag_base;
+} guest_file_open_modes[] = {
+    { (ccpc[]){ "r",  "rb",         NULL }, O_RDONLY                      },
+    { (ccpc[]){ "w",  "wb",         NULL }, O_WRONLY | O_CREAT | O_TRUNC  },
+    { (ccpc[]){ "a",  "ab",         NULL }, O_WRONLY | O_CREAT | O_APPEND },
+    { (ccpc[]){ "r+", "rb+", "r+b", NULL }, O_RDWR                        },
+    { (ccpc[]){ "w+", "wb+", "w+b", NULL }, O_RDWR   | O_CREAT | O_TRUNC  },
+    { (ccpc[]){ "a+", "ab+", "a+b", NULL }, O_RDWR   | O_CREAT | O_APPEND }
+};
+
+static int
+find_open_flag(const char *mode_str, Error **err)
+{
+    unsigned mode;
+
+    for (mode = 0; mode < ARRAY_SIZE(guest_file_open_modes); ++mode) {
+        ccpc *form;
+
+        form = guest_file_open_modes[mode].forms;
+        while (*form != NULL && strcmp(*form, mode_str) != 0) {
+            ++form;
+        }
+        if (*form != NULL) {
+            break;
+        }
+    }
+
+    if (mode == ARRAY_SIZE(guest_file_open_modes)) {
+        error_setg(err, "invalid file open mode '%s'", mode_str);
+        return -1;
+    }
+    return guest_file_open_modes[mode].oflag_base | O_NOCTTY | O_NONBLOCK;
+}
+
+#define DEFAULT_NEW_FILE_MODE (S_IRUSR | S_IWUSR | \
+                               S_IRGRP | S_IWGRP | \
+                               S_IROTH | S_IWOTH)
+
+static FILE *
+safe_open_or_create(const char *path, const char *mode, Error **err)
+{
+    Error *local_err = NULL;
+    int oflag;
+
+    oflag = find_open_flag(mode, &local_err);
+    if (local_err == NULL) {
+        int fd;
+
+        /* If the caller wants / allows creation of a new file, we implement it
+         * with a two step process: open() + (open() / fchmod()).
+         *
+         * First we insist on creating the file exclusively as a new file. If
+         * that succeeds, we're free to set any file-mode bits on it. (The
+         * motivation is that we want to set those file-mode bits independently
+         * of the current umask.)
+         *
+         * If the exclusive creation fails because the file already exists
+         * (EEXIST is not possible for any other reason), we just attempt to
+         * open the file, but in this case we won't be allowed to change the
+         * file-mode bits on the preexistent file.
+         *
+         * The pathname should never disappear between the two open()s in
+         * practice. If it happens, then someone very likely tried to race us.
+         * In this case just go ahead and report the ENOENT from the second
+         * open() to the caller.
+         *
+         * If the caller wants to open a preexistent file, then the first
+         * open() is decisive and its third argument is ignored, and the second
+         * open() and the fchmod() are never called.
+         */
+        fd = open(path, oflag | ((oflag & O_CREAT) ? O_EXCL : 0), 0);
+        if (fd == -1 && errno == EEXIST) {
+            oflag &= ~(unsigned)O_CREAT;
+            fd = open(path, oflag);
+        }
+
+        if (fd == -1) {
+            error_setg_errno(&local_err, errno, "failed to open file '%s' "
+                             "(mode: '%s')", path, mode);
+        } else {
+            qemu_set_cloexec(fd);
+
+            if ((oflag & O_CREAT) && fchmod(fd, DEFAULT_NEW_FILE_MODE) == -1) {
+                error_setg_errno(&local_err, errno, "failed to set permission "
+                                 "0%03o on new file '%s' (mode: '%s')",
+                                 (unsigned)DEFAULT_NEW_FILE_MODE, path, mode);
+            } else {
+                FILE *f;
+
+                f = fdopen(fd, mode);
+                if (f == NULL) {
+                    error_setg_errno(&local_err, errno, "failed to associate "
+                                     "stdio stream with file descriptor %d, "
+                                     "file '%s' (mode: '%s')", fd, path, mode);
+                } else {
+                    return f;
+                }
+            }
+
+            close(fd);
+        }
+    }
+
+    error_propagate(err, local_err);
+    return NULL;
+}
+
 int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode, Error **err)
 {
     FILE *fh;
+    Error *local_err = NULL;
     int fd;
     int64_t ret = -1, handle;

And qmp_guest_file_open() was updated to use the newly added safe_open_or_create() routine shown above.

     slog("guest-file-open called, filepath: %s, mode: %s", path, mode);
-    fh = fopen(path, mode);
-    if (!fh) {
-        error_setg_errno(err, errno, "failed to open file '%s' (mode: '%s')",
-                         path, mode);
+    fh = safe_open_or_create(path, mode, &local_err);
+    if (local_err != NULL) {
+        error_propagate(err, local_err);
         return -1;
     }

Written by xorl

May 26, 2013 at 14:14

Posted in bugs

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s