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

[Max Reitz]

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)

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

Max

signature.asc
Description: OpenPGP digital signature