On 09.01.19 17:20, Markus Armbruster wrote: > Max Reitz <mre...@redhat.com> writes: > >> On 09.01.19 15:32, Markus Armbruster wrote: >>> 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? >> >> 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 ;
signature.asc
Description: OpenPGP digital signature
