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

[Max Reitz]

On 09.01.19 15:32, Markus Armbruster wrote:
> 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?

They are supposed to handle such cases gracefully, that's for sure.  At
least for qcow2 we do care about it.

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

I don't think an untrusted image should be able to crash qemu.

> 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 %-/

Er, then I suppose it is no security bug? O:-)

Max

signature.asc
Description: OpenPGP digital signature