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

[Daniel P . Berrangé]

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. 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. 

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.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|