block: prevent snapshot mode $TMPDIR symlink attack

In snapshot mode, bdrv_open creates an empty temporary file without
checking for mkstemp or close failure, and ignoring the possibility
of a buffer overrun given a surprisingly long $TMPDIR.
Change the get_tmp_filename function to return int (not void),
so that it can inform its two callers of those failures.
Also avoid the risk of buffer overrun and do not ignore mkstemp
or close failure.
Update both callers (in block.c and vvfat.c) to propagate
temp-file-creation failure to their callers.

get_tmp_filename creates and closes an empty file, while its
callers later open that presumed-existing file with O_CREAT.
The problem was that a malicious user could provoke mkstemp failure
and race to create a symlink with the selected temporary file name,
thus causing the qemu process (usually root owned) to open through
the symlink, overwriting an attacker-chosen file.

This addresses CVE-2012-2652.
http://bugzilla.redhat.com/CVE-2012-2652

Signed-off-by: Jim Meyering <meyering@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>

diff --git a/block.c b/block.c
index af2ab4f3ea13e1e50f414b163fe7d5cae169b645..7547051ec2abd8cf84e495d097a83358bc9cc49e 100644 (file)
--- a/block.c
+++ b/block.c
@@ -409,28 +409,36 @@ int bdrv_create_file(const char* filename, QEMUOptionParameter *options)
     return bdrv_create(drv, filename, options);
 }
 
-#ifdef _WIN32
-void get_tmp_filename(char *filename, int size)
+/*
+ * Create a uniquely-named empty temporary file.
+ * Return 0 upon success, otherwise a negative errno value.
+ */
+int get_tmp_filename(char *filename, int size)
 {
+#ifdef _WIN32
     char temp_dir[MAX_PATH];
-
-    GetTempPath(MAX_PATH, temp_dir);
-    GetTempFileName(temp_dir, "qem", 0, filename);
-}
+    /* GetTempFileName requires that its output buffer (4th param)
+       have length MAX_PATH or greater.  */
+    assert(size >= MAX_PATH);
+    return (GetTempPath(MAX_PATH, temp_dir)
+            && GetTempFileName(temp_dir, "qem", 0, filename)
+            ? 0 : -GetLastError());
 #else
-void get_tmp_filename(char *filename, int size)
-{
     int fd;
     const char *tmpdir;
-    /* XXX: race condition possible */
     tmpdir = getenv("TMPDIR");
     if (!tmpdir)
         tmpdir = "/tmp";
-    snprintf(filename, size, "%s/vl.XXXXXX", tmpdir);
+    if (snprintf(filename, size, "%s/vl.XXXXXX", tmpdir) >= size) {
+        return -EOVERFLOW;
+    }
     fd = mkstemp(filename);
-    close(fd);
-}
+    if (fd < 0 || close(fd)) {
+        return -errno;
+    }
+    return 0;
 #endif
+}
 
 /*
  * Detect host devices. By convention, /dev/cdrom[N] is always
@@ -753,7 +761,10 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
 
         bdrv_delete(bs1);
 
-        get_tmp_filename(tmp_filename, sizeof(tmp_filename));
+        ret = get_tmp_filename(tmp_filename, sizeof(tmp_filename));
+        if (ret < 0) {
+            return ret;
+        }
 
         /* Real path is meaningless for protocols */
         if (is_protocol)

diff --git a/block/vvfat.c b/block/vvfat.c
index 2dc9d50888585eb2978ed43322d2af88539f986d..0fd3367d820ae84e5d31879ea50dcb1ba00ea5b9 100644 (file)
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2808,7 +2808,12 @@ static int enable_write_target(BDRVVVFATState *s)
     array_init(&(s->commits), sizeof(commit_t));
 
     s->qcow_filename = g_malloc(1024);
-    get_tmp_filename(s->qcow_filename, 1024);
+    ret = get_tmp_filename(s->qcow_filename, 1024);
+    if (ret < 0) {
+        g_free(s->qcow_filename);
+        s->qcow_filename = NULL;
+        return ret;
+    }
 
     bdrv_qcow = bdrv_find_format("qcow");
     options = parse_option_parameters("", bdrv_qcow->create_options, NULL);

diff --git a/block_int.h b/block_int.h
index b80e66db6ef64e71f44156bc5fae1e00d5a39dab..3d4abc6575adbf324ebf48af9e71717bc5c9e65d 100644 (file)
--- a/block_int.h
+++ b/block_int.h
@@ -335,7 +335,7 @@ struct BlockDriverState {
     BlockJob *job;
 };
 
-void get_tmp_filename(char *filename, int size);
+int get_tmp_filename(char *filename, int size);
 
 void bdrv_set_io_limits(BlockDriverState *bs,
                         BlockIOLimit *io_limits);