From 6b831d7313fb3c4d4c452ec9dfed5afc1a1b85bf Mon Sep 17 00:00:00 2001
Message-Id: <6b831d7313fb3c4d4c452ec9dfed5afc1a1b85bf.1346840133.git.minovotn@redhat.com>
In-Reply-To: <2a711d63f161f5fa916f886f6b7b78ff366f9d0c.1346840133.git.minovotn@redhat.com>
References: <2a711d63f161f5fa916f886f6b7b78ff366f9d0c.1346840133.git.minovotn@redhat.com>
From: Jim Meyering <jim@meyering.net>
Date: Tue, 4 Sep 2012 09:22:31 -0400
Subject: [PATCH 4/4] 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>
(cherry picked from commit c2d76497b6eafcaedc806e07804e7bed55a98a0b)

Conflicts:
	block.c
	block/vvfat.c
	block_int.h

v2 adds missing comments header, removes blank line [Markus]
Bugzilla: 825687
Brew: http://brewweb.devel.redhat.com/brew/taskinfo?taskID=4830039
Signed-off-by: Michal Novotny <minovotn@redhat.com>
---
 qemu/block-vvfat.c |  8 +++++++-
 qemu/block.c       | 37 ++++++++++++++++++++++++-------------
 qemu/block_int.h   |  2 +-
 3 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/qemu/block-vvfat.c b/qemu/block-vvfat.c
index 9c629f7..4b20c94 100644
--- a/qemu/block-vvfat.c
+++ b/qemu/block-vvfat.c
@@ -2769,13 +2769,19 @@ static BlockDriver vvfat_write_target = {
 
 static int enable_write_target(BDRVVVFATState *s)
 {
+    int ret;
     int size = sector2cluster(s, s->sector_count);
     s->used_clusters = calloc(size, 1);
 
     array_init(&(s->commits), sizeof(commit_t));
 
     s->qcow_filename = malloc(1024);
-    get_tmp_filename(s->qcow_filename, 1024);
+    ret = get_tmp_filename(s->qcow_filename, 1024);
+    if (ret < 0) {
+        free(s->qcow_filename);
+        s->qcow_filename = NULL;
+        return ret;
+    }
     if (bdrv_create(&bdrv_qcow,
 		s->qcow_filename, s->sector_count, "fat:", 0) < 0)
 	return -1;
diff --git a/qemu/block.c b/qemu/block.c
index 7525fb6..b5f4036 100644
--- a/qemu/block.c
+++ b/qemu/block.c
@@ -227,28 +227,36 @@ int bdrv_create(BlockDriver *drv,
     return drv->bdrv_create(filename, size_in_sectors, backing_file, flags);
 }
 
-#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
+}
 
 #ifdef _WIN32
 static int is_windows_drive_prefix(const char *filename)
@@ -445,7 +453,10 @@ int bdrv_open2(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/qemu/block_int.h b/qemu/block_int.h
index bf21589..52205e5 100644
--- a/qemu/block_int.h
+++ b/qemu/block_int.h
@@ -173,7 +173,7 @@ struct BlockDriverAIOCB {
     BlockDriverAIOCB *next;
 };
 
-void get_tmp_filename(char *filename, int size);
+int get_tmp_filename(char *filename, int size);
 
 void *qemu_aio_get(BlockDriverState *bs, BlockDriverCompletionFunc *cb,
                    void *opaque);
-- 
1.7.11.4