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

[Markus Armbruster]

Daniel P. Berrangé <address@hidden> 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 <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.
>> 
>> 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.