Re: [Qemu-devel] [PATCH 5/5] qtest: Document calling conventions

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 07/21/2017 01:42 AM, Markus Armbruster wrote:
>>> But with json-lexer style, %s MODIFIES its input.
>> 
>> Your assertion "MODIFIES its input" confused me briefly, because I read
>> it as "side effect on the argument string".  That would be bonkers.
>> What you mean is it doesn't emit the argument string unmodified.
>
> Yes. I'm not sure how I could have worded that better; maybe "does not
> do a straight passthrough of its input"
>
>>> So adding the format immediately causes lots of warnings, such as:
>>>
>>>   CC      tests/vhost-user-test.o
>>> tests/vhost-user-test.c: In function ‘test_migrate’:
>>> tests/vhost-user-test.c:668:5: error: format not a string literal and no
>>> format arguments [-Werror=format-security]
>>>      rsp = qmp(cmd);
>>>      ^~~
>> 
>>     cmd = g_strdup_printf("{ 'execute': 'migrate',"
>>                           "'arguments': { 'uri': '%s' } }",
>>                           uri);
>>     rsp = qmp(cmd);
>>     g_free(cmd);
>>     g_assert(qdict_haskey(rsp, "return"));
>>     QDECREF(rsp);
>> 
>> For this to work, @uri must not contain funny characters.
>> 
>> Shouldn't we simply use the built-in quoting here?
>
> We can - but there are a lot of tests that have to be updated.

Not that many, see "[PATCH 0/9] tests: Clean up around qmp() and hmp()".
Its PATCH 4/9 makes the case for built-in quoting:

    When you build QMP input manually like this

        cmd = g_strdup_printf("{ 'execute': 'migrate',"
                              "'arguments': { 'uri': '%s' } }",
                              uri);
        rsp = qmp(cmd);
        g_free(cmd);

    you're responsible for escaping the interpolated values for JSON.  Not
    done here, and therefore works only for sufficiently nice @uri.  For
    instance, if @uri contained a single "'", qobject_from_jsonv() would
    fail, qmp_fd_sendv() would misinterpret the failure as empty input and
    do nothing, and the test would hang waiting for a response that never
    comes.

    Leaving interpolation into JSON to qmp() is more robust:

        rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);

    It's also more concise.

In short, using printf() & similar to format JSON is a JSON injection
vulnerability waiting to happen.  Special case: g_strdup_printf() to
format input for qobject_from_jsonf() & friends.  Leaving the job to
qobject_from_jsonf() is more robust.

>> 
>>     rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);
>>     g_assert(qdict_haskey(rsp, "return"));
>>     QDECREF(rsp);
>> 
>>>
>>>   CC      tests/boot-order-test.o
>>> tests/boot-order-test.c: In function ‘test_a_boot_order’:
>>> tests/boot-order-test.c:46:26: error: zero-length gnu_printf format
>>> string [-Werror=format-zero-length]
>>>      qmp_discard_response("");   /* HACK: wait for event */
>>>                           ^~
>> 
>> Should use qmp_eventwait().  Doesn't because it predates it.
>
> We can - but there are a lot of tests that have to be updated.

Also not that many, see same series.

>>> but we CAN'T rewrite those to qmp("%s", command).
>> 
>> Got more troublemakers?
>
> When I worked on getting rid of qobject_from_jsonf(), I was able to
> reduce the tests down to JUST using %s, which I then handled locally in
> qmp() rather than relying on qobject_from_jsonf().  Going the opposite
> direction and more fully relying on qobject_from_jsonf() by fixing
> multiple tests that were using older idioms is also an approach (in the
> opposite direction I originally took) - but the more work it is, the
> less likely that this patch is 2.10 material.

No need to worry about 2.10, I think.

>>> Now you can sort-of understand why my larger series wanted to get rid of
>>> qobject_from_jsonf(), since our use of GCC_FMT_ATTR() there is a lie?
>> 
>> I don't think it's a lie.  qobject_from_jsonf() satisfies the attribute
>> format(printf, ...) contract: it fetches varargs exactly like printf().
>> What it does with them is of no concern to this attribute.
>
> I still find it odd that you can't safely turn foo(var) into foo("%s", var).

For me, the protection against JSON injection bugs is well worth it.