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

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

[Philippe Mathieu-Daudé]

Fakhri Zulkifli reported BZ#1594054
https://bugzilla.redhat.com/show_bug.cgi?id=1594054

Dietmar Maurer noticed the fix from 807e2b6fce0 doesn't help
as error_setg() also calls malloc().

Daniel Berrangé suggested a clever fix, restrict the
guest-agent command size to avoid heap mayhem.

v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg691773.html

Philippe Mathieu-Daudé (4):
  Revert "prevent crash when executing guest-file-read with large count"
  qga: Extract guest_file_handle_find() to commands-common.h
  qga: Extract qmp_guest_file_read() to common commands.c
  qga: Restrict guest-file-read count to 10 MB to avoid crashes

 qga/qapi-schema.json  |  6 ++++--
 qga/commands-common.h | 21 +++++++++++++++++++++
 qga/commands-posix.c  | 29 +++++++----------------------
 qga/commands-win32.c  | 35 ++++++++---------------------------
 qga/commands.c        | 29 +++++++++++++++++++++++++++++
 5 files changed, 69 insertions(+), 51 deletions(-)
 create mode 100644 qga/commands-common.h

-- 
2.21.1

[PATCH-for-5.0 v2 1/4] Revert "prevent crash when executing guest-file-read with large count"

[Philippe Mathieu-Daudé]

By using g_try_malloc() instead of g_malloc() the qemu-guest-agent
Denial-of-Service attack referred in commit 807e2b6fce is reduced,
but still triggerable:

  - bisect file size S until g_try_malloc(S) fails,
  - use S - 1:
    g_try_malloc(S - 1) succeeds, but g_new0() few lines later will
    fail.

 346     buf = g_try_malloc0(count + 1);
 347     if (!buf) {
 348         error_setg(errp,
 349                    "failed to allocate sufficient memory "
 350                    "to complete the requested service");
 351         return NULL;
 352     }
 353     is_ok = ReadFile(fh, buf, count, &read_count, NULL);
 354     if (!is_ok) {
 355         error_setg_win32(errp, GetLastError(), "failed to read file");
 356         slog("guest-file-read failed, handle %" PRId64, handle);
 357     } else {
 358         buf[read_count] = 0;
 359         read_data = g_new0(GuestFileRead, 1);
                         ^^^^^^

Instead we are going to put a low hard limit on 'count' in the next
commits.
This reverts commit 807e2b6fce022707418bc8f61c069d91c613b3d2.

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 qga/commands-win32.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index b49920e201..46cea7d1d9 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -343,13 +343,7 @@ GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
     }
 
     fh = gfh->fh;
-    buf = g_try_malloc0(count + 1);
-    if (!buf) {
-        error_setg(errp,
-                   "failed to allocate sufficient memory "
-                   "to complete the requested service");
-        return NULL;
-    }
+    buf = g_malloc0(count + 1);
     is_ok = ReadFile(fh, buf, count, &read_count, NULL);
     if (!is_ok) {
         error_setg_win32(errp, GetLastError(), "failed to read file");
-- 
2.21.1

Re: [PATCH-for-5.0 v2 1/4] Revert "prevent crash when executing guest-file-read with large count"

[Daniel P. Berrangé]

On Tue, Mar 31, 2020 at 04:06:35PM +0200, Philippe Mathieu-Daudé wrote:
> By using g_try_malloc() instead of g_malloc() the qemu-guest-agent
> Denial-of-Service attack referred in commit 807e2b6fce is reduced,
> but still triggerable:

As explained previously, I believe there is *no* denial of service
attack here. The described scenario is just a user hurting themselves
by intentionally telling QEMU not to limit the amount of data returned.

> 
>   - bisect file size S until g_try_malloc(S) fails,
>   - use S - 1:
>     g_try_malloc(S - 1) succeeds, but g_new0() few lines later will
>     fail.
> 
>  346     buf = g_try_malloc0(count + 1);
>  347     if (!buf) {
>  348         error_setg(errp,
>  349                    "failed to allocate sufficient memory "
>  350                    "to complete the requested service");
>  351         return NULL;
>  352     }
>  353     is_ok = ReadFile(fh, buf, count, &read_count, NULL);
>  354     if (!is_ok) {
>  355         error_setg_win32(errp, GetLastError(), "failed to read file");
>  356         slog("guest-file-read failed, handle %" PRId64, handle);
>  357     } else {
>  358         buf[read_count] = 0;
>  359         read_data = g_new0(GuestFileRead, 1);
>                          ^^^^^^
> 
> Instead we are going to put a low hard limit on 'count' in the next
> commits.
> This reverts commit 807e2b6fce022707418bc8f61c069d91c613b3d2.
> 
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  qga/commands-win32.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index b49920e201..46cea7d1d9 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -343,13 +343,7 @@ GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
>      }
>  
>      fh = gfh->fh;
> -    buf = g_try_malloc0(count + 1);
> -    if (!buf) {
> -        error_setg(errp,
> -                   "failed to allocate sufficient memory "
> -                   "to complete the requested service");
> -        return NULL;
> -    }
> +    buf = g_malloc0(count + 1);
>      is_ok = ReadFile(fh, buf, count, &read_count, NULL);
>      if (!is_ok) {
>          error_setg_win32(errp, GetLastError(), "failed to read file");
> -- 
> 2.21.1
> 

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 :|

Re: [PATCH-for-5.0 v2 1/4] Revert "prevent crash when executing guest-file-read with large count"

[Philippe Mathieu-Daudé]

On 3/31/20 4:12 PM, Daniel P. Berrangé wrote:
> On Tue, Mar 31, 2020 at 04:06:35PM +0200, Philippe Mathieu-Daudé wrote:
>> By using g_try_malloc() instead of g_malloc() the qemu-guest-agent
>> Denial-of-Service attack referred in commit 807e2b6fce is reduced,
>> but still triggerable:
> 
> As explained previously, I believe there is *no* denial of service
> attack here. The described scenario is just a user hurting themselves
> by intentionally telling QEMU not to limit the amount of data returned.

Yes. Do you mind updating the BZ, eventually marking as NOTABUG? Then I 
can adapt the patch descriptions.
https://bugzilla.redhat.com/show_bug.cgi?id=1594054

> 
>>
>>    - bisect file size S until g_try_malloc(S) fails,
>>    - use S - 1:
>>      g_try_malloc(S - 1) succeeds, but g_new0() few lines later will
>>      fail.
>>
>>   346     buf = g_try_malloc0(count + 1);
>>   347     if (!buf) {
>>   348         error_setg(errp,
>>   349                    "failed to allocate sufficient memory "
>>   350                    "to complete the requested service");
>>   351         return NULL;
>>   352     }
>>   353     is_ok = ReadFile(fh, buf, count, &read_count, NULL);
>>   354     if (!is_ok) {
>>   355         error_setg_win32(errp, GetLastError(), "failed to read file");
>>   356         slog("guest-file-read failed, handle %" PRId64, handle);
>>   357     } else {
>>   358         buf[read_count] = 0;
>>   359         read_data = g_new0(GuestFileRead, 1);
>>                           ^^^^^^
>>
>> Instead we are going to put a low hard limit on 'count' in the next
>> commits.
>> This reverts commit 807e2b6fce022707418bc8f61c069d91c613b3d2.
>>
>> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   qga/commands-win32.c | 8 +-------
>>   1 file changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>> index b49920e201..46cea7d1d9 100644
>> --- a/qga/commands-win32.c
>> +++ b/qga/commands-win32.c
>> @@ -343,13 +343,7 @@ GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
>>       }
>>   
>>       fh = gfh->fh;
>> -    buf = g_try_malloc0(count + 1);
>> -    if (!buf) {
>> -        error_setg(errp,
>> -                   "failed to allocate sufficient memory "
>> -                   "to complete the requested service");
>> -        return NULL;
>> -    }
>> +    buf = g_malloc0(count + 1);
>>       is_ok = ReadFile(fh, buf, count, &read_count, NULL);
>>       if (!is_ok) {
>>           error_setg_win32(errp, GetLastError(), "failed to read file");
>> -- 
>> 2.21.1
>>
> 
> Regards,
> Daniel
> 

[PATCH-for-5.0 v2 2/4] qga: Extract guest_file_handle_find() to commands-common.h

[Philippe Mathieu-Daudé]

As we are going to reuse this method, declare it in common
header.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 qga/commands-common.h | 18 ++++++++++++++++++
 qga/commands-posix.c  |  7 ++++---
 qga/commands-win32.c  |  7 ++++---
 3 files changed, 26 insertions(+), 6 deletions(-)
 create mode 100644 qga/commands-common.h

diff --git a/qga/commands-common.h b/qga/commands-common.h
new file mode 100644
index 0000000000..af90e5481e
--- /dev/null
+++ b/qga/commands-common.h
@@ -0,0 +1,18 @@
+/*
+ * QEMU Guest Agent common/cross-platform common commands
+ *
+ * Copyright (c) 2020 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#ifndef QGA_COMMANDS_COMMON_H
+#define QGA_COMMANDS_COMMON_H
+
+#include "qga-qapi-types.h"
+
+typedef struct GuestFileHandle GuestFileHandle;
+
+GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp);
+
+#endif
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 93474ff770..2199b3b6d9 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -26,6 +26,7 @@
 #include "qemu/sockets.h"
 #include "qemu/base64.h"
 #include "qemu/cutils.h"
+#include "commands-common.h"
 
 #ifdef HAVE_UTMPX
 #include <utmpx.h>
@@ -237,12 +238,12 @@ typedef enum {
     RW_STATE_WRITING,
 } RwState;
 
-typedef struct GuestFileHandle {
+struct GuestFileHandle {
     uint64_t id;
     FILE *fh;
     RwState state;
     QTAILQ_ENTRY(GuestFileHandle) next;
-} GuestFileHandle;
+};
 
 static struct {
     QTAILQ_HEAD(, GuestFileHandle) filehandles;
@@ -268,7 +269,7 @@ static int64_t guest_file_handle_add(FILE *fh, Error **errp)
     return handle;
 }
 
-static GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp)
+GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp)
 {
     GuestFileHandle *gfh;
 
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 46cea7d1d9..cfaf6b84b8 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -37,6 +37,7 @@
 #include "qemu/queue.h"
 #include "qemu/host-utils.h"
 #include "qemu/base64.h"
+#include "commands-common.h"
 
 #ifndef SHTDN_REASON_FLAG_PLANNED
 #define SHTDN_REASON_FLAG_PLANNED 0x80000000
@@ -50,11 +51,11 @@
 
 #define INVALID_SET_FILE_POINTER ((DWORD)-1)
 
-typedef struct GuestFileHandle {
+struct GuestFileHandle {
     int64_t id;
     HANDLE fh;
     QTAILQ_ENTRY(GuestFileHandle) next;
-} GuestFileHandle;
+};
 
 static struct {
     QTAILQ_HEAD(, GuestFileHandle) filehandles;
@@ -126,7 +127,7 @@ static int64_t guest_file_handle_add(HANDLE fh, Error **errp)
     return handle;
 }
 
-static GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp)
+GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp)
 {
     GuestFileHandle *gfh;
     QTAILQ_FOREACH(gfh, &guest_file_state.filehandles, next) {
-- 
2.21.1

[PATCH-for-5.0 v2 3/4] qga: Extract qmp_guest_file_read() to common commands.c

[Philippe Mathieu-Daudé]

Extract the common code shared by both POSIX/Win32 implementations.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 qga/commands-common.h |  3 +++
 qga/commands-posix.c  | 22 +++-------------------
 qga/commands-win32.c  | 20 +++-----------------
 qga/commands.c        | 25 +++++++++++++++++++++++++
 4 files changed, 34 insertions(+), 36 deletions(-)

diff --git a/qga/commands-common.h b/qga/commands-common.h
index af90e5481e..90785ed4bb 100644
--- a/qga/commands-common.h
+++ b/qga/commands-common.h
@@ -15,4 +15,7 @@ typedef struct GuestFileHandle GuestFileHandle;
 
 GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp);
 
+GuestFileRead *guest_file_read_unsafe(GuestFileHandle *gfh,
+                                      int64_t count, Error **errp);
+
 #endif
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 2199b3b6d9..3352e9ca66 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -461,29 +461,14 @@ void qmp_guest_file_close(int64_t handle, Error **errp)
     g_free(gfh);
 }
 
-struct GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
-                                          int64_t count, Error **errp)
+GuestFileRead *guest_file_read_unsafe(GuestFileHandle *gfh,
+                                      int64_t count, Error **errp)
 {
-    GuestFileHandle *gfh = guest_file_handle_find(handle, errp);
     GuestFileRead *read_data = NULL;
     guchar *buf;
-    FILE *fh;
+    FILE *fh = gfh->fh;
     size_t read_count;
 
-    if (!gfh) {
-        return NULL;
-    }
-
-    if (!has_count) {
-        count = QGA_READ_COUNT_DEFAULT;
-    } else if (count < 0 || count >= UINT32_MAX) {
-        error_setg(errp, "value '%" PRId64 "' is invalid for argument count",
-                   count);
-        return NULL;
-    }
-
-    fh = gfh->fh;
-
     /* explicitly flush when switching from writing to reading */
     if (gfh->state == RW_STATE_WRITING) {
         int ret = fflush(fh);
@@ -498,7 +483,6 @@ struct GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
     read_count = fread(buf, 1, count, fh);
     if (ferror(fh)) {
         error_setg_errno(errp, errno, "failed to read file");
-        slog("guest-file-read failed, handle: %" PRId64, handle);
     } else {
         buf[read_count] = 0;
         read_data = g_new0(GuestFileRead, 1);
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index cfaf6b84b8..9717a8d52d 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -322,33 +322,19 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **errp)
     }
 }
 
-GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
-                                   int64_t count, Error **errp)
+GuestFileRead *guest_file_read_unsafe(GuestFileHandle *gfh,
+                                      int64_t count, Error **errp)
 {
     GuestFileRead *read_data = NULL;
     guchar *buf;
-    HANDLE fh;
+    HANDLE fh = gfh->fh;
     bool is_ok;
     DWORD read_count;
-    GuestFileHandle *gfh = guest_file_handle_find(handle, errp);
 
-    if (!gfh) {
-        return NULL;
-    }
-    if (!has_count) {
-        count = QGA_READ_COUNT_DEFAULT;
-    } else if (count < 0 || count >= UINT32_MAX) {
-        error_setg(errp, "value '%" PRId64
-                   "' is invalid for argument count", count);
-        return NULL;
-    }
-
-    fh = gfh->fh;
     buf = g_malloc0(count + 1);
     is_ok = ReadFile(fh, buf, count, &read_count, NULL);
     if (!is_ok) {
         error_setg_win32(errp, GetLastError(), "failed to read file");
-        slog("guest-file-read failed, handle %" PRId64, handle);
     } else {
         buf[read_count] = 0;
         read_data = g_new0(GuestFileRead, 1);
diff --git a/qga/commands.c b/qga/commands.c
index 4471a9f08d..8ee1244ebb 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -547,3 +547,28 @@ error:
     g_free(info);
     return NULL;
 }
+
+GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
+                                   int64_t count, Error **errp)
+{
+    GuestFileHandle *gfh = guest_file_handle_find(handle, errp);
+    GuestFileRead *read_data;
+
+    if (!gfh) {
+        return NULL;
+    }
+    if (!has_count) {
+        count = QGA_READ_COUNT_DEFAULT;
+    } else if (count < 0 || count >= UINT32_MAX) {
+        error_setg(errp, "value '%" PRId64 "' is invalid for argument count",
+                   count);
+        return NULL;
+    }
+
+    read_data = guest_file_read_unsafe(gfh, count, errp);
+    if (!read_data) {
+        slog("guest-file-write failed, handle: %" PRId64, handle);
+    }
+
+    return read_data;
+}
-- 
2.21.1

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

[Philippe Mathieu-Daudé]

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;
-- 
2.21.1

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

[Daniel P. Berrangé]

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
-- 
|: 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 :|

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

[Philippe Mathieu-Daudé]

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
> 

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

[Markus Armbruster]

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 :)

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

[Daniel P. Berrangé]

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
-- 
|: 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 :|

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

[Markus Armbruster]

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.