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

From: P J P <ppandit@redhat.com>
To: "Marc-André Lureau" <marcandre.lureau@gmail.com>
Cc: "Fermín J. Serna" <fjserna@gmail.com>,
	"Daniel P . Berrange" <berrange@redhat.com>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Michael Roth" <mdroth@linux.vnet.ibm.com>
Subject: Re: [Qemu-devel] [PATCH v2] qga: check length of command-line & environment variables
Date: Thu, 23 May 2019 13:23:36 +0530 (IST)	[thread overview]
Message-ID: <nycvar.YSQ.7.76.1905231257400.23354@xnncv> (raw)
In-Reply-To: <CAJ+F1CLXdw4gE45vVEpStKrKsu-OYy1+5caC9wUduEtQRhjrpA@mail.gmail.com>

+-- 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