On Wed, Feb 19, 2020 at 10:21:00 -0600, Eric Blake wrote:
> On 2/17/20 11:13 AM, Peter Krempa wrote:
> > Allow format probing to work around lazy clients which did not specify
> > their format in the overlay. Format probing will be allowed only, if we
>
> s/only, if/only if/
>
> > are able to probe the image, the probing result was successful and the
> > probed image does not have any backing or data file.
> >
> > This relaxes the restrictions which were imposed in commit 3615e8b39bad
> > in cases when we know that the image probing will not result in security
> > issues or data corruption.
>
> It took me a few minutes of thinking about this.
>
> Scenario 1:
>
> base.raw <- wrap.qcow2
>
> where wrap.qcow2 specifies backing of base.raw but not the format. If we
> probe, we can have a couple of outcomes:
>
> 1a: base.raw probes as raw (the probed image has no backing or data file),
> using it as raw is safe (it matches what wrap.qcow2 should have specified
> but didn't, and we aren't changing the data the guest would read nor are we
> opening unexpected files)
>
> 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.
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.
>
> 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.
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.
> > 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.
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.
> > With pre-blockdev configurations this will restore the previous
> > behaviour for the images mentioned above as qemu would probe the format
> > anyways. It also improves error reporting compared to the old state as
> > we now report that the backing chain will be broken in case when there
> > is a backing file.
>
> Improved error reporting because the probe returned qcow2 that would have
> required us to chase a backing file is fine; but while blindly accepting a
> qcow2 probe result when there is no backing file might avoid the security
> issue of chasing a backing file under guest control, it does not solve the
> data corruption issue of interpreting data as qcow2 that should have been
> interpreted as raw.
Again, this same scenario would happen previously, where we allowed qemu
to probe despite assuming the image to be raw, since we were unable to
tell qemu otherwise.
> > In blockdev configurations this ensures that libvirt will not cause data
> > corruption by ending the chain prematurely without notifying the user,
> > but still allows the old semantics when the users forgot to specify the
> > format.
>
> The only time where it is safe to imply a forgotten format is if the probed
> format is still raw.
>
> >
> > 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.
> then qemu exposes different data. But as long as libvirt always passes
> explicit format to qemu (including explicit raw format of a backing file
> whose format was forgotten but probing said it was raw), then it doesn't
> matter what other formats libvirt can probe for. The only benefit for
> libvirt probing formats is then for better error messages for non-raw.
>
> >
> > Signed-off-by: Peter Krempa <pkrempa@xxxxxxxxxx>
> > ---
> > src/util/virstoragefile.c | 52 ++++++++++++++++++++++-----------------
> > 1 file changed, 30 insertions(+), 22 deletions(-)
> >
[....]
> > - if (backingStore->format == VIR_STORAGE_FILE_AUTO) {
> > - /* Assuming the backing store to be raw can lead to failures. We do
> > - * it only when we must not report an error to prevent losing VMs.
> > - * Otherwise report an error.
> > - */
> > - if (report_broken) {
> > + if ((rv = virStorageFileGetMetadataRecurse(backingStore, parent,
> > + uid, gid,
> > + report_broken,
> > + cycle, depth + 1)) < 0) {
> > + if (!report_broken)
> > + return 0;
> > +
> > + if (rv == -2) {
> > virReportError(VIR_ERR_OPERATION_INVALID,
> > _("format of backing image '%s' of image '%s' was not specified in the image metadata "
> > "(See https://libvirt.org/kbase/backing_chains.html for troubleshooting)"),
> > src->backingStoreRaw, NULLSTR(src->path));
>
> 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.
