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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5037 bytes --]

On 09.01.19 17:20, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> On 09.01.19 15:32, Markus Armbruster wrote:
>>> Max Reitz <mreitz@redhat.com> 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 <cfergeau@redhat.com> writes:
>>>>>
>>>>>> On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote:
>>>>>>> Eric Blake <eblake@redhat.com> 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.
> 
> "Should" in the sense of "if they don't, it's a bug, and we'll do what
> we can to fix it", or "if they don't, I'll be surprised"?

Depends.  If it's Linux's VMM design (lazy allocation + OOM killer), I
don't care.  If there is something we can do to fix it, I do think it's
a bug.

Max

>>> 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:-)
> 
> I'm not charging toll for the bridge I built for you ;


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]