Re: [Qemu-devel] [PATCH 1/8] qdict: fix unbounded stack for qdict_array_entries

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2114 bytes --]

On 03/08/2016 07:57 PM, Peter Xu wrote:
> On Tue, Mar 08, 2016 at 11:19:44AM +0100, Kevin Wolf wrote:
>> Am 08.03.2016 um 09:22 hat Markus Armbruster geschrieben:
>>> Same arguments as for PATCH 2, except here an argument on the maximum
>>> length of subqdict would probably be easier.
>>
>> Yes, these are constant string literals in all callers, including the
>> one non-test case in quorum.
>>
>> Let's simply assert a reasonable maximum for subqdict_length. The
>> minimum we need to allow with the existing callers is 9, and I expect
>> we'll never get keys longer than 16 characters.
> 
> Hi, Kevin, Markus,
> 
> The patch should be trying to do as mentioned above. To make it
> clearer, how about the following one:
> 
> diff --git a/qobject/qdict.c b/qobject/qdict.c
> index 9833bd0..dde99e0 100644
> --- a/qobject/qdict.c
> +++ b/qobject/qdict.c
> @@ -704,17 +704,16 @@ int qdict_array_entries(QDict *src, const char *subqdict)
>      for (i = 0; i < INT_MAX; i++) {
>          QObject *subqobj;
>          int subqdict_entries;
> -        size_t slen = 32 + subqdict_len;
> -        char indexstr[slen], prefix[slen];
> +        char indexstr[128], prefix[128];
>          size_t snprintf_ret;
> 
> -        snprintf_ret = snprintf(indexstr, slen, "%s%u", subqdict, i);
> -        assert(snprintf_ret < slen);
> +        snprintf_ret = snprintf(indexstr, ARRAY_SIZE(indexstr), "%s%u", subqdict, i);
> +        assert(snprintf_ret < ARRAY_SIZE(indexstr));

sizeof(indexstr) works, and is a bit nicer than ARRAY_SIZE() when
dealing with char.

But I'm worried that this can trigger an abort() by someone hammering on
the command line.  Just because we don't expect any QMP command to
validate with a key name longer than 128 doesn't mean that we don't have
to deal with a command line with a garbage key name that long.  What's
wrong with just using g_strdup_printf() and heap-allocating the result,
avoiding snprintf() and fixed lengths altogether?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]