[PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances

Eric Blake eblake at redhat.com
Wed Feb 19 17:07:09 UTC 2020


On 2/19/20 10:40 AM, Peter Krempa wrote:

>> 1b: base.raw probes as qcow2 (because of whatever the guest wrote there),
>> using it as qcow2 is wrong - the guest will see corrupted data. What's more,
>> if the probe sees it as qcow2 with backing file, and we open the backing
>> file, it also has security implications.
> 
> We don't open the backing file in the proposed logic. That is
> specifically forbidden.

I agree that you contained the security risk by not following any file 
names mentioned in based.raw when interpreted as qcow2, but that does 
not mean you contained the data corruption risk.

> 
> Also in pre-blockdev configurations the same situation would happen as
> we allowed qemu to probe the backing file despite assuming the format to
> be raw. This was as we weren't able to tell qemu what the backing format
> is. This is fixed with -blockdev, so this scenario is exactly the same
> as it was before.

We assumed it to be raw, the question is whether qemu also assumed it to 
be raw, or qemu assumed it to be qcow2.  If qemu assumed it to be qcow2, 
sVirt may have saved us from the assumption going haywire and opening 
forbidden files, but it did NOT save the guest from seeing corrupted 
data.  If qemu assumed it to be raw, then all that was missing 
pre-blockdev was a way for us to tell qemu its assumptions were correct.

> 
>>
>> Scenario 2:
>>
>> base.qcow2 <- wrap.qcow2
>>
>> where wrap.qcow2 specifies backing of base.qcow2 but not the format.  If we
>> probe, we will always have just one outcome:
>>
>> 2a: base.qcow2 probes as qcow2. Using it as qcow2 is correct, but if
>> base.qcow2 has a further backing image, the backing chain is now dependent
>> on a probe.
>>
>> Since 1b and 2a have the same probe result, but massively different data
>> corruption and/or security concerns, it is NOT sufficient to claim that a
>> probe was safe merely because "the probed image does not have any backing or
>> data file".  It is ONLY safe if the probe turns up raw.  Any other probed
>> format is inherently unsafe.
> 
> I disagree here. If the probe of 'base.qcow2' showed a backing file, we
> refuse startup right away. If it didn't show any backing file, we
> continue:
> 
> 1) with old (pre-blockdev qemus) libvirt starts qemu with wrap.qcow2 as
> image. Qemu tries to open the backing file and probes it. Now if we've
> mis-detected that there is a backing file, we will depend on sVirt to
> save us. This scenario is how all of this was working until 2 months
> ago. It's because we've asumed that the format of 'base.qcow2' was raw,
> but started qemu. Since we didn't tell qemu what the format of
> 'base.qcow2' as it was impossible, we've relied on sVirt anyways.
> 
> This is the same as it was before.

So you're arguing that because qemu probes and treats the file as qcow2, 
even though we had assumed raw, but that the data actually was qcow2 (so 
the guest did not see data corruption), that we want to continue this 
practice by explicitly telling qemu that it is qcow2.

Then this all boils down to "what does qemu do when it probes an image 
that does not result in raw".  If qemu trusted the probe results anyway, 
then data corruption was already possible, but once the data corruption 
happens, the guest can no longer reverse the corruption nor cause 
further security damage (once qemu treats a previously-raw image as 
qcow2, the guest can no longer rewrite the qcow2 metadata).  If qemu 
failed to use the image because it treated the image as raw, then 
libvirt's decision to tell qemu that the image is qcow2 will CAUSE data 
corruption.

I recall that older qemu did blindly trust the probe results, but that 
there was discussion on the qemu list about patching things to warn 
about probes that resulted in anything other than raw.  But I could not 
quickly find mailing list discussions or specific patches that mention 
what actually happens; the closest I got was:

commit e4c8f2925d22584b2008aadea5c70e1e05c2a522
Author: Daniel P. Berrangé <berrange at redhat.com>
Date:   Tue Nov 20 17:56:46 2018 +0000

     iotests: fix nbd test 233 to work correctly with raw images

     The first qemu-io command must honour the $IMGFMT that is set rather
     than hardcoding qcow2. The qemu-nbd commands should also set $IMGFMT
     to avoid the insecure format probe warning.

> 
> 2) with new qemu, we do the same, but start qemu and specifically tell
> it that "the backing format is qcow2 and that the image has no backing
> file. That way qemu doesn't even attempt to open anythting. This means
> that this scenario fixes any deployment without selinux, while keeping
> old semantics around.

If you tell qemu that something is qcow2, but qemu has ever in the past 
treated that file as raw, then you are forcing data corruption on the 
guest, even if you avoid a security issue of chasing further backing 
files from treating that image as qcow2.

> 
>>> We perform the image format detection and in the case that we were able
>>> to probe the format and the format does not specify a backing store (or
>>> doesn't support backing store) we can use this format.
>>
>> Wrong.  The condition needs to be:
>>
>> If we probe the format, and the probe returns raw, then it is safe to use
>> raw as the format.
> 
> That doesn't solve anythign then. The idea of this series is to relax
> the restrictions we've imposed after introducing blockdev to return the
> main semantics back to what we've allowed in pre-blockdev
> configurations.

The only way I see that might be safe to relax restrictions is if the 
filename heuristics give us a clue as to the user's intent.  Data 
corruption is bad enough that we should never be the cause of it.  A 
user with a broken setup deserves a decent error message that their 
setup is broken, and how to fix it (even so much as "we detected qcow2, 
but weren't sure if you might have had a raw file instead, so do this to 
tell us which one you meant"), but blindly picking one always creates a 
corner case where our choice is wrong.

> 
> Namely a user creates an overlay on top of single raw/qcow2 image and
> expects it to work. And it's not just random users, libguestfs and
> openstack also neglected to set the backing format.
> 

Yes, and they are getting patched.  Belatedly, but better late than never.


>>> The price for this is that libvirt will need to keep the image format
>>> detector still current and working or replace it by invocation of
>>> qemu-img.
>>
>> Maybe.  Any format that qemu recognizes but libvirt does not risks a case
>> where libvirt probes the image as raw but lets qemu re-probe the image and
> 
> That doesn't happen with blockdev. We dont' let qemu probe.

We are just shifting the burden on who causes the data corruption when a 
probe goes wrong - it used to be qemu, now it is libvirt.


>>
>> I disagree with the logic here.  What we really need is:
>>
>> If the backing format was not specified, we probe to see what is there. If
>> the result of that probe is raw, it is safe to treat the image as raw.  If
>> the result is anything else, we must report an error stating that what we
>> probed could not be trusted unless the user adds an explicit backing format
>> (either confirming that our probe was correct, or with the correct format
>> overriding what we mis-probed).
> 
> As noted above, using this logic would be pointless. We are better off
> just reporting the error always if we also don't allow qcow2 without
> backing file to be used as it was previously used.

Then report the error always.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org




More information about the libvir-list mailing list