diff mbox series

[PULL,for-5.0,4/4] qga: Restrict guest-file-read count to 48 MB to avoid crashes

Message ID 20200415152202.14463-5-mdroth@linux.vnet.ibm.com
State New
Headers show
Series [PULL,for-5.0,1/4] Revert "prevent crash when executing guest-file-read with large count" | expand

Commit Message

Michael Roth April 15, 2020, 3:22 p.m. UTC 
From: Philippe Mathieu-Daudé <philmd@redhat.com>

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 <mohdfakhrizulkifli@gmail.com>
Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
*update schema documentation to indicate 48MB limit instead of 10MB
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/commands.c       | 9 ++++++++-
 qga/qapi-schema.json | 6 ++++--
 2 files changed, 12 insertions(+), 3 deletions(-)
diff mbox series

Patch

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.
 #