| 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 |
On 4/15/20 5:22 PM, Michael Roth wrote: > 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 Thanks! > 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 --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. #