Re: [Qemu-devel] [PATCH] json: Fix % handling when not interpolating

[Markus Armbruster]

Max Reitz <address@hidden> writes:

> On 08.01.19 11:36, Markus Armbruster wrote:
>> Copying block maintainers for help with assessing the bug's (non-)impact
>> on security.
>> 
>> Christophe Fergeau <address@hidden> writes:
>> 
>>> On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote:
>>>> Eric Blake <address@hidden> writes:
>>>>
>>>>> On 1/2/19 12:01 PM, Christophe Fergeau wrote:
>>>>>> Adding Markus to cc: list, I forgot to do it when sending the patch.
>>>>>
>>>>> Also worth backporting via qemu-stable, now in cc.
>>>>>
>>>>>>
>>>>>> Christophe
>>>>>>
>>>>>> On Wed, Jan 02, 2019 at 03:05:35PM +0100, Christophe Fergeau wrote:
>>>>>>> commit 8bca4613 added support for %% in json strings when interpolating,
>>>>>>> but in doing so, this broke handling of % when not interpolating as the
>>>>>>> '%' is skipped in both cases.
>>>>>>> This commit ensures we only try to handle %% when interpolating.
>>>>
>>>> Impact?
>>>>
>>>> If you're unable to assess, could you give us at least a reproducer?
>>>
>>> This all came from 
>>> https://lists.freedesktop.org/archives/spice-devel/2018-December/046644.html
>>> Setting up a VM with libvirt with <graphics type='spice' autoport='yes' 
>>> passwd='password%'/>
>>> fails to start with:
>>>   qemu-system-x86_64: qobject/json-parser.c:146: parse_string: Assertion 
>>> `*ptr' failed.
>>>
>>> If you use 'password%%' as the password instead, when trying to connect
>>> to the VM, you type 'password%' as the password instead of 'password%%'
>>> as configured in the domain XML.
>> 
>> Thanks.
>> 
>> As the commit message says, the bug bites when we parse a string
>> containing '%s' with !ctxt->ap.  The parser then swallows a character.
>> If it swallows the terminating '"', it fails the assertion.
>> 
>> We parse with !ctxt->ap in the following cases:
>> 
>> * Tests (tests/check-qjson.c, tests/test-qobject-input-visitor.c,
>>   tests/test-visitor-serialization.c)
>> 
>>   Plenty of tests, but we still failed to cover the buggy case :(
>> 
>> * QMP input (monitor.c)
>> 
>> * QGA input (qga/main.c)
>> 
>> * qobject_from_json()
>> 
>>   - JSON pseudo-filenames (block.c)
>> 
>>     These are pseudo-filenames starting with "json:".
>> 
>>   - JSON key pairs (block/rbd.c)
>> 
>>     As far as I can tell, these can come only from pseudo-filenames
>>     starting with "rbd:".
>> 
>>   - JSON command line option arguments of -display and -blockdev
>>     (qobject-input-visitor.c)
>> 
>>     Reproducer: -blockdev '{"%"}'
>> 
>> Command line, QMP and QGA input are trusted.
>> 
>> Filenames are trusted when they come from command line, QMP or HMP.
>> They are untrusted when they come from from image file headers.
>> Example: QCOW2 backing file name.  Note that this is *not* the security
>> boundary between host and guest.  It's the boundary between host and an
>> image file from an untrusted source.
>> 
>> I can't see how the bug could be exploited.  Neither failing an
>> assertion nor skipping a character in a filename of your choice is
>> interesting.  We don't support compiling with NDEBUG.
>> 
>> Kevin, Max, do you agree?
>
> I wouldn't call it "not interesting" if adding an image to your VM at
> runtime can crash the whole thing.
>
> (qemu-img create -f qcow2 -u -b 'json:{"%"}' foo.qcow2 64M)

"Not interesting" strictly from the point of view of exploiting the bug
to penetrate trust boundaries.

> Whether this is a security issue...  I don't know, but it is a DoS.

I'm not sure whether feeding untrusted images to QEMU is a good idea in
general --- there's so much that could go wrong.  How hardened against
abuse are out block drivers?

I figure what distinguishes this case is how utterly trivial creating a
"bad" image is.

Anyway, you are the block layer maintainers, so you get to decide
whether to give this the full security bug treatment.  I'm merely the
clown who broke it %-/