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

[Marc-André Lureau]

Hi

On Thu, May 23, 2019 at 9:54 AM P J P <address@hidden> wrote:
>
> +-- On Wed, 22 May 2019, Marc-André Lureau wrote --+
> | On Sun, May 19, 2019 at 10:55 AM P J P <address@hidden> 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



-- 
Marc-André Lureau