[PATCH-for-5.0,v2,4/4] qga: Restrict guest-file-read count to 10 MB to avoid crashes

On https://www.mail-archive.com/qemu-devel@nongnu.org/msg693176.html
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, 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 10 MB.

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       | 6 +++++-
 2 files changed, 9 insertions(+), 3 deletions(-)

On Tue, Mar 31, 2020 at 04:06:38PM +0200, Philippe Mathieu-Daudé wrote:
> On https://www.mail-archive.com/qemu-devel@nongnu.org/msg693176.html
> 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, 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 10 MB.
> 
> 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       | 6 +++++-
>  2 files changed, 9 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.
>  #
>  # @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 8ee1244ebb..c130d1b0f5 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"
> @@ -18,11 +19,14 @@
>  #include "qemu/base64.h"
>  #include "qemu/cutils.h"
>  #include "qemu/atomic.h"
> +#include "commands-common.h"

This needs to be in the previous patch AFAICT.


Regards,
Daniel

On 3/31/20 4:15 PM, Daniel P. Berrangé wrote:
> On Tue, Mar 31, 2020 at 04:06:38PM +0200, Philippe Mathieu-Daudé wrote:
>> On https://www.mail-archive.com/qemu-devel@nongnu.org/msg693176.html
>> 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, 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 10 MB.
>>
>> 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       | 6 +++++-
>>   2 files changed, 9 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.
>>   #
>>   # @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 8ee1244ebb..c130d1b0f5 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"
>> @@ -18,11 +19,14 @@
>>   #include "qemu/base64.h"
>>   #include "qemu/cutils.h"
>>   #include "qemu/atomic.h"
>> +#include "commands-common.h"
> 
> This needs to be in the previous patch AFAICT.

Oops, thanks :)

> 
> 
> Regards,
> Daniel
>

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On https://www.mail-archive.com/qemu-devel@nongnu.org/msg693176.html
> 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, 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 10 MB.
>
> 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       | 6 +++++-
>  2 files changed, 9 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.
>  #
>  # @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 8ee1244ebb..c130d1b0f5 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"
> @@ -18,11 +19,14 @@
>  #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)
>  /* 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 - 10MB */
> +#define GUEST_FILE_READ_COUNT_MAX (10 * MiB)
>  
>  /* Note: in some situations, like with the fsfreeze, logging may be
>   * temporarilly disabled. if it is necessary that a command be able
> @@ -559,7 +563,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;

What about qmp-guest-file-write?

Hmm, the JSON parser already puts a limit on the base-64 encoded data,
namely MAX_TOKEN_SIZE, which is 64MiB.  Yes, MAX_TOKEN_SIZE is
ridiculously generous.

In case you look at the code: there are *two* MAX_TOKEN_SIZE, both
64MiB.  One actually applies to tokens, the other to all the tokens in a
top-level expression.  Yes, this is criminally confusing.

We additionally limit the number of tokens within a top-level
expression, and its nesting depth.

The stated reason for these limits is

    /*
     * Security consideration, we limit total memory allocated per object
     * and the maximum recursion depth that a message can force.
     */

"Security" is misleading; we use this parser only for parsing QMP input,
QGA input, and string literals.  All trusted.  It's actually a
robustness consideration.

We fail to put similar limits on JSON output.

While we protect you from the rather far-fetched mistake of driving QEMU
into OOM state by sending ridiculously huge QMP/QGA input, we do nothing
to protect you from the more realistic mistake of sending a QMP/QGA
command that produces ridiculously huge output.

Could use a rethink, I guess :)

On Thu, Apr 02, 2020 at 03:09:55PM +0200, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
> > On https://www.mail-archive.com/qemu-devel@nongnu.org/msg693176.html
> > 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, 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 10 MB.
> >
> > 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       | 6 +++++-
> >  2 files changed, 9 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.
> >  #
> >  # @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 8ee1244ebb..c130d1b0f5 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"
> > @@ -18,11 +19,14 @@
> >  #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)
> >  /* 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 - 10MB */
> > +#define GUEST_FILE_READ_COUNT_MAX (10 * MiB)
> >  
> >  /* Note: in some situations, like with the fsfreeze, logging may be
> >   * temporarilly disabled. if it is necessary that a command be able
> > @@ -559,7 +563,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;
> 
> What about qmp-guest-file-write?
> 
> Hmm, the JSON parser already puts a limit on the base-64 encoded data,
> namely MAX_TOKEN_SIZE, which is 64MiB.  Yes, MAX_TOKEN_SIZE is
> ridiculously generous.
> 
> In case you look at the code: there are *two* MAX_TOKEN_SIZE, both
> 64MiB.  One actually applies to tokens, the other to all the tokens in a
> top-level expression.  Yes, this is criminally confusing.

Oh fun :-(

Anyway, if the JSON parser has 64 MB limits in various places, then
I'd be inclined to set the QGA guest-file-read limit such that the
encoded JSON is at approx the 64 MB limit too so we're somewhat
consistent in limits.

Base64 has a 3:4 overhead, so that would suggest setting guest-file-read
to be 48 MB limit.

Regards,
Daniel

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Apr 02, 2020 at 03:09:55PM +0200, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>> 
>> > On https://www.mail-archive.com/qemu-devel@nongnu.org/msg693176.html
>> > 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, 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 10 MB.
>> >
>> > 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       | 6 +++++-
>> >  2 files changed, 9 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.
>> >  #
>> >  # @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 8ee1244ebb..c130d1b0f5 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"
>> > @@ -18,11 +19,14 @@
>> >  #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)
>> >  /* 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 - 10MB */
>> > +#define GUEST_FILE_READ_COUNT_MAX (10 * MiB)
>> >  
>> >  /* Note: in some situations, like with the fsfreeze, logging may be
>> >   * temporarilly disabled. if it is necessary that a command be able
>> > @@ -559,7 +563,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;
>> 
>> What about qmp-guest-file-write?
>> 
>> Hmm, the JSON parser already puts a limit on the base-64 encoded data,
>> namely MAX_TOKEN_SIZE, which is 64MiB.  Yes, MAX_TOKEN_SIZE is
>> ridiculously generous.
>> 
>> In case you look at the code: there are *two* MAX_TOKEN_SIZE, both
>> 64MiB.  One actually applies to tokens, the other to all the tokens in a
>> top-level expression.  Yes, this is criminally confusing.
>
> Oh fun :-(
>
> Anyway, if the JSON parser has 64 MB limits in various places, then
> I'd be inclined to set the QGA guest-file-read limit such that the
> encoded JSON is at approx the 64 MB limit too so we're somewhat
> consistent in limits.
>
> Base64 has a 3:4 overhead, so that would suggest setting guest-file-read
> to be 48 MB limit.

If a command imposes its own limit in addition to the JSON parser
limits, it should be lower, or else it's dead code.

Parse errors can be nasty.  Could be improved, but doesn't seem to be
worthwhile.  Commands can do better easily.

The parser's limits could theoretically change.  I consider them overly
generous for control plane.