diff mbox series

[v2] qga: check length of command-line & environment variables

Message ID 20190519084815.7410-1-ppandit@redhat.com (mailing list archive)
State New, archived
Headers show
Series [v2] qga: check length of command-line & environment variables | expand

Commit Message

Prasad Pandit May 19, 2019, 8:48 a.m. UTC 
From: Prasad J Pandit <pjp@fedoraproject.org>

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 <niuguoxiang@huawei.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 qga/commands-posix.c   | 18 ++++++++++++++++++
 qga/commands-win32.c   | 13 +++++++++++++
 qga/commands.c         |  8 ++++++--
 qga/guest-agent-core.h |  2 ++
 4 files changed, 39 insertions(+), 2 deletions(-)

Update v2: add helper function ga_get_arg_max()
  -> https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg06360.html

Comments

Daniel Henrique Barboza May 20, 2019, 6:22 p.m. UTC | #1 
On 5/19/19 5:48 AM, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> 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 <niuguoxiang@huawei.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   qga/commands-posix.c   | 18 ++++++++++++++++++
>   qga/commands-win32.c   | 13 +++++++++++++
>   qga/commands.c         |  8 ++++++--
>   qga/guest-agent-core.h |  2 ++
>   4 files changed, 39 insertions(+), 2 deletions(-)
>
> Update v2: add helper function ga_get_arg_max()
>    -> https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg06360.html
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 7ee6a33cce..e0455722e0 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -60,6 +60,24 @@ extern char **environ;
>   #endif
>   #endif
>   
> +size_t ga_get_arg_max(void)
> +{
> +    /* Since kernel 2.6.23, most architectures support argument size limit
> +     * derived from the soft RLIMIT_STACK resource limit (see getrlimit(2)).
> +     * For these architectures, the total size is limited to 1/4 of the
> +     * allowed stack size. (see execve(2))
> +     *
> +     * struct rlimit r;
> +     *
> +     * getrlimit(RLIMIT_STACK, &r);
> +     * ARG_MAX = r.rlim_cur / 4;
> +     *
> +     * ARG_MAX is _NOT_ the maximum number of arguments. It is size of the
> +     * memory used to hold command line arguments and environment variables.
> +     */
> +    return sysconf(_SC_ARG_MAX);
> +}
> +
>   static void ga_wait_child(pid_t pid, int *status, Error **errp)
>   {
>       pid_t rpid;
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 6b67f16faf..47bbddd74a 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -92,6 +92,19 @@ static OpenFlags guest_file_open_modes[] = {
>       g_free(suffix); \
>   } while (0)
>   
> +size_t ga_get_arg_max(void)
> +{
> +    /* Win32 environment has different values for the ARG_MAX constant.
> +     * We'll go with the maximum here.
> +     *
> +     * https://devblogs.microsoft.com/oldnewthing/?p=41553
> +     *
> +     * ARG_MAX is _NOT_ the maximum number of arguments. It is size of the
> +     * memory used to hold command line arguments and environment variables.
> +     */
> +    return 32767;
> +}
> +
>   static OpenFlags *find_open_flag(const char *mode_str)
>   {
>       int mode;
> diff --git a/qga/commands.c b/qga/commands.c
> index 0c7d1385c2..425a4c405f 100644
> --- a/qga/commands.c
> +++ b/qga/commands.c
> @@ -231,17 +231,21 @@ 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, arg_max;
>   
> +    arg_max = ga_get_arg_max();
>       for (it = entry; it != NULL; it = it->next) {
>           count++;
>           str_size += 1 + strlen(it->value);
> +        if (str_size >= arg_max || count >= arg_max / 2) {
> +            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) {
> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> index 60eae16f27..300ff7e482 100644
> --- a/qga/guest-agent-core.h
> +++ b/qga/guest-agent-core.h
> @@ -46,6 +46,8 @@ const char *ga_fsfreeze_hook(GAState *s);
>   int64_t ga_get_fd_handle(GAState *s, Error **errp);
>   int ga_parse_whence(GuestFileWhence *whence, Error **errp);
>   
> +size_t ga_get_arg_max(void);
> +
>   #ifndef _WIN32
>   void reopen_fd_to_null(int fd);
>   #endif
Marc-André Lureau May 22, 2019, 1:34 p.m. UTC | #2 
Hi

On Sun, May 19, 2019 at 10:55 AM P J P <ppandit@redhat.com> wrote:
>
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> 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.

Are you intentionally not telling where these overflow or loop happen?

Isn't the kernel already giving an error if given too much
environment/arguments on exec?

>
> Reported-by: Niu Guoxiang <niuguoxiang@huawei.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  qga/commands-posix.c   | 18 ++++++++++++++++++
>  qga/commands-win32.c   | 13 +++++++++++++
>  qga/commands.c         |  8 ++++++--
>  qga/guest-agent-core.h |  2 ++
>  4 files changed, 39 insertions(+), 2 deletions(-)
>
> Update v2: add helper function ga_get_arg_max()
>   -> https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg06360.html
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 7ee6a33cce..e0455722e0 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -60,6 +60,24 @@ extern char **environ;
>  #endif
>  #endif
>
> +size_t ga_get_arg_max(void)
> +{
> +    /* Since kernel 2.6.23, most architectures support argument size limit
> +     * derived from the soft RLIMIT_STACK resource limit (see getrlimit(2)).
> +     * For these architectures, the total size is limited to 1/4 of the
> +     * allowed stack size. (see execve(2))
> +     *
> +     * struct rlimit r;
> +     *
> +     * getrlimit(RLIMIT_STACK, &r);
> +     * ARG_MAX = r.rlim_cur / 4;
> +     *
> +     * ARG_MAX is _NOT_ the maximum number of arguments. It is size of the
> +     * memory used to hold command line arguments and environment variables.
> +     */
> +    return sysconf(_SC_ARG_MAX);
> +}
> +
>  static void ga_wait_child(pid_t pid, int *status, Error **errp)
>  {
>      pid_t rpid;
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 6b67f16faf..47bbddd74a 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -92,6 +92,19 @@ static OpenFlags guest_file_open_modes[] = {
>      g_free(suffix); \
>  } while (0)
>
> +size_t ga_get_arg_max(void)
> +{
> +    /* Win32 environment has different values for the ARG_MAX constant.
> +     * We'll go with the maximum here.
> +     *
> +     * https://devblogs.microsoft.com/oldnewthing/?p=41553
> +     *
> +     * ARG_MAX is _NOT_ the maximum number of arguments. It is size of the
> +     * memory used to hold command line arguments and environment variables.
> +     */
> +    return 32767;
> +}
> +
>  static OpenFlags *find_open_flag(const char *mode_str)
>  {
>      int mode;
> diff --git a/qga/commands.c b/qga/commands.c
> index 0c7d1385c2..425a4c405f 100644
> --- a/qga/commands.c
> +++ b/qga/commands.c
> @@ -231,17 +231,21 @@ 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, arg_max;
>
> +    arg_max = ga_get_arg_max();
>      for (it = entry; it != NULL; it = it->next) {
>          count++;
>          str_size += 1 + strlen(it->value);
> +        if (str_size >= arg_max || count >= arg_max / 2) {
> +            break;

This seems to silently drop remaining arguments, which is probably not
what you want.

> +        }
>      }
>
>      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) {
> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> index 60eae16f27..300ff7e482 100644
> --- a/qga/guest-agent-core.h
> +++ b/qga/guest-agent-core.h
> @@ -46,6 +46,8 @@ const char *ga_fsfreeze_hook(GAState *s);
>  int64_t ga_get_fd_handle(GAState *s, Error **errp);
>  int ga_parse_whence(GuestFileWhence *whence, Error **errp);
>
> +size_t ga_get_arg_max(void);
> +
>  #ifndef _WIN32
>  void reopen_fd_to_null(int fd);
>  #endif
> --
> 2.20.1
>
>
Prasad Pandit May 23, 2019, 7:53 a.m. UTC | #3 
+-- On Wed, 22 May 2019, Marc-André Lureau wrote --+
| On Sun, May 19, 2019 at 10:55 AM P J P <ppandit@redhat.com> 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.
| 
| Are you intentionally not telling where these overflow or loop happen?
| 
| Isn't the kernel already giving an error if given too much
| environment/arguments on exec?

Kernel would report error; But integer overflow would occur while computing 
'str_size' in a loop below, if count++ wraps around due to long list of 
arguments (or a loop) in 'strList *entry'. Negative 'count' would allocate 
large memory for 'args'

    args = g_malloc(count * sizeof(char *));

We don't have a reproducer. It does seem remote/unlikely, considering 
guest-agent is to be used by trusted parties to manage a guest.

| >      int count = 1, i = 0;  /* reserve for NULL terminator */
| > +    size_t str_size = 1, arg_max;
| >
| > +    arg_max = ga_get_arg_max();
| >      for (it = entry; it != NULL; it = it->next) {
| >          count++;
| >          str_size += 1 + strlen(it->value);
| > +        if (str_size >= arg_max || count >= arg_max / 2) {
| > +            break;
| 
| This seems to silently drop remaining arguments, which is probably not
| what you want.

Umnm, report an error and return?


Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Marc-André Lureau May 23, 2019, 12:05 p.m. UTC | #4 
Hi

On Thu, May 23, 2019 at 9:54 AM P J P <ppandit@redhat.com> wrote:
>
> +-- On Wed, 22 May 2019, Marc-André Lureau wrote --+
> | On Sun, May 19, 2019 at 10:55 AM P J P <ppandit@redhat.com> 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.
> |
> | Are you intentionally not telling where these overflow or loop happen?
> |
> | Isn't the kernel already giving an error if given too much
> | environment/arguments on exec?
>
> Kernel would report error; But integer overflow would occur while computing
> 'str_size' in a loop below, if count++ wraps around due to long list of
> arguments (or a loop) in 'strList *entry'. Negative 'count' would allocate
> large memory for 'args'

I don't see how you could exploit this today.

QMP parser has MAX_TOKEN_COUNT (2ULL << 20).

We could have "assert(count < MAX_TOKEN_COUNT)" in the loop, if it helps.


>     args = g_malloc(count * sizeof(char *));
>
> We don't have a reproducer. It does seem remote/unlikely, considering
> guest-agent is to be used by trusted parties to manage a guest.
>
> | >      int count = 1, i = 0;  /* reserve for NULL terminator */
> | > +    size_t str_size = 1, arg_max;
> | >
> | > +    arg_max = ga_get_arg_max();
> | >      for (it = entry; it != NULL; it = it->next) {
> | >          count++;
> | >          str_size += 1 + strlen(it->value);
> | > +        if (str_size >= arg_max || count >= arg_max / 2) {
> | > +            break;
> |
> | This seems to silently drop remaining arguments, which is probably not
> | what you want.
>
> Umnm, report an error and return?
>
>
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Prasad Pandit May 29, 2019, 9:38 a.m. UTC | #5 
Hello Marc,

+-- On Thu, 23 May 2019, Marc-André Lureau wrote --+
| I don't see how you could exploit this today.
| 
| QMP parser has MAX_TOKEN_COUNT (2ULL << 20).

I see, didn't realise that. I tried to reproduce it and

   {"error": {"class": "GenericError", "desc": "JSON token count limit exceeded"}}

got above error around ~1048570 tokens; Much earlier than 0x200000(=2097152)  
as defined by MAX_TOKEN_COUNT. I guess multiple packets are being merged to 
form the incoming command and there is a glitch in there.

| We could have "assert(count < MAX_TOKEN_COUNT)" in the loop, if it helps.

No, assert() doesn't seem good.

I think same limit will apply to commands coming via QAPIs as well?


Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Marc-André Lureau May 29, 2019, 11:47 a.m. UTC | #6 
Hi

On Wed, May 29, 2019 at 11:38 AM P J P <ppandit@redhat.com> wrote:
>
>   Hello Marc,
>
> +-- On Thu, 23 May 2019, Marc-André Lureau wrote --+
> | I don't see how you could exploit this today.
> |
> | QMP parser has MAX_TOKEN_COUNT (2ULL << 20).
>
> I see, didn't realise that. I tried to reproduce it and
>
>    {"error": {"class": "GenericError", "desc": "JSON token count limit exceeded"}}
>
> got above error around ~1048570 tokens; Much earlier than 0x200000(=2097152)
> as defined by MAX_TOKEN_COUNT. I guess multiple packets are being merged to
> form the incoming command and there is a glitch in there.
>
> | We could have "assert(count < MAX_TOKEN_COUNT)" in the loop, if it helps.
>
> No, assert() doesn't seem good.

assert() is good if it's a programming error: that is if it should
never happen at run-time.
It's a decent way to document the code.

>
> I think same limit will apply to commands coming via QAPIs as well?

What do you mean? If the generated API is used internally by QEMU?
(it's not, but in this case there would be no limit)
Prasad Pandit May 29, 2019, 2:35 p.m. UTC | #7 
+-- On Wed, 29 May 2019, Marc-André Lureau wrote --+
| assert() is good if it's a programming error: that is if it should never 
| happen at run-time. It's a decent way to document the code.

  True; But terminating server because a user sent more input parameters does 
not sound good.

  {"error": {"class": "GenericError", "desc": "Guest agent command failed, 
   error was 'Failed to execute child process \u201C/bin/ls\u201D
  (Argument list too long)'"}}

returning an error, as it does, is better IMO.

| >
| > I think same limit will apply to commands coming via QAPIs as well?
|
| What do you mean? If the generated API is used internally by QEMU?
| (it's not, but in this case there would be no limit)

IIUC, the QAPIs could be used by external libraries/clients to send 
messages/commands to qemu/qemu-ga?

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Marc-André Lureau May 29, 2019, 2:44 p.m. UTC | #8 
Hi

On Wed, May 29, 2019 at 4:35 PM P J P <ppandit@redhat.com> wrote:
>
> +-- On Wed, 29 May 2019, Marc-André Lureau wrote --+
> | assert() is good if it's a programming error: that is if it should never
> | happen at run-time. It's a decent way to document the code.
>
>   True; But terminating server because a user sent more input parameters does
> not sound good.
>
>   {"error": {"class": "GenericError", "desc": "Guest agent command failed,
>    error was 'Failed to execute child process \u201C/bin/ls\u201D
>   (Argument list too long)'"}}
>
> returning an error, as it does, is better IMO.

The error is handled before guest_exec_get_args(), isn't it?

>
> | >
> | > I think same limit will apply to commands coming via QAPIs as well?
> |
> | What do you mean? If the generated API is used internally by QEMU?
> | (it's not, but in this case there would be no limit)
>
> IIUC, the QAPIs could be used by external libraries/clients to send
> messages/commands to qemu/qemu-ga?

The qga commands are only called through QMP, afaik.

thanks
Prasad Pandit May 29, 2019, 5:40 p.m. UTC | #9 
+-- On Wed, 29 May 2019, Marc-André Lureau wrote --+
| The error is handled before guest_exec_get_args(), isn't it?

Yes, which is okay I think.
 
| The qga commands are only called through QMP, afaik.

I see, cool! Thanks much for the confirmation.


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

Patch

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 7ee6a33cce..e0455722e0 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -60,6 +60,24 @@  extern char **environ;
 #endif
 #endif
 
+size_t ga_get_arg_max(void)
+{
+    /* Since kernel 2.6.23, most architectures support argument size limit
+     * derived from the soft RLIMIT_STACK resource limit (see getrlimit(2)).
+     * For these architectures, the total size is limited to 1/4 of the
+     * allowed stack size. (see execve(2))
+     *
+     * struct rlimit r;
+     *
+     * getrlimit(RLIMIT_STACK, &r);
+     * ARG_MAX = r.rlim_cur / 4;
+     *
+     * ARG_MAX is _NOT_ the maximum number of arguments. It is size of the
+     * memory used to hold command line arguments and environment variables.
+     */
+    return sysconf(_SC_ARG_MAX);
+}
+
 static void ga_wait_child(pid_t pid, int *status, Error **errp)
 {
     pid_t rpid;
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 6b67f16faf..47bbddd74a 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -92,6 +92,19 @@  static OpenFlags guest_file_open_modes[] = {
     g_free(suffix); \
 } while (0)
 
+size_t ga_get_arg_max(void)
+{
+    /* Win32 environment has different values for the ARG_MAX constant.
+     * We'll go with the maximum here.
+     *
+     * https://devblogs.microsoft.com/oldnewthing/?p=41553
+     *
+     * ARG_MAX is _NOT_ the maximum number of arguments. It is size of the
+     * memory used to hold command line arguments and environment variables.
+     */
+    return 32767;
+}
+
 static OpenFlags *find_open_flag(const char *mode_str)
 {
     int mode;
diff --git a/qga/commands.c b/qga/commands.c
index 0c7d1385c2..425a4c405f 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -231,17 +231,21 @@  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, arg_max;
 
+    arg_max = ga_get_arg_max();
     for (it = entry; it != NULL; it = it->next) {
         count++;
         str_size += 1 + strlen(it->value);
+        if (str_size >= arg_max || count >= arg_max / 2) {
+            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) {
diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
index 60eae16f27..300ff7e482 100644
--- a/qga/guest-agent-core.h
+++ b/qga/guest-agent-core.h
@@ -46,6 +46,8 @@  const char *ga_fsfreeze_hook(GAState *s);
 int64_t ga_get_fd_handle(GAState *s, Error **errp);
 int ga_parse_whence(GuestFileWhence *whence, Error **errp);
 
+size_t ga_get_arg_max(void);
+
 #ifndef _WIN32
 void reopen_fd_to_null(int fd);
 #endif