On Mon, Feb 24, 2020 at 10:08:31 -0600, Eric Blake wrote: > On 2/24/20 5:38 AM, Peter Krempa wrote: > > On Sat, Feb 22, 2020 at 05:23:41 -0600, Eric Blake wrote: > > > Creating an image that requires format probing of the backing image is > > > inherently unsafe (we've had several CVEs over the years based on > > > probes leaking information to the guest on a subsequent boot). If our > > > probing algorithm ever changes, or if other tools like libvirt > > > determine a different probe result than we do, then subsequent use of > > > that backing file under a different format will present corrupted data > > > to the guest. Start a deprecation clock so that future qemu-img can > > > refuse to create unsafe backing chains that would rely on probing. > > > > > > However, there is one time where probing is safe: when we first create > > > an image, no guest has yet used the new image, so as long as we record > > > what we probed, all future uses of the image will see the same data - > > > > I disagree. If you are creating an overlay on top of an existing image > > it's not safe to probe the format any more generally. (obviously you'd > > have to trust the image and express the trust somehow) > > > > The image may have been used in a VM as raw and that means that the VM > > might have recorded a valid qcow2 header into it. Creating the overlay > > with probing would legitimize this. > > > > Let's assume we have a malicious image written by the guest but we > > simulate it by: > > > > > b) Now with this patchset: > > > > $ ./qemu-img create -f qcow2 -b /tmp/malicious /tmp/post-patch.qcow2 > > qemu-img: warning: Deprecated use of non-raw backing file without explicit backing format, using detected format of qcow2 > > Formatting '/tmp/post-patch.qcow2', fmt=qcow2 size=2560 backing_file=/tmp/malicious backing_fmt=qcow2 cluster_size=65536 lazy_refcounts=off refcount_bits=16 > > > > > You now get a warning, but "backing file format" is now recorded in the > > overlay. Now this is WAY worse than it was before. The overlay now > > legitimizes the format recorded by the malicious guest which circumvents > > libvirt's protections. The warning is very easy to miss, and if you run > > it in scripts you might never get to see it. We can't allow that. > > Good point. I'll respin this series where v2 never writes the implicit > format except for a raw image (because probing raw is not only safe to > record, but also prevents the guest from ever changing that probe, and the > real risk we are interested in preventing is when a formerly raw image later > probes as non-raw). > > > > > > > > so the code now records the probe results as if the user had passed > > > -F. When this happens, it is unconditionally safe to record a probe > > > of 'raw', but any other probe is still worth warning the user in case > > > > While it's safe I don't think it should be encouraged. IMO -F should be > > made mandatory with -b. > > Making it mandatory will require the completion of the deprecation period. > For 5.0 and 5.1, the best we can do is the warning, but for 5.2 (assuming v2 > of this series is acceptable), it WILL become a hard error. Yes, that's fair. I just wanted to point out that the warning and later error should be reported also if raw is probed. I'm okay with recording raw into the overlay even now.
