| Message ID | 20200414133052.13712-5-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | various bugfixes | expand |
On Tue, Apr 14, 2020 at 03:30:44PM +0200, Philippe Mathieu-Daudé wrote: > 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> > --- > qga/qapi-schema.json | 6 ++++-- > qga/commands.c | 9 ++++++++- > 2 files changed, 12 insertions(+), 3 deletions(-) > > diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json > index f6fcb59f34..7758d9daf8 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 10 MB. s/10/48/ > # > # @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 10MB) s/10/48/ > # > # Returns: @GuestFileRead on success. > # > 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; With the docs typos fixed: Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel
On 4/15/20 2:34 PM, Daniel P. Berrangé wrote: > On Tue, Apr 14, 2020 at 03:30:44PM +0200, Philippe Mathieu-Daudé wrote: >> 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> >> --- >> qga/qapi-schema.json | 6 ++++-- >> qga/commands.c | 9 ++++++++- >> 2 files changed, 12 insertions(+), 3 deletions(-) >> >> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json >> index f6fcb59f34..7758d9daf8 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 10 MB. > > s/10/48/ > >> # >> # @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 10MB) > > s/10/48/ Oops I totally missed those, thanks! > >> # >> # Returns: @GuestFileRead on success. >> # >> 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; > > With the docs typos fixed: > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > > Regards, > Daniel >
Quoting Philippe Mathieu-Daudé (2020-04-15 08:02:18) > On 4/15/20 2:34 PM, Daniel P. Berrangé wrote: > > On Tue, Apr 14, 2020 at 03:30:44PM +0200, Philippe Mathieu-Daudé wrote: > >> 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> > >> --- > >> qga/qapi-schema.json | 6 ++++-- > >> qga/commands.c | 9 ++++++++- > >> 2 files changed, 12 insertions(+), 3 deletions(-) > >> > >> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json > >> index f6fcb59f34..7758d9daf8 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 10 MB. > > > > s/10/48/ > > > >> # > >> # @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 10MB) > > > > s/10/48/ > > Oops I totally missed those, thanks! I've rolled in the doc fix-ups and sent a pull request for patches 1-4 > > > > >> # > >> # Returns: @GuestFileRead on success. > >> # > >> 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; > > > > With the docs typos fixed: > > > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > > > > > Regards, > > Daniel > > >
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json index f6fcb59f34..7758d9daf8 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 10 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 10MB) # # Returns: @GuestFileRead on success. # 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;
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> --- qga/qapi-schema.json | 6 ++++-- qga/commands.c | 9 ++++++++- 2 files changed, 12 insertions(+), 3 deletions(-)