Re: [PATCH v3 1/4] block: Add trivial backing_fmt support to qcow, sheepdog, vmdk

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Mar 09, 2020 at 10:32:52AM -0500, Eric Blake wrote:
> On 3/9/20 10:21 AM, Kevin Wolf wrote:
> > Am 06.03.2020 um 23:51 hat Eric Blake geschrieben:
> > > For qcow2 and qed, we want to encourage the use of -F always, as these
> > > formats can suffer from data corruption or security holes if backing
> > > format is probed.  But for other formats, the backing format cannot be
> > > recorded.  Making the user decide on a per-format basis whether to
> > > supply a backing format string is awkward, better is to just blindly
> > > accept a backing format argument even if it is ignored by the
> > > contraints of the format at hand.
> > > 
> > > Signed-off-by: Eric Blake <eblake@redhat.com>
> > 
> > I'm not sure if I agree with this reasoning. Accepting and silently
> > ignoring -F could give users a false sense of security. If I specify a
> > -F raw and QEMU later probes qcow2, that would be very surprising.
> 
> Do we know what formats qcow, sheepdog, and vmdk expect to probe?  I'm
> wondering if we can compromise by checking that the requested backing image
> has the specified format, and error if it is not, rather than completely
> ignoring it - but at the same time, the image formats have no where to
> record a backing format.

Consider the user creates an image with "-F raw". We can validate the backing
image is raw, and so our check succeeds.  Later the malicious <something> can
write a qcow header into this raw file and QEMU will thereafter probe the
image as qcow, not raw.

IOW, in the case of "-F raw", even if we immediately check the format, we're
still not offering the protection promised by the "-F" flag, because that
promise refers to the runtime behaviour of the QEMU emulator, not the
immediate qemu-img cmd.

We could support "-F ..." and validate any non-raw formats, while raising a
runtime error in the case of "-F raw", as only the "raw" backing format has
the probing security risk.

Users who need  to use qcow, with a backing file, without a format can
just not pass "-F" and in doing so will be insecure.

We could take this opportunity to deprecate 'qcow' perhaps, declare it
a read-only format, restricted to qemu-img/qemu-io for purpose of data
liberation ?

For sheepdog, if it is something we genuinely still care about, then
adding a backing file format record seems neccessary, unless we either
forbid use of raw backing files, or forbid use of non-raw backing files,
either way would be safe.

> I'm guessing that qcow works with either raw or qcow as backing format (and
> anything else is odd - a qcow2 backing to a qcow is unusual, and would be
> better to reject).  I'm not sure if sheepdog can be backed by anything but
> another sheepdog, similarly, I'm not sure if a vmdk can be backed by
> anything but another vmdk.  If so, it should be simple enough to do a v4 of
> this patch which requires -F to be a known-acceptable probe type for these
> images.
> 
> Still, the point of this patch is that I want to add -F into all the
> iotests, and without something along the lines of this patch, all of those
> iotests are broken for these image formats.  Patch 2 is a lot harder to
> write if we have to make our use of -F conditional on the image format in
> question.
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|