Re: [PATCH v5 7/7] qemu-img: Deprecate use of -b without -F
On Tue, May 05, 2020 at 10:11:03 +0200, Kevin Wolf wrote: > Am 03.04.2020 um 19:58 hat Eric Blake geschrieben: > > 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, although > > these days tools like libvirt are aware of the issue enough to prevent > > the worst effects). However, 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. Most warnings are intentionally > > emitted from bdrv_img_create() in the block layer, but qemu-img > > convert uses bdrv_create() which cannot emit its own warning without > > causing spurious warnings on other code paths. In the end, all > > command-line image creation or backing file rewriting now performs a > > check. > > > > However, there is one time where probing is safe: if we probe raw, > > then it is safe to record that implicitly in the image (but we still > > warn, as it's better to teach the user to supply -F always than to > > make them guess when it is safe). > > You're not stating it explicitly, but I guess the thing that you mean > that is actually unsafe is if you have a raw image, always pass > format=raw to QEMU (so the guest could write e.g. a qcow2 header), but > then create a backing file without -F, so it will be probed. This is as > bad as specifying format=raw only sometimes. > > I don't like the idea of responding to this by making raw images more > convenient to use than actual image formats. > > How about we approach it the other way around: The troublemaker is raw, > so let's require specifying raw explicitly, and record the probed format > implicitly in other cases. This is a bit weaker in the immediate effect > in that it doesn't protect you when you actually deal with a malicious > image, but in normal use it will point out where your scripts or > management software is too careless. The final result should be that > management tools are fixed and you'll be safe, while manual users who > can usually trust their guests aren't inconvenienced too much. That is definitely better than just probing. In my opinion though the format should always be specified explicitly. No favorism of 'raw' or any other format. In libvirt we fill-in that the format of the image is 'raw' unless the user specifies something else rather than reporting an error. This causes continuous grief for users who forget to set the format. (one example is that if you create a fully-allocated qcow2 image but use it as raw and install your VM, you'll never notice until you want to use qcow2 features. Users usually notice once they try to create a sparse image though due to the size difference)
Re: [PATCH v5 7/7] qemu-img: Deprecate use of -b without -F
Am 03.04.2020 um 19:58 hat Eric Blake geschrieben: > 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, although > these days tools like libvirt are aware of the issue enough to prevent > the worst effects). However, 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. Most warnings are intentionally > emitted from bdrv_img_create() in the block layer, but qemu-img > convert uses bdrv_create() which cannot emit its own warning without > causing spurious warnings on other code paths. In the end, all > command-line image creation or backing file rewriting now performs a > check. > > However, there is one time where probing is safe: if we probe raw, > then it is safe to record that implicitly in the image (but we still > warn, as it's better to teach the user to supply -F always than to > make them guess when it is safe). You're not stating it explicitly, but I guess the thing that you mean that is actually unsafe is if you have a raw image, always pass format=raw to QEMU (so the guest could write e.g. a qcow2 header), but then create a backing file without -F, so it will be probed. This is as bad as specifying format=raw only sometimes. I don't like the idea of responding to this by making raw images more convenient to use than actual image formats. How about we approach it the other way around: The troublemaker is raw, so let's require specifying raw explicitly, and record the probed format implicitly in other cases. This is a bit weaker in the immediate effect in that it doesn't protect you when you actually deal with a malicious image, but in normal use it will point out where your scripts or management software is too careless. The final result should be that management tools are fixed and you'll be safe, while manual users who can usually trust their guests aren't inconvenienced too much. Kevin
[PATCH v5 7/7] qemu-img: Deprecate use of -b without -F
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, although
these days tools like libvirt are aware of the issue enough to prevent
the worst effects). However, 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. Most warnings are intentionally
emitted from bdrv_img_create() in the block layer, but qemu-img
convert uses bdrv_create() which cannot emit its own warning without
causing spurious warnings on other code paths. In the end, all
command-line image creation or backing file rewriting now performs a
check.
However, there is one time where probing is safe: if we probe raw,
then it is safe to record that implicitly in the image (but we still
warn, as it's better to teach the user to supply -F always than to
make them guess when it is safe).
iotest 114 specifically wants to create an unsafe image for later
amendment rather than defaulting to our new default of recording a
probed format, so it needs an update. While touching it, expand it to
cover all of the various warnings enabled by this patch. iotest 290
also shows a change to qcow messages; note that the fact that we now
make a probed format of 'raw' explicit now results in a double
warning, but no one should be creating new qcow images so it is not
worth cleaning up.
Signed-off-by: Eric Blake
---
docs/system/deprecated.rst | 20
block.c| 21 -
qemu-img.c | 9 -
tests/qemu-iotests/114 | 12
tests/qemu-iotests/114.out | 9 +
tests/qemu-iotests/290.out | 5 -
6 files changed, 73 insertions(+), 3 deletions(-)
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index aac6be58917a..8abfd436820a 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -429,6 +429,26 @@ image). Rather, any changes to the backing chain should
be performed
with ``qemu-img rebase -u`` either before or after the remaining
changes being performed by amend, as appropriate.
+qemu-img backing file without format (since 5.0.0)
+''
+
+The use of ``qemu-img create``, ``qemu-img rebase``, or ``qemu-img
+convert`` to create or modify an image that depends on a backing file
+now recommends that an explicit backing format be provided. This is
+for safety: if QEMU probes a different format than what you thought,
+the data presented to the guest will be corrupt; similarly, presenting
+a raw image to a guest allows a potential security exploit if a future
+probe sees a non-raw image based on guest writes.
+
+To avoid the warning message, or even future refusal to create an
+unsafe image, you must pass ``-o backing_fmt=`` (or the shorthand
+``-F`` during create) to specify the intended backing format. You may
+use ``qemu-img rebase -u`` to retroactively add a backing format to an
+existing image. However, be aware that there are already potential
+security risks to blindly using ``qemu-img info`` to probe the format
+of an untrusted backing image, when deciding what format to add into
+an existing image.
+
``qemu-img convert -n -o`` (since 4.2.0)
diff --git a/block.c b/block.c
index 70cc6123dd91..61871965eee4 100644
--- a/block.c
+++ b/block.c
@@ -6072,6 +6072,20 @@ void bdrv_img_create(const char *filename, const char
*fmt,
"Could not open backing image to determine
size.\n");
goto out;
} else {
+if (!backing_fmt) {
+warn_report("Deprecated use of backing file without explicit "
+"backing format (detected format of %s)",
+bs->drv->format_name);
+if (bs->drv == _raw) {
+/*
+ * A probe of raw is always correct, so in this one
+ * case, we can write that into the image.
+ */
+backing_fmt = bs->drv->format_name;
+qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, backing_fmt,
+ NULL);
+}
+}
if (size == -1) {
/* Opened BS, have no size */
size = bdrv_getlength(bs);
@@ -6085,7 +6099,12 @@ void bdrv_img_create(const char *filename, const char
*fmt,
}
bdrv_unref(bs);
}
-} /* (backing_file && !(flags & BDRV_O_NO_BACKING)) */
+/* (backing_file && !(flags & BDRV_O_NO_BACKING)) */
+} else if
