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@xxxxxxxxxx> > > > > 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 :|
