[PATCH] qcow2: Forbid use of protocol: prefix on data_file

[PATCH] qcow2: Forbid use of protocol: prefix on data_file

[Eric Blake]

Ever since CVE-2024-4467 (see commit 7ead9469 in qemu v9.1.0), we have
intentionally treated command-line arguments as local files, and not
protocol specifications (you have to specify backing files with
full-blown QMP if it is intentional to access something more
complicated).  However, that patch forgot about qcow2 data-file, which
is another place where we really should not be hard-coding protocol
names in the qcow2 metadata.

Fix this by changing the decision point on whether to allow protocols
to each driver, rather than hard-coded to true in the generic code;
qcow2 data_file is the only place where we change the former default
of true.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/block-global-state.h | 3 ++-
 block.c                            | 4 ++--
 block/crypto.c                     | 2 +-
 block/parallels.c                  | 2 +-
 block/qcow.c                       | 2 +-
 block/qcow2.c                      | 4 ++--
 block/qed.c                        | 2 +-
 block/raw-format.c                 | 2 +-
 block/vdi.c                        | 2 +-
 block/vhdx.c                       | 2 +-
 block/vmdk.c                       | 2 +-
 block/vpc.c                        | 2 +-
 12 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 9be34b3c990..e53400de1cf 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -65,7 +65,8 @@ int co_wrapper bdrv_create(BlockDriver *drv, const char *filename,
                            QemuOpts *opts, Error **errp);

 int coroutine_fn GRAPH_UNLOCKED
-bdrv_co_create_file(const char *filename, QemuOpts *opts, Error **errp);
+bdrv_co_create_file(const char *filename, QemuOpts *opts,
+                    bool allow_protocol_prefix, Error **errp);

 BlockDriverState *bdrv_new(void);
 int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
diff --git a/block.c b/block.c
index f222e1a50a8..a5b5351e584 100644
--- a/block.c
+++ b/block.c
@@ -693,7 +693,7 @@ out:
 }

 int coroutine_fn bdrv_co_create_file(const char *filename, QemuOpts *opts,
-                                     Error **errp)
+                                     bool allow_protocol_prefix, Error **errp)
 {
     QemuOpts *protocol_opts;
     BlockDriver *drv;
@@ -702,7 +702,7 @@ int coroutine_fn bdrv_co_create_file(const char *filename, QemuOpts *opts,

     GLOBAL_STATE_CODE();

-    drv = bdrv_find_protocol(filename, true, errp);
+    drv = bdrv_find_protocol(filename, allow_protocol_prefix, errp);
     if (drv == NULL) {
         return -ENOENT;
     }
diff --git a/block/crypto.c b/block/crypto.c
index d4226cc68a4..5116bb6382c 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -821,7 +821,7 @@ block_crypto_co_create_opts_luks(BlockDriver *drv, const char *filename,
     }

     /* Create protocol layer */
-    ret = bdrv_co_create_file(filename, opts, errp);
+    ret = bdrv_co_create_file(filename, opts, true, errp);
     if (ret < 0) {
         goto fail;
     }
diff --git a/block/parallels.c b/block/parallels.c
index 3a375e2a8ab..7a90fb5220b 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1117,7 +1117,7 @@ parallels_co_create_opts(BlockDriver *drv, const char *filename,
     }

     /* Create and open the file (protocol layer) */
-    ret = bdrv_co_create_file(filename, opts, errp);
+    ret = bdrv_co_create_file(filename, opts, true, errp);
     if (ret < 0) {
         goto done;
     }
diff --git a/block/qcow.c b/block/qcow.c
index 8a3e7591a92..f7501fa2f03 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -978,7 +978,7 @@ qcow_co_create_opts(BlockDriver *drv, const char *filename,
     }

     /* Create and open the file (protocol layer) */
-    ret = bdrv_co_create_file(filename, opts, errp);
+    ret = bdrv_co_create_file(filename, opts, true, errp);
     if (ret < 0) {
         goto fail;
     }
diff --git a/block/qcow2.c b/block/qcow2.c
index 66fba89b414..bcf4d920946 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3954,7 +3954,7 @@ qcow2_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts,
     }

     /* Create and open the file (protocol layer) */
-    ret = bdrv_co_create_file(filename, opts, errp);
+    ret = bdrv_co_create_file(filename, opts, true, errp);
     if (ret < 0) {
         goto finish;
     }
@@ -3969,7 +3969,7 @@ qcow2_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts,
     /* Create and open an external data file (protocol layer) */
     val = qdict_get_try_str(qdict, BLOCK_OPT_DATA_FILE);
     if (val) {
-        ret = bdrv_co_create_file(val, opts, errp);
+        ret = bdrv_co_create_file(val, opts, false, errp);
         if (ret < 0) {
             goto finish;
         }
diff --git a/block/qed.c b/block/qed.c
index 4a36fb39294..da23a83d623 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -788,7 +788,7 @@ bdrv_qed_co_create_opts(BlockDriver *drv, const char *filename,
     }

     /* Create and open the file (protocol layer) */
-    ret = bdrv_co_create_file(filename, opts, errp);
+    ret = bdrv_co_create_file(filename, opts, true, errp);
     if (ret < 0) {
         goto fail;
     }
diff --git a/block/raw-format.c b/block/raw-format.c
index df16ac1ea25..a57c2922d55 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -463,7 +463,7 @@ static int coroutine_fn GRAPH_UNLOCKED
 raw_co_create_opts(BlockDriver *drv, const char *filename,
                    QemuOpts *opts, Error **errp)
 {
-    return bdrv_co_create_file(filename, opts, errp);
+    return bdrv_co_create_file(filename, opts, true, errp);
 }

 static int raw_open(BlockDriverState *bs, QDict *options, int flags,
diff --git a/block/vdi.c b/block/vdi.c
index 3ddc62a5690..87b874a7ef5 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -938,7 +938,7 @@ vdi_co_create_opts(BlockDriver *drv, const char *filename,
     qdict = qemu_opts_to_qdict_filtered(opts, NULL, &vdi_create_opts, true);

     /* Create and open the file (protocol layer) */
-    ret = bdrv_co_create_file(filename, opts, errp);
+    ret = bdrv_co_create_file(filename, opts, true, errp);
     if (ret < 0) {
         goto done;
     }
diff --git a/block/vhdx.c b/block/vhdx.c
index b2a4b813a0b..c16e4a00c8d 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -2096,7 +2096,7 @@ vhdx_co_create_opts(BlockDriver *drv, const char *filename,
     }

     /* Create and open the file (protocol layer) */
-    ret = bdrv_co_create_file(filename, opts, errp);
+    ret = bdrv_co_create_file(filename, opts, true, errp);
     if (ret < 0) {
         goto fail;
     }
diff --git a/block/vmdk.c b/block/vmdk.c
index 9c7ab037e14..576af241e59 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2332,7 +2332,7 @@ vmdk_create_extent(const char *filename, int64_t filesize, bool flat,
     int ret;
     BlockBackend *blk = NULL;

-    ret = bdrv_co_create_file(filename, opts, errp);
+    ret = bdrv_co_create_file(filename, opts, true, errp);
     if (ret < 0) {
         goto exit;
     }
diff --git a/block/vpc.c b/block/vpc.c
index 801ff5793f8..07e8ae0309a 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -1118,7 +1118,7 @@ vpc_co_create_opts(BlockDriver *drv, const char *filename,
     }

     /* Create and open the file (protocol layer) */
-    ret = bdrv_co_create_file(filename, opts, errp);
+    ret = bdrv_co_create_file(filename, opts, true, errp);
     if (ret < 0) {
         goto fail;
     }
-- 
2.49.0

Re: [PATCH] qcow2: Forbid use of protocol: prefix on data_file

[Eric Blake]

On Fri, May 23, 2025 at 01:20:32PM -0500, Eric Blake wrote:
> Ever since CVE-2024-4467 (see commit 7ead9469 in qemu v9.1.0), we have
> intentionally treated command-line arguments as local files, and not
> protocol specifications (you have to specify backing files with
> full-blown QMP if it is intentional to access something more
> complicated).  However, that patch forgot about qcow2 data-file, which
> is another place where we really should not be hard-coding protocol
> names in the qcow2 metadata.
> 
> Fix this by changing the decision point on whether to allow protocols
> to each driver, rather than hard-coded to true in the generic code;
> qcow2 data_file is the only place where we change the former default
> of true.

I should probably add an example to the commit message:

Pre-patch:

$ qemu-img create -f qcow2 -o data_file=rbd:virtual-disk-ceph-pool/datastore... datastore_rbd.qcow2 500M

tries to access the remote disk using rbd; but the "External data file
name" extension header in qcow2 stores an empty string; so the qcow2
file can't be reopened without QMP anyway.  Post-patch, the creation
attempt fails with:

$ qemu-img create -f qcow2 -o data_file=rbd:virtual-disk-ceph-pool/datastore datastore_rbd.qcow2 500M
Formatting 'datastore_rbd.qcow2', fmt=qcow2 cluster_size=65536 extended_l2=off compression_type=zlib size=524288000 data_file=rbd:virtual-disk-ceph-pool/datastore lazy_refcounts=off refcount_bits=16
qemu-img: datastore_rbd.qcow2: Could not create 'rbd:virtual-disk-ceph-pool/datastore': No such file or directory

and no qcow2 file is created.  It is still possible to use QMP to open
a qcow2 wrapper with rbd as the backing file.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH] qcow2: Forbid use of protocol: prefix on data_file

[Kevin Wolf]

Am 23.05.2025 um 20:20 hat Eric Blake geschrieben:
> Ever since CVE-2024-4467 (see commit 7ead9469 in qemu v9.1.0), we have
> intentionally treated command-line arguments as local files, and not
> protocol specifications (you have to specify backing files with
> full-blown QMP if it is intentional to access something more
> complicated).  However, that patch forgot about qcow2 data-file, which
> is another place where we really should not be hard-coding protocol
> names in the qcow2 metadata.
> 
> Fix this by changing the decision point on whether to allow protocols
> to each driver, rather than hard-coded to true in the generic code;
> qcow2 data_file is the only place where we change the former default
> of true.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

This commit message is very confusing. Commit 7ead9469 was primarily
about qcow2 data files, it certainly didn't forget them. It also didn't
do something in other places, but not in qcow2. Another thing it wasn't
about is command line arguments, but it restricted the references stored
in (potentially untrusted) image files.

The main difference between it and this patch is that commit 7ead9469
was about opening images (which is a security problem because you might
deal with untrusted images), and this one is about creating images
(which has no such problem, you're creating the image only now).

Of course, if you can't open an image with protocol: syntax, it makes
sense that creating it with the same syntax fails, too, for consistency.
So I'm not opposed to this patch, but I think it needs a completely
different commit message.

> diff --git a/block/vmdk.c b/block/vmdk.c
> index 9c7ab037e14..576af241e59 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -2332,7 +2332,7 @@ vmdk_create_extent(const char *filename, int64_t filesize, bool flat,
>      int ret;
>      BlockBackend *blk = NULL;
> 
> -    ret = bdrv_co_create_file(filename, opts, errp);
> +    ret = bdrv_co_create_file(filename, opts, true, errp);
>      if (ret < 0) {
>          goto exit;
>      }

If we want to be consistent with opening, VMDK extents should pass
allow_protocol_prefix=false.

Kevin