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

[Markus Armbruster]

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?