[3/3] qemu-img: Deprecate use of -b without -F
diff mbox series

Message ID 20200222112341.4170045-4-eblake@redhat.com
State New
Headers show
Series
  • Tighten qemu-img rules on missing backing format
Related show

Commit Message

Eric Blake Feb. 22, 2020, 11:23 a.m. UTC
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 -
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
our probe differed from their expectations.  Similarly, if the backing
file name uses the json: psuedo-protocol, the backing name includes
the format.

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.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block.c                    | 17 ++++++++++++++++-
 qemu-deprecated.texi       | 12 ++++++++++++
 qemu-img.c                 |  8 +++++++-
 tests/qemu-iotests/114     |  4 ++--
 tests/qemu-iotests/114.out |  1 +
 5 files changed, 38 insertions(+), 4 deletions(-)

Comments

Peter Krempa Feb. 24, 2020, 11:38 a.m. UTC | #1
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:

$ qemu-img  create -f qcow2 -F raw -b /etc/passwd /tmp/malicious
Formatting '/tmp/malicious', fmt=qcow2 size=2560 backing_file=/etc/passwd backing_fmt=raw cluster_size=65536 lazy_refcounts=off refcount_bits=16


Now we want to create an overlay.

a) without this patchset:

$ qemu-img create -f qcow2 -b /tmp/malicious /tmp/pre-patch.qcow2
Formatting '/tmp/pre-patch.qcow2', fmt=qcow2 size=2560 backing_file=/tmp/malicious cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ qemu-img info /tmp/pre-patch.qcow2
image: /tmp/pre-patch.qcow2
file format: qcow2
virtual size: 2.5 KiB (2560 bytes)
disk size: 196 KiB
cluster_size: 65536
backing file: /tmp/malicious
Format specific information:
    compat: 1.1
    lazy refcounts: false
    refcount bits: 16
    corrupt: false

There's no 'backing file format'. When used by libvirt we'd not allow
the VM to touch the backing file of /tmp/malicious in pre-blockdev era
and in libvirt-6.0 we'd report an error right away.

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
$ qemu-img info /tmp/post-patch.qcow2
image: /tmp/post-patch.qcow2
file format: qcow2
virtual size: 2.5 KiB (2560 bytes)
disk size: 196 KiB
cluster_size: 65536
backing file: /tmp/malicious
backing file format: qcow2
Format specific information:
    compat: 1.1
    lazy refcounts: false
    refcount bits: 16
    corrupt: false

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.


> 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.

> our probe differed from their expectations.  Similarly, if the backing
> file name uses the json: psuedo-protocol, the backing name includes
> the format.

Not necessarily. The backing store string can be e.g.:

$ ./qemu-img create -f qcow1 -b 'json:{"driver":"file","filename":"/tmp/malicious"}' /tmp/json.qcow2
Formatting '/tmp/json.qcow1', fmt=qcow2 size=197120 backing_file=json:{"driver":"file",,"filename":"/tmp/malicious"} cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ qemu-img info /tmp/json.qcow1
image: /tmp/json.qcow1
file format: qcow1
virtual size: 191 KiB (197120 bytes)
disk size: 195 KiB
cluster_size: 65535
backing file: json:{"driver":"file","filename":"/tmp/malicious"}
Format specific information:
    compat: 0.1
    lazy refcounts: false
    refcount bits: 15
    corrupt: false

Now this has the old semantics but we didn't even get the warning. But
at least the backing file format is not written into the overlay.


> 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.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  block.c                    | 17 ++++++++++++++++-
>  qemu-deprecated.texi       | 12 ++++++++++++
>  qemu-img.c                 |  8 +++++++-
>  tests/qemu-iotests/114     |  4 ++--
>  tests/qemu-iotests/114.out |  1 +
>  5 files changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 695decbfd7b7..6595683ac52a 100644
> --- a/block.c
> +++ b/block.c
> @@ -6013,6 +6013,15 @@ 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 && !strstart(backing_file, "json:", NULL)) {
> +                backing_fmt = bs->drv->format_name;
> +                qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, backing_fmt, NULL);

We must never write the detected format into the overlay. Not even when
we print a warning. This can legitimize a malicious file if the user
mises the warning.
Eric Blake Feb. 24, 2020, 4:08 p.m. UTC | #2
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.

> 
>> our probe differed from their expectations.  Similarly, if the backing
>> file name uses the json: psuedo-protocol, the backing name includes
>> the format.
> 
> Not necessarily. The backing store string can be e.g.:
> 
> $ ./qemu-img create -f qcow1 -b 'json:{"driver":"file","filename":"/tmp/malicious"}' /tmp/json.qcow2
> Formatting '/tmp/json.qcow1', fmt=qcow2 size=197120 backing_file=json:{"driver":"file",,"filename":"/tmp/malicious"} cluster_size=65536 lazy_refcounts=off refcount_bits=16
> $ qemu-img info /tmp/json.qcow1
> image: /tmp/json.qcow1
> file format: qcow1
> virtual size: 191 KiB (197120 bytes)
> disk size: 195 KiB
> cluster_size: 65535
> backing file: json:{"driver":"file","filename":"/tmp/malicious"}
> Format specific information:
>      compat: 0.1
>      lazy refcounts: false
>      refcount bits: 15
>      corrupt: false
> 
> Now this has the old semantics but we didn't even get the warning. But
> at least the backing file format is not written into the overlay.

Hmm.  json:{"driver":"qcow2",...} encodes the format, but your argument 
is that json:{"driver":"file",...} encodes only the protocol but not the 
format.  We want the warning when there is no format, but with json:, it 
becomes harder to tell if a format is present or not.  I'll have to 
think about what to do in that case.


>> +++ b/block.c
>> @@ -6013,6 +6013,15 @@ 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 && !strstart(backing_file, "json:", NULL)) {
>> +                backing_fmt = bs->drv->format_name;
>> +                qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, backing_fmt, NULL);
> 
> We must never write the detected format into the overlay. Not even when
> we print a warning. This can legitimize a malicious file if the user
> mises the warning.
> 

Point taken. v2 will address this differently.
Peter Krempa Feb. 24, 2020, 5:28 p.m. UTC | #3
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.

Patch
diff mbox series

diff --git a/block.c b/block.c
index 695decbfd7b7..6595683ac52a 100644
--- a/block.c
+++ b/block.c
@@ -6013,6 +6013,15 @@  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 && !strstart(backing_file, "json:", NULL)) {
+                backing_fmt = bs->drv->format_name;
+                qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, backing_fmt, NULL);
+                if (bs->drv != &bdrv_raw) {
+                    warn_report("Deprecated use of non-raw backing file "
+                                "without explicit backing format, using "
+                                "detected format of %s", backing_fmt);
+                }
+            }
             if (size == -1) {
                 /* Opened BS, have no size */
                 size = bdrv_getlength(bs);
@@ -6026,7 +6035,13 @@  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 (backing_file && !backing_fmt &&
+               !strstart(backing_file, "json:", NULL)) {
+        warn_report("Deprecated use of unopened backing file without "
+                    "explicit backing format, use of this image requires "
+                    "potentially unsafe format probing");
+    }

     if (size == -1) {
         error_setg(errp, "Image creation needs a size parameter");
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 0671c26c806e..9228bcecd138 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -318,6 +318,18 @@  The above, converted to the current supported format:

 @section Related binaries

+@subsection qemu-img backing file without format (since 5.0.0)
+
+The use of @command{qemu-img create} or @command{qemu-img rebase} to
+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 the guest a potential security exploit if a future probe
+sees non-raw.  To avoid warning messages, or even future refusal to
+create an unsafe image, you must pass @option{-F} to specify the
+intended backing format.
+
 @subsection qemu-img convert -n -o (since 4.2.0)

 All options specified in @option{-o} are image creation options, so
diff --git a/qemu-img.c b/qemu-img.c
index b9375427404d..e75ec1bdb555 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3637,7 +3637,13 @@  static int img_rebase(int argc, char **argv)
      * doesn't change when we switch the backing file.
      */
     if (out_baseimg && *out_baseimg) {
-        ret = bdrv_change_backing_file(bs, out_baseimg, out_basefmt, false);
+        if (blk_new_backing && !out_basefmt) {
+            out_basefmt = blk_bs(blk_new_backing)->drv->format_name;
+            warn_report("Deprecated use of backing file "
+                        "without explicit backing format, using "
+                        "detected format of %s", out_basefmt);
+        }
+        ret = bdrv_change_backing_file(bs, out_baseimg, out_basefmt, true);
     } else {
         ret = bdrv_change_backing_file(bs, NULL, NULL, false);
     }
diff --git a/tests/qemu-iotests/114 b/tests/qemu-iotests/114
index 26104fff6c67..727e06e283a5 100755
--- a/tests/qemu-iotests/114
+++ b/tests/qemu-iotests/114
@@ -42,9 +42,9 @@  _unsupported_proto vxhs
 # qcow2.py does not work too well with external data files
 _unsupported_imgopts data_file

-
+# Intentionally specify backing file without backing format
 TEST_IMG="$TEST_IMG.base" _make_test_img 64M
-_make_test_img -b "$TEST_IMG.base" 64M
+_make_test_img -u -b "$TEST_IMG.base" 64M

 # Set an invalid backing file format
 $PYTHON qcow2.py "$TEST_IMG" add-header-ext 0xE2792ACA "foo"
diff --git a/tests/qemu-iotests/114.out b/tests/qemu-iotests/114.out
index 67adef37a4f6..81d5a8e0ad03 100644
--- a/tests/qemu-iotests/114.out
+++ b/tests/qemu-iotests/114.out
@@ -1,5 +1,6 @@ 
 QA output created by 114
 Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
+qemu-img: warning: Deprecated use of unopened backing file without explicit backing format, use of this image requires potentially unsafe format probing
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.base
 image: TEST_DIR/t.IMGFMT
 file format: IMGFMT