From f62ebb63848107336e57adc12369aefaa639e38c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 14 Apr 2020 15:30:41 +0200 Subject: [PATCH 1/4] Revert "prevent crash when executing guest-file-read with large count" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As noted by Daniel Berrangé in [*], the fix from commit 807e2b6fce which replaced malloc() by try_malloc() is not enough, the process can still run out of memory a few line later: 346 buf = g_try_malloc0(count + 1); 347 if (!buf) { 348 error_setg(errp, 349 "failed to allocate sufficient memory " 350 "to complete the requested service"); 351 return NULL; 352 } 353 is_ok = ReadFile(fh, buf, count, &read_count, NULL); 354 if (!is_ok) { 355 error_setg_win32(errp, GetLastError(), "failed to read file"); 356 slog("guest-file-read failed, handle %" PRId64, handle); 357 } else { 358 buf[read_count] = 0; 359 read_data = g_new0(GuestFileRead, 1); ^^^^^^ Instead we are going to put a low hard limit on 'count' in the next commits. This reverts commit 807e2b6fce022707418bc8f61c069d91c613b3d2. [*] https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg03471.html Suggested-by: Daniel P. Berrangé Signed-off-by: Philippe Mathieu-Daudé Signed-off-by: Michael Roth --- qga/commands-win32.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/qga/commands-win32.c b/qga/commands-win32.c index b49920e201..46cea7d1d9 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -343,13 +343,7 @@ GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count, } fh = gfh->fh; - buf = g_try_malloc0(count + 1); - if (!buf) { - error_setg(errp, - "failed to allocate sufficient memory " - "to complete the requested service"); - return NULL; - } + buf = g_malloc0(count + 1); is_ok = ReadFile(fh, buf, count, &read_count, NULL); if (!is_ok) { error_setg_win32(errp, GetLastError(), "failed to read file"); From 5d3586b834633c8ac462d4741b85b4036cbc0f93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 14 Apr 2020 15:30:42 +0200 Subject: [PATCH 2/4] qga: Extract guest_file_handle_find() to commands-common.h MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As we are going to reuse this method, declare it in common header. Signed-off-by: Philippe Mathieu-Daudé Signed-off-by: Michael Roth --- qga/commands-common.h | 18 ++++++++++++++++++ qga/commands-posix.c | 7 ++++--- qga/commands-win32.c | 7 ++++--- 3 files changed, 26 insertions(+), 6 deletions(-) create mode 100644 qga/commands-common.h diff --git a/qga/commands-common.h b/qga/commands-common.h new file mode 100644 index 0000000000..af90e5481e --- /dev/null +++ b/qga/commands-common.h @@ -0,0 +1,18 @@ +/* + * QEMU Guest Agent common/cross-platform common commands + * + * Copyright (c) 2020 Red Hat, Inc. + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + */ +#ifndef QGA_COMMANDS_COMMON_H +#define QGA_COMMANDS_COMMON_H + +#include "qga-qapi-types.h" + +typedef struct GuestFileHandle GuestFileHandle; + +GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp); + +#endif diff --git a/qga/commands-posix.c b/qga/commands-posix.c index cc69b82704..c59c32185c 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -26,6 +26,7 @@ #include "qemu/sockets.h" #include "qemu/base64.h" #include "qemu/cutils.h" +#include "commands-common.h" #ifdef HAVE_UTMPX #include @@ -237,12 +238,12 @@ typedef enum { RW_STATE_WRITING, } RwState; -typedef struct GuestFileHandle { +struct GuestFileHandle { uint64_t id; FILE *fh; RwState state; QTAILQ_ENTRY(GuestFileHandle) next; -} GuestFileHandle; +}; static struct { QTAILQ_HEAD(, GuestFileHandle) filehandles; @@ -268,7 +269,7 @@ static int64_t guest_file_handle_add(FILE *fh, Error **errp) return handle; } -static GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp) +GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp) { GuestFileHandle *gfh; diff --git a/qga/commands-win32.c b/qga/commands-win32.c index 46cea7d1d9..cfaf6b84b8 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -37,6 +37,7 @@ #include "qemu/queue.h" #include "qemu/host-utils.h" #include "qemu/base64.h" +#include "commands-common.h" #ifndef SHTDN_REASON_FLAG_PLANNED #define SHTDN_REASON_FLAG_PLANNED 0x80000000 @@ -50,11 +51,11 @@ #define INVALID_SET_FILE_POINTER ((DWORD)-1) -typedef struct GuestFileHandle { +struct GuestFileHandle { int64_t id; HANDLE fh; QTAILQ_ENTRY(GuestFileHandle) next; -} GuestFileHandle; +}; static struct { QTAILQ_HEAD(, GuestFileHandle) filehandles; @@ -126,7 +127,7 @@ static int64_t guest_file_handle_add(HANDLE fh, Error **errp) return handle; } -static GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp) +GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp) { GuestFileHandle *gfh; QTAILQ_FOREACH(gfh, &guest_file_state.filehandles, next) { From ead83a136d54f7faa315922aff26fa11d216909f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 14 Apr 2020 15:30:43 +0200 Subject: [PATCH 3/4] qga: Extract qmp_guest_file_read() to common commands.c MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract the common code shared by both POSIX/Win32 implementations. Signed-off-by: Philippe Mathieu-Daudé Signed-off-by: Michael Roth --- qga/commands-common.h | 3 +++ qga/commands-posix.c | 22 +++------------------- qga/commands-win32.c | 20 +++----------------- qga/commands.c | 26 ++++++++++++++++++++++++++ 4 files changed, 35 insertions(+), 36 deletions(-) diff --git a/qga/commands-common.h b/qga/commands-common.h index af90e5481e..90785ed4bb 100644 --- a/qga/commands-common.h +++ b/qga/commands-common.h @@ -15,4 +15,7 @@ typedef struct GuestFileHandle GuestFileHandle; GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp); +GuestFileRead *guest_file_read_unsafe(GuestFileHandle *gfh, + int64_t count, Error **errp); + #endif diff --git a/qga/commands-posix.c b/qga/commands-posix.c index c59c32185c..a52af0315f 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -461,29 +461,14 @@ void qmp_guest_file_close(int64_t handle, Error **errp) g_free(gfh); } -struct GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count, - int64_t count, Error **errp) +GuestFileRead *guest_file_read_unsafe(GuestFileHandle *gfh, + int64_t count, Error **errp) { - GuestFileHandle *gfh = guest_file_handle_find(handle, errp); GuestFileRead *read_data = NULL; guchar *buf; - FILE *fh; + FILE *fh = gfh->fh; size_t read_count; - if (!gfh) { - return NULL; - } - - if (!has_count) { - count = QGA_READ_COUNT_DEFAULT; - } else if (count < 0 || count >= UINT32_MAX) { - error_setg(errp, "value '%" PRId64 "' is invalid for argument count", - count); - return NULL; - } - - fh = gfh->fh; - /* explicitly flush when switching from writing to reading */ if (gfh->state == RW_STATE_WRITING) { int ret = fflush(fh); @@ -498,7 +483,6 @@ struct GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count, read_count = fread(buf, 1, count, fh); if (ferror(fh)) { error_setg_errno(errp, errno, "failed to read file"); - slog("guest-file-read failed, handle: %" PRId64, handle); } else { buf[read_count] = 0; read_data = g_new0(GuestFileRead, 1); diff --git a/qga/commands-win32.c b/qga/commands-win32.c index cfaf6b84b8..9717a8d52d 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -322,33 +322,19 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **errp) } } -GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count, - int64_t count, Error **errp) +GuestFileRead *guest_file_read_unsafe(GuestFileHandle *gfh, + int64_t count, Error **errp) { GuestFileRead *read_data = NULL; guchar *buf; - HANDLE fh; + HANDLE fh = gfh->fh; bool is_ok; DWORD read_count; - GuestFileHandle *gfh = guest_file_handle_find(handle, errp); - if (!gfh) { - return NULL; - } - if (!has_count) { - count = QGA_READ_COUNT_DEFAULT; - } else if (count < 0 || count >= UINT32_MAX) { - error_setg(errp, "value '%" PRId64 - "' is invalid for argument count", count); - return NULL; - } - - fh = gfh->fh; buf = g_malloc0(count + 1); is_ok = ReadFile(fh, buf, count, &read_count, NULL); if (!is_ok) { error_setg_win32(errp, GetLastError(), "failed to read file"); - slog("guest-file-read failed, handle %" PRId64, handle); } else { buf[read_count] = 0; read_data = g_new0(GuestFileRead, 1); diff --git a/qga/commands.c b/qga/commands.c index 4471a9f08d..5611117372 100644 --- a/qga/commands.c +++ b/qga/commands.c @@ -18,6 +18,7 @@ #include "qemu/base64.h" #include "qemu/cutils.h" #include "qemu/atomic.h" +#include "commands-common.h" /* Maximum captured guest-exec out_data/err_data - 16MB */ #define GUEST_EXEC_MAX_OUTPUT (16*1024*1024) @@ -547,3 +548,28 @@ error: g_free(info); return NULL; } + +GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count, + int64_t count, Error **errp) +{ + GuestFileHandle *gfh = guest_file_handle_find(handle, errp); + GuestFileRead *read_data; + + if (!gfh) { + return NULL; + } + if (!has_count) { + count = QGA_READ_COUNT_DEFAULT; + } else if (count < 0 || count >= UINT32_MAX) { + error_setg(errp, "value '%" PRId64 "' is invalid for argument count", + count); + return NULL; + } + + read_data = guest_file_read_unsafe(gfh, count, errp); + if (!read_data) { + slog("guest-file-write failed, handle: %" PRId64, handle); + } + + return read_data; +} From 1329651fb4d4c5068ad12fd86aff7e52f9e18c34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 14 Apr 2020 15:30:44 +0200 Subject: [PATCH 4/4] qga: Restrict guest-file-read count to 48 MB to avoid crashes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On [*] Daniel Berrangé commented: The QEMU guest agent protocol is not sensible way to access huge files inside the guest. It requires the inefficient process of reading the entire data into memory than duplicating it again in base64 format, and then copying it again in the JSON serializer / monitor code. For arbitrary general purpose file access, especially for large files, use a real file transfer program or use a network block device, not the QEMU guest agent. To avoid bug reports as BZ#1594054 (CVE-2018-12617), follow his suggestion to put a low, hard limit on "count" in the guest agent QAPI schema, and don't allow count to be larger than 48 MB. [*] https://www.mail-archive.com/qemu-devel@nongnu.org/msg693176.html Fixes: CVE-2018-12617 Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1594054 Reported-by: Fakhri Zulkifli Suggested-by: Daniel P. Berrangé Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Daniel P. Berrangé *update schema documentation to indicate 48MB limit instead of 10MB Signed-off-by: Michael Roth --- qga/commands.c | 9 ++++++++- qga/qapi-schema.json | 6 ++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/qga/commands.c b/qga/commands.c index 5611117372..efc8b90281 100644 --- a/qga/commands.c +++ b/qga/commands.c @@ -11,6 +11,7 @@ */ #include "qemu/osdep.h" +#include "qemu/units.h" #include "guest-agent-core.h" #include "qga-qapi-commands.h" #include "qapi/error.h" @@ -24,6 +25,12 @@ #define GUEST_EXEC_MAX_OUTPUT (16*1024*1024) /* Allocation and I/O buffer for reading guest-exec out_data/err_data - 4KB */ #define GUEST_EXEC_IO_SIZE (4*1024) +/* + * Maximum file size to read - 48MB + * + * (48MB + Base64 3:4 overhead = JSON parser 64 MB limit) + */ +#define GUEST_FILE_READ_COUNT_MAX (48 * MiB) /* Note: in some situations, like with the fsfreeze, logging may be * temporarilly disabled. if it is necessary that a command be able @@ -560,7 +567,7 @@ GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count, } if (!has_count) { count = QGA_READ_COUNT_DEFAULT; - } else if (count < 0 || count >= UINT32_MAX) { + } else if (count < 0 || count > GUEST_FILE_READ_COUNT_MAX) { error_setg(errp, "value '%" PRId64 "' is invalid for argument count", count); return NULL; diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json index f6fcb59f34..4be9aad48e 100644 --- a/qga/qapi-schema.json +++ b/qga/qapi-schema.json @@ -266,11 +266,13 @@ ## # @guest-file-read: # -# Read from an open file in the guest. Data will be base64-encoded +# Read from an open file in the guest. Data will be base64-encoded. +# As this command is just for limited, ad-hoc debugging, such as log +# file access, the number of bytes to read is limited to 48 MB. # # @handle: filehandle returned by guest-file-open # -# @count: maximum number of bytes to read (default is 4KB) +# @count: maximum number of bytes to read (default is 4KB, maximum is 48MB) # # Returns: @GuestFileRead on success. #