Re: [PATCH-for-5.0] qga-posix: Avoid crashing process when failing to allocate memory

[Daniel P . Berrangé]

On Tue, Mar 24, 2020 at 08:48:36PM +0100, Philippe Mathieu-Daudé wrote:
> Similarly to commit 807e2b6fce0 for Windows, kindly return a
> QMP error message instead of crashing the whole process.
> 
> Cc: address@hidden
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1594054

I find that bug report pretty dubious

   "The QEMU Guest Agent in QEMU is vulnerable to an integer 
    overflow in the qmp_guest_file_read(). An attacker could 
    exploit this by sending a crafted QMP command (including 
    guest-file-read with a large count value) to the agent via 
    the listening socket to trigger a g_malloc() call with a 
    large memory chunk resulting in a segmentation fault."

"the attacker" in this case has to have access to the QEMU
chardev associated with the guest agent. IOW, in any sensible
configuration of the chardev, "the attacker" is the same person
who launched QEMU in the first place.  There's no elevation of
privilege here and any denial of service is inflicted against
themselves. Finally it doesn't cause a segmentation fault,
it causes an abort.

This is *NOT* a security bug.

In fact I'd call this NOTABUG entirely. It is just user error
to set the "count" to such a huge value that it hits OOM.

> Reported-by: Fakhri Zulkifli <address@hidden>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
>  qga/commands-posix.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 93474ff770..8f127788e6 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -493,7 +493,13 @@ struct GuestFileRead *qmp_guest_file_read(int64_t 
> handle, bool has_count,
>          gfh->state = RW_STATE_NEW;
>      }
>  
> -    buf = g_malloc0(count+1);
> +    buf = g_try_malloc0(count + 1);
> +    if (!buf) {
> +        error_setg(errp,
> +                   "failed to allocate sufficient memory "
> +                   "to complete the requested service");
> +        return NULL;
> +    }

So you've prevented an OOM when we call fread() on the file.

The contents of 'buf' now need to be turned into JSON which
means the buffer need to be base64 encoded. This will consume
even more memory than the original read.  So checking malloc
here has achieved nothing AFAICT.

>      read_count = fread(buf, 1, count, fh);
>      if (ferror(fh)) {
>          error_setg_errno(errp, errno, "failed to read file");

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|