From d9058294051f8fe83a6f8b39f88c98b23b8400cd Mon Sep 17 00:00:00 2001 Message-Id: In-Reply-To: References: From: Jim Meyering Date: Wed, 22 Aug 2012 15:56:37 -0400 Subject: [PATCH 10/10] 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 Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf (cherry picked from commit c2d76497b6eafcaedc806e07804e7bed55a98a0b) Conflicts: block_int.h Signed-off-by: Jeff Cody Signed-off-by: Michal Novotny --- block.c | 37 ++++++++++++++++++++++++------------- block/vvfat.c | 7 ++++++- block_int.h | 2 +- 3 files changed, 31 insertions(+), 15 deletions(-) diff --git a/block.c b/block.c index 02b61fa..aa5f011 100644 --- a/block.c +++ b/block.c @@ -276,28 +276,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 @@ -602,7 +610,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 cc83b1a..f3abd1b 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -2809,7 +2809,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 8fae15e..b986599 100644 --- a/block_int.h +++ b/block_int.h @@ -294,7 +294,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(AIOPool *pool, BlockDriverState *bs, BlockDriverCompletionFunc *cb, void *opaque); -- 1.7.11.4