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

From: "Marc-André Lureau" <marcandre.lureau@gmail.com>
To: P J P <ppandit@redhat.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: Wed, 29 May 2019 13:47:11 +0200	[thread overview]
Message-ID: <CAJ+F1C+xhBeoVqoE4aPgLqquq7rNKbZTtNSHe73FFgMyDCUzyw@mail.gmail.com> (raw)
In-Reply-To: <nycvar.YSQ.7.76.1905291448250.16122@xnncv>

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)

-- 
Marc-André Lureau