qga: check length of command-line & environment variables

Commit Message

Prasad Pandit Jan. 7, 2019, 10:34 a.m. UTC

From: Prasad J Pandit <[e-mail]>

Qemu guest agent while executing user commands does not seem to
check length of argument list and/or environment variables passed.
It may lead to integer overflow or infinite loop issues. Add check
to avoid it.

Reported-by: Niu Guoxiang <[e-mail]>
Signed-off-by: Prasad J Pandit <[e-mail]>
---
 qga/commands.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

[e-mail] Jan. 7, 2019, 1:04 p.m. UTC | #1

Patchew URL: https://patchew.org/QEMU/[e-mail]/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=8
=== TEST SCRIPT END ===

  LINK    qemu-edid.exe
  LINK    qemu-img.exe
/tmp/qemu-test/src/qga/commands.c: In function 'guest_exec_get_args':
/tmp/qemu-test/src/qga/commands.c:236:16: error: implicit declaration of function 'sysconf'; did you mean 'swscanf'? [-Werror=implicit-function-declaration]
     args_max = sysconf(_SC_ARG_MAX);
                ^~~~~~~
                swscanf
/tmp/qemu-test/src/qga/commands.c:236:16: error: nested extern declaration of 'sysconf' [-Werror=nested-externs]
/tmp/qemu-test/src/qga/commands.c:236:24: error: '_SC_ARG_MAX' undeclared (first use in this function); did you mean 'SCHAR_MAX'?
     args_max = sysconf(_SC_ARG_MAX);
                        ^~~~~~~~~~~
                        SCHAR_MAX


The full log is available at
http://patchew.org/logs/[e-mail]/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to [e-mail]

Prasad Pandit Jan. 11, 2019, 9:52 a.m. UTC | #2

+-- On Mon, 7 Jan 2019, P J P wrote --+
| Qemu guest agent while executing user commands does not seem to
| check length of argument list and/or environment variables passed.
| It may lead to integer overflow or infinite loop issues. Add check
| to avoid it.
| 
| -    size_t str_size = 1;
| +    size_t str_size = 1, args_max;
|  
| +    args_max = sysconf(_SC_ARG_MAX);

Looks like sysconf()/_SC_ARG_MAX declarations aren't available. Is it okay to 
include header <unistd.h> ?

===
diff --git a/qga/commands.c b/qga/commands.c
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -18,6 +18,7 @@
 #include "qemu/atomic.h"
+#include <unistd.h>
===

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Daniel P. Berrangé Jan. 11, 2019, 9:56 a.m. UTC | #3

On Fri, Jan 11, 2019 at 03:22:51PM +0530, P J P wrote:
> +-- On Mon, 7 Jan 2019, P J P wrote --+
> | Qemu guest agent while executing user commands does not seem to
> | check length of argument list and/or environment variables passed.
> | It may lead to integer overflow or infinite loop issues. Add check
> | to avoid it.
> | 
> | -    size_t str_size = 1;
> | +    size_t str_size = 1, args_max;
> |  
> | +    args_max = sysconf(_SC_ARG_MAX);
> 
> Looks like sysconf()/_SC_ARG_MAX declarations aren't available. Is it okay to 
> include header <unistd.h> ?

qga/commands.c already includes qemu/osdep.h which includs unistd.h.

The build problem patchew reported was from *mingw* builds where
sysconf does not exist.

Regards,
Daniel

Prasad Pandit Jan. 13, 2019, 5:28 p.m. UTC | #4

+-- On Fri, 11 Jan 2019, Daniel P. Berrangé wrote --+
| qga/commands.c already includes qemu/osdep.h which includs unistd.h.
|
| The build problem patchew reported was from *mingw* builds where
| sysconf does not exist.

I see; Not sure how to fix it. Maybe with conditional declaration?

#ifdef __MINGW[32|64]__
extern long int sysconf (int __name);
#endif

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

niuguoxiang Jan. 22, 2019, 3:49 a.m. UTC | #5

Hi,

Is this issue fixed?


Br，
Guoxiang Niu

华为技术有限公司 Huawei Technologies Co., Ltd.



本邮件及其附件含有华为公司的保密信息，仅限于发送给上面地址中列出的个人或群组。禁
止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、或散发）本邮件中
的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本邮件！
This e-mail and its attachments contain confidential information from HUAWEI, which 
is intended only for the person or entity whose address is listed above. Any use of the 
information contained herein in any way (including, but not limited to, total or partial 
disclosure, reproduction, or dissemination) by persons other than the intended 
recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by 
phone or email immediately and delete it!

-----邮件原件-----
发件人: P J P [mailto:[e-mail]] 
发送时间: 2019年1月14日 1:28
收件人: Daniel P. Berrangé <[e-mail]>
抄送: Michael Roth <[e-mail]>; QEMU Developers <[e-mail]>; niuguoxiang <[e-mail]>
主题: Re: [Qemu-devel] [PATCH] qga: check length of command-line & environment variables

+-- On Fri, 11 Jan 2019, Daniel P. Berrangé wrote --+
| qga/commands.c already includes qemu/osdep.h which includs unistd.h.
|
| The build problem patchew reported was from *mingw* builds where 
| sysconf does not exist.

I see; Not sure how to fix it. Maybe with conditional declaration?

#ifdef __MINGW[32|64]__
extern long int sysconf (int __name);
#endif

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Michael Roth Jan. 24, 2019, 5:38 p.m. UTC | #6

Quoting P J P (2019-01-13 11:28:03)
> +-- On Fri, 11 Jan 2019, Daniel P. Berrangé wrote --+
> | qga/commands.c already includes qemu/osdep.h which includs unistd.h.
> |
> | The build problem patchew reported was from *mingw* builds where
> | sysconf does not exist.
> 
> I see; Not sure how to fix it. Maybe with conditional declaration?
> 
> #ifdef __MINGW[32|64]__
> extern long int sysconf (int __name);
> #endif

I would call a helper function like get_args_max() or whatever and have
the posix implementation in qga/commands-posix.c and a stub'd version
in qga/commands-win32.c. There's an article here that might be useful
for figuring out how we would implement get_args_max() it for win32:

  https://blogs.msdn.microsoft.com/oldnewthing/20031210-00/?p=41553

> 
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Prasad Pandit Jan. 24, 2019, 5:53 p.m. UTC | #7

+-- On Thu, 24 Jan 2019, Michael Roth wrote --+
| I would call a helper function like get_args_max() or whatever and have
| the posix implementation in qga/commands-posix.c and a stub'd version
| in qga/commands-win32.c. There's an article here that might be useful
| for figuring out how we would implement get_args_max() it for win32:
| 
|   https://blogs.msdn.microsoft.com/oldnewthing/20031210-00/?p=41553

Interesting, I'll follow-up on it.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Patch

diff --git a/qga/commands.c b/qga/commands.c
index 0c7d1385c2..6d684ef209 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -231,17 +231,22 @@  static char **guest_exec_get_args(const strList *entry, bool log)
     int count = 1, i = 0;  /* reserve for NULL terminator */
     char **args;
     char *str; /* for logging array of arguments */
-    size_t str_size = 1;
+    size_t str_size = 1, args_max;
 
+    args_max = sysconf(_SC_ARG_MAX);
     for (it = entry; it != NULL; it = it->next) {
         count++;
         str_size += 1 + strlen(it->value);
+        if (str_size >= args_max / 2
+            || count >= args_max / sizeof(char *)) {
+            break;
+        }
     }
 
     str = g_malloc(str_size);
     *str = 0;
     args = g_malloc(count * sizeof(char *));
-    for (it = entry; it != NULL; it = it->next) {
+    for (it = entry; it != NULL && i < count; it = it->next) {
         args[i++] = it->value;
         pstrcat(str, str_size, it->value);
         if (it->next) {