11:07 a.m.
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(a)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