Max Reitz <mre...@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 <cferg...@redhat.com> writes: >> >>> On Mon, Jan 07, 2019 at 04:47:44PM +0100, Markus Armbruster wrote: >>>> Eric Blake <ebl...@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? I figure what distinguishes this case is how utterly trivial creating a "bad" image is. 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 %-/
