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

[Max Reitz]

On 09.01.19 15:49, Daniel P. Berrangé wrote:
> On Wed, Jan 09, 2019 at 03:32:47PM +0100, 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?
>>
>> I figure what distinguishes this case is how utterly trivial creating a
>> "bad" image is.
> 
> Consider that you can already create a qcow2 image with a backing file
> of /etc/shadow.

If you cannot access this file, then it should just be an error and not
crash qemu.

If you can access this file, that's your own fault for bad permissions.

> Or create a qcow2 image many EB in size that causes QEMU 
> to allocate massive amounts of RAM and/or burn CPU time, and so on.

That would be the qcow2 driver's fault.  We do try to open only images
which are sane.

And memory allocation failures should be handled gracefully, so the VM
shouldn't crash.  Well, at least qcow2 does its best, what Linux makes
of it, who knows.  (e.g. it may assign the memory to qemu and then the
OOM killer may crash it later)

> IOW, mgmt apps should never pass untrusted images to QEMU.  Crashing is
> just one of many bad things, and probably not the worst that can happen.
> 
> They need to do validation upfront in some manner if receiving an 
> untrustworthy image. Openstack does this by running qemu-img, with 
> limits set on virutal memory size, CPU time, and then rejecting any 
> image with a backing file from being used at all.
> 
>> 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 %-/
> 
> Accepting an image with any backing file at all from an untrusted user
> would be a flaw in the layered management app itself, not QEMU.
> 
> So I think it would only be considered a security bug in QEMU if there was 
> a way for an unprivileged user to trick QEMU into writing malformed JSON
> into an otherwise trusted image.

Not making it a security bug makes me happy, of course, but I don't
quite agree that qemu is not to blame if you pass it some image which
makes it crash.

Max

signature.asc
Description: OpenPGP digital signature