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

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 09.03.2016 um 04:04 hat Eric Blake geschrieben:
>> 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?
>
> I can only repeat myself, we're not dealing with user data here, but
> with constant literal strings. Put an assert(subqdict_len < 32); at the
> beginning of the function and be done with it. Any violation of it is
> not unexpected user input, but a caller bug.

The fact that the keys are literals is a non-local, not-quite-obvious
argument.

It's non-local, because in qdict.c we don't know what its users store in
their qdicts.

It's not quite obvious for the only user so far, quorum_open().  New
uses outside the block layer are possible, but seem unlikely; the block
layer is the most demanding user of QDicts.

The block layer's use of qdicts and QemuOpts is "interesting" enough for
me to call it not quite obvious.  In particular, QemuOpts can either
accept a fixed set of keys, or arbitrary keys.  In the latter case,
unknown keys should be rejected by the consumer of the QemuOpts.
Whether that happens before they can reach qdict_array_entries() is not
obvious to me.

We can rely on non-local or subtle arguments when it's worthwhile, but
I'm not sure it's worthwhile here.  I'd use g_strdup_printf() and call
it a day.

>> Two assertions on the snprintf_ret should make sure we are safe,
>> right?
>
> No, asserting after the fact that you haven't just overflown a buffer is
> generally not a valid way of error handling (especially if you consider
> that compiling with NDEBUG would make the assert disappear).

snprintf() doesn't overflow the buffer, unless you pass a silly size.
Instead, it truncates and returns the untruncated length.  That lets you
assert it didn't truncate.  Perfectly fine way to assert the buffer
suffices.  More direct than assertions on the length that imply the
buffer will suffice.

> Of course, the strnlen() would already avoid this, so what the assertion
> would really catch is string truncation.
>
> In summary, the behaviour after your patch would still be correct, but
> it's pointless, it's less obvious what the reason for the array size is
> and it wastes memory on the stack. So I wouldn't do that.

I doubt replacing the variable length array

    char indexstr[32 + subqdict_len]

by

    char indexstr[128]

and an assertion "indexstr[] suffices" would be a wast.  Sure, we'd use
128 bytes instead of 32 + subqdict_len of stack, and that's roughly 80
bytes more for typical keys, but we'd save the stack pointer fiddling.

Anyway, I'd rather engage in undeniable waste and allocate dynamically.