Re: [Qemu-devel] [PATCH v2] qga: check length of command-line & environment variables

[Marc-André Lureau]

Hi

On Sun, May 19, 2019 at 10:55 AM P J P <address@hidden> wrote:
>
> From: Prasad J Pandit <address@hidden>
>
> 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 <address@hidden>
> Signed-off-by: Prasad J Pandit <address@hidden>
> ---
>  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
>
>


-- 
Marc-André Lureau