[Qemu-devel] [PATCH RFC 2/2] block: Warn on insecure format probing

[Markus Armbruster]

If the user neglects to specify the image format, QEMU probes the
image to guess it automatically, for convenience.

Relying on format probing is insecure for raw images (CVE-2008-2004).
If the guest writes a suitable header to the device, the next probe
will recognize a format chosen by the guest.  A malicious guest can
abuse this to gain access to host files, e.g. by crafting a QCOW2
header with backing file /etc/shadow.

Commit 1e72d3b (April 2008) provided -drive parameter format to let
users disable probing.  Commit f965509 (March 2009) extended QCOW2 to
optionally store the backing file format, to let users disable backing
file probing.  QED has had a flag to suppress probing since the
beginning (2010), set whenever a raw backing file is assigned.

Despite all this work (and time!), we're still insecure by default.  I
think we're doing our users a disservice by sticking to the fatally
flawed probing.  "Broken by default" is just wrong, and "convenience"
is no excuse.

I believe we can retain 90% of the convenience without compromising
security by keying on image file name instead of image contents: if
the file name ends in .img or .iso, assume raw, if it ends in .qcow2,
assume qcow2, and so forth.

Naturally, this would break command lines where the filename doesn't
provide the correct clue.  So don't do it just yet, only warn if the
the change would lead to a different result.  Looks like this:

    qemu: -drive file=my.img: warning: insecure format probing of image 'my.img'
    To get rid of this warning, specify format=qcow2 explicitly, or change
    the image name to end with '.qcow2'

This should steer users away from insecure format probing.  After a
suitable grace period, we can hopefully drop format probing
alltogether.

Example 0: file=WHATEVER,format=F

Never warns, because the explicit format suppresses probing.

Example 1: file=FOO.img

Warns when probing of FOO.img results in anything but raw.  In
particular, it warns when the guest just p0wned you.

Example 2: file=FOO.qcow2 with backing file name FOO.img and no
backing image format.

Warns when probing of FOO.qcow2 results in anything but qcow2, or
probing of FOO.img results in anything but raw.

This patch is RFC because of open questions:

* Should tools warn, too?  Probing isn't insecure there, but a "this
  may pick a different format in the future" warning may be
  appropriate.

* I didn't specify recognized extensions for formats "bochs", "cloop",
  "parallels", "vpc", because I have no idea which ones to recognize.

Additionally, some tests still need to be updated.

Signed-off-by: Markus Armbruster <address@hidden>
---
 block.c                   | 46 ++++++++++++++++++++++++++++++++++++++++++++--
 block/dmg.c               |  1 +
 block/qcow.c              |  1 +
 block/qcow2.c             |  1 +
 block/qed.c               |  1 +
 block/raw_bsd.c           |  1 +
 block/vdi.c               |  1 +
 block/vhdx.c              |  1 +
 block/vmdk.c              |  1 +
 include/block/block_int.h |  1 +
 10 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 8da6e61..7a1c592 100644
--- a/block.c
+++ b/block.c
@@ -674,10 +674,37 @@ static BlockDriver *bdrv_probe_all(const uint8_t *buf, 
int buf_size,
     return drv;
 }
 
+/*
+ * Guess image format from file name @fname
+ */
+static BlockDriver *bdrv_guess_format(const char *fname)
+{
+    BlockDriver *d;
+    int i, ext;
+    size_t fn_len = strlen(fname);
+    size_t ext_len;
+
+    QLIST_FOREACH(d, &bdrv_drivers, list) {
+        for (i = 0; i < ARRAY_SIZE(d->fname_ext) && d->fname_ext[i]; i++) {
+            ext_len = strlen(d->fname_ext[i]);
+            if (fn_len <= ext_len) {
+                continue;
+            }
+            ext = fn_len - ext_len;
+            if (fname[ext - 1] == '.'
+                && !strcmp(fname + ext, d->fname_ext[i])) {
+                return d;
+            }
+        }
+    }
+
+    return NULL;
+}
+
 static int find_image_format(BlockDriverState *bs, const char *filename,
                              BlockDriver **pdrv, Error **errp)
 {
-    BlockDriver *drv;
+    BlockDriver *drv, *secure_drv;
     uint8_t buf[2048];
     int ret = 0;
 
@@ -704,8 +731,23 @@ static int find_image_format(BlockDriverState *bs, const 
char *filename,
     if (!drv) {
         error_setg(errp, "Could not determine image format: No compatible "
                    "driver found");
-        ret = -ENOENT;
+        *pdrv = NULL;
+        return -ENOENT;
     }
+
+    secure_drv = bdrv_guess_format(filename);
+    if (use_bdrv_whitelist && drv != secure_drv) {
+        error_report("warning: insecure format probing of image '%s'",
+                     filename);
+        error_printf("To get rid of this warning, specify format=%s"
+                     " explicitly", drv->format_name);
+        if (drv->fname_ext[0]) {
+            error_printf(", or change\nthe image name to end with '.%s'",
+                         drv->fname_ext[0]);
+        }
+        error_printf("\n");
+    }
+
     *pdrv = drv;
     return ret;
 }
diff --git a/block/dmg.c b/block/dmg.c
index e455886..9f65f94 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -443,6 +443,7 @@ static BlockDriver bdrv_dmg = {
     .format_name    = "dmg",
     .instance_size  = sizeof(BDRVDMGState),
     .bdrv_probe     = dmg_probe,
+    .fname_ext      = { "dmg" },
     .bdrv_open      = dmg_open,
     .bdrv_read      = dmg_co_read,
     .bdrv_close     = dmg_close,
diff --git a/block/qcow.c b/block/qcow.c
index ece2269..268b248 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -956,6 +956,7 @@ static BlockDriver bdrv_qcow = {
     .format_name       = "qcow",
     .instance_size     = sizeof(BDRVQcowState),
     .bdrv_probe                = qcow_probe,
+    .fname_ext          = { "qcow" },
     .bdrv_open         = qcow_open,
     .bdrv_close                = qcow_close,
     .bdrv_reopen_prepare    = qcow_reopen_prepare,
diff --git a/block/qcow2.c b/block/qcow2.c
index d031515..31ff872 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2660,6 +2660,7 @@ static BlockDriver bdrv_qcow2 = {
     .format_name        = "qcow2",
     .instance_size      = sizeof(BDRVQcowState),
     .bdrv_probe         = qcow2_probe,
+    .fname_ext          = { "qcow2" },
     .bdrv_open          = qcow2_open,
     .bdrv_close         = qcow2_close,
     .bdrv_reopen_prepare  = qcow2_reopen_prepare,
diff --git a/block/qed.c b/block/qed.c
index 80f18d8..e1cbb94 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1659,6 +1659,7 @@ static BlockDriver bdrv_qed = {
     .supports_backing         = true,
 
     .bdrv_probe               = bdrv_qed_probe,
+    .fname_ext                = { "qed" },
     .bdrv_rebind              = bdrv_qed_rebind,
     .bdrv_open                = bdrv_qed_open,
     .bdrv_close               = bdrv_qed_close,
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 401b967..b429e66 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -176,6 +176,7 @@ static int raw_probe(const uint8_t *buf, int buf_size, 
const char *filename)
 static BlockDriver bdrv_raw = {
     .format_name          = "raw",
     .bdrv_probe           = &raw_probe,
+    .fname_ext            = { "img", "iso" },
     .bdrv_reopen_prepare  = &raw_reopen_prepare,
     .bdrv_open            = &raw_open,
     .bdrv_close           = &raw_close,
diff --git a/block/vdi.c b/block/vdi.c
index 19701ee..6c915cb 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -854,6 +854,7 @@ static BlockDriver bdrv_vdi = {
     .format_name = "vdi",
     .instance_size = sizeof(BDRVVdiState),
     .bdrv_probe = vdi_probe,
+    .fname_ext = { "vdi" },
     .bdrv_open = vdi_open,
     .bdrv_close = vdi_close,
     .bdrv_reopen_prepare = vdi_reopen_prepare,
diff --git a/block/vhdx.c b/block/vhdx.c
index 12bfe75..d2c3a20 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1945,6 +1945,7 @@ static BlockDriver bdrv_vhdx = {
     .format_name            = "vhdx",
     .instance_size          = sizeof(BDRVVHDXState),
     .bdrv_probe             = vhdx_probe,
+    .fname_ext              = { "vhd" },
     .bdrv_open              = vhdx_open,
     .bdrv_close             = vhdx_close,
     .bdrv_reopen_prepare    = vhdx_reopen_prepare,
diff --git a/block/vmdk.c b/block/vmdk.c
index 673d3f5..91a42d2 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2225,6 +2225,7 @@ static BlockDriver bdrv_vmdk = {
     .format_name                  = "vmdk",
     .instance_size                = sizeof(BDRVVmdkState),
     .bdrv_probe                   = vmdk_probe,
+    .fname_ext                    = { "vdmk" },
     .bdrv_open                    = vmdk_open,
     .bdrv_check                   = vmdk_check,
     .bdrv_reopen_prepare          = vmdk_reopen_prepare,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8898c6c..55609ae 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -91,6 +91,7 @@ struct BlockDriver {
 
     int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
     int (*bdrv_probe_device)(const char *filename);
+    const char *fname_ext[2];
 
     /* Any driver implementing this callback is expected to be able to handle
      * NULL file names in its .bdrv_open() implementation */
-- 
1.9.3