Daniel P. Berrangé <berra...@redhat.com> writes: > On Wed, Jan 09, 2019 at 04:02:28PM +0100, Max Reitz wrote: >> 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 <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. >> > >> > 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. > > The defintion of "sane" is quite hard though, as its contextual. There are > things that are sane when viewed from QEMU level, which can none the less > be considered a security bug from the mgmt app level. CPU/memory usage > associated with huge images is in this bucket I think, given how enourmous > some disk images can genuinely be. > >> 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) > > Yep, you'll quite likely succeed and trigger the OOM killer, or > alternatively succeed and push other running VMs out to swap > effectively inflicting a denial of service on them. > >> > 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.
No argument, it's definitely a bug. What we've been debating (as far as I'm concerned) is whether it's an exploitable security hole. I don't think it is, and it sounds like nobody really disagrees. The ability to run random crap downloaded from the internet as root with SELinux turned off is not a security hole. It's to be filed under "don't do that". Likewise, the ability to make a virtual machine abort() by feeding it untrusted images is not a security hole, it's a "don't do that". > Certainly QEMU should never crash on any input. I just think that wrt > untrustworthy disk images, you need security protection well before you > get to a live QEMU VM process, so I think this can be just a "normal" bug. Concur.
