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

[Daniel Henrique Barboza]

On 5/19/19 5:48 AM, P J P 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.
>
> Reported-by: Niu Guoxiang <address@hidden>
> Signed-off-by: Prasad J Pandit <address@hidden>
> ---

Reviewed-by: Daniel Henrique Barboza <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;
> +        }
>       }
>   
>       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