raw: Prohibit dangerous writes for probed images

The only image format driver that even potentially accesses anything after 512 bytes in its bdrv_probe() implementation is VMDK, which reads a plain-text descriptor file. In practice, the field it's looking for seems to come first and will be well within the first 512 bytes, too.

Signed-off-by Kevin Wolf <[email protected]>

---
 block.c                   | 2 +-
 include/block/block_int.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

The other differences of this patch to the old one are that it doesn't silently write something different than the guest requested by zeroing out some bytes (it fails the request instead) and that it doesn't maintain a list of signatures in the raw driver (it calls the usual probe function instead).

Signed-off-by Kevin Wolf <[email protected]>

---
 block.c                   |  5 +++--

Quoted 11 lines

The other differences of this patch to the old one are that it doesn't silently write something different than the guest requested by zeroing out some bytes (it fails the request instead) and that it doesn't maintain a list of signatures in the raw driver (it calls the usual probe function instead).

Signed-off-by Kevin Wolf <[email protected]>

---
 block.c                   |  5 +++--

block/raw_bsd.c static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num, · line 88

Quoted 6 lines

block/raw_bsd.c +6 -0

+
+        drv = bdrv_probe_all(buf, 512, NULL);
+        if (drv != bs->drv) {
+            ret = -EPERM;
+            goto fail;
+        }

So, what happens when this returns -EPERM? If the guest is configured to halt on write errors, does this halt the guest and send an event to management? How does it compare to the case of halting on ENOSPACE? Is this particular failure mode something that the host should be able to easily distinguish from other failure modes?

But I definitely like that you only do this failure on probed images, and that a user that requests an explicit raw format will never trip up.

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.

All of these additions that allow to avoid format probing have to be specified explicitly. The default still allows the attack.

In order to fix this, commit 79368c8 (July 2010) put probed raw images in a restricted mode, in which they wouldn't be able to overwrite the first few bytes of the image so that they would identify as a different image. If a write to the first sector would write one of the signatures of another driver, qemu would instead zero out the first four bytes. This patch was later reverted in commit 8b33d9e (September 2010) because it didn't get the handling of unaligned qiov members right.

Today's block layer that is based on coroutines and has qiov utility functions makes it much easier to get this functionality right, so this patch implements it.

The other differences of this patch to the old one are that it doesn't silently write something different than the guest requested by zeroing out some bytes (it fails the request instead) and that it doesn't maintain a list of signatures in the raw driver (it calls the usual probe function instead).

Signed-off-by Kevin Wolf <[email protected]>

block/raw_bsd.c +2 -0

index 401b967..80f3a50 100644
@@ -58,8 +58,52 @@ static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num,
 static int coroutine_fn raw_co_writev(BlockDriverState *bs, int64_t sector_num,
                                       int nb_sectors, QEMUIOVector *qiov)
 {
+    void *buf = NULL;
+    BlockDriver *drv;

block/raw_bsd.c +12 -0

+        if (!buf) {
+            ret = -ENOMEM;
+            goto fail;
+        }
+
+        ret = qemu_iovec_to_buf(qiov, 0, buf, 512);
+        if (ret != 512) {
+            ret = -EINVAL;
+            goto fail;
+        }
+
+        drv = bdrv_probe_all(buf, 512, NULL);

block/raw_bsd.c +3 -0

+        qiov = &local_qiov;
+    }
+
     BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);

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.

All of these additions that allow to avoid format probing have to be specified explicitly. The default still allows the attack.

In order to fix this, commit 79368c8 (July 2010) put probed raw images in a restricted mode, in which they wouldn't be able to overwrite the first few bytes of the image so that they would identify as a different image. If a write to the first sector would write one of the signatures of another driver, qemu would instead zero out the first four bytes. This patch was later reverted in commit 8b33d9e (September 2010) because it didn't get the handling of unaligned qiov members right.

Today's block layer that is based on coroutines and has qiov utility functions makes it much easier to get this functionality right, so this patch implements it.

The other differences of this patch to the old one are that it doesn't silently write something different than the guest requested by zeroing out some bytes (it fails the request instead) and that it doesn't maintain a list of signatures in the raw driver (it calls the usual probe function instead).

Signed-off-by Kevin Wolf <[email protected]>

---
 block.c                   |  5 +++--
 block/raw_bsd.c           | 46 +++++++++++++++++++++++++++++++++++++++++++++-
 include/block/block_int.h |  3 +++
 3 files changed, 51 insertions(+), 3 deletions(-)

block.c +3 -2

index 8006685..bd7b2b5 100644
@@ -655,8 +655,8 @@ BlockDriver *bdrv_find_protocol(const char *filename,
  * probing score.
  * Return the first block driver with the highest probing score.
  */
-static BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size,
-                                   const char *filename)
+BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size,
+                            const char *filename)
 {
     int score_max = 0, score;
     BlockDriver *drv = NULL, *d;

@@ -1482,6 +1482,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
     }
 
     /* Image format probing */
+    bs->probed = !drv;
     if (!drv && file) {
         ret = find_image_format(file, filename, &drv, &local_err);
         if (ret < 0) {

block/raw_bsd.c +37 -0

index 401b967..80f3a50 100644
@@ -58,8 +58,52 @@ static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t sector_num,
 static int coroutine_fn raw_co_writev(BlockDriverState *bs, int64_t sector_num,
                                       int nb_sectors, QEMUIOVector *qiov)
 {
+    void *buf = NULL;
+    BlockDriver *drv;
+    QEMUIOVector local_qiov;
+    int ret;
+
+    if (bs->probed && sector_num == 0) {
+        /* As long as these conditions are true, we can't get partial writes to
+         * the probe buffer and can just directly check the request. */
+        QEMU_BUILD_BUG_ON(BLOCK_PROBE_BUF_SIZE != 512);
+        QEMU_BUILD_BUG_ON(BDRV_SECTOR_SIZE != 512);
+
+        buf = g_try_malloc(512);
+        if (!buf) {
+            ret = -ENOMEM;
+            goto fail;
+        }
+
+        ret = qemu_iovec_to_buf(qiov, 0, buf, 512);
+        if (ret != 512) {
+            ret = -EINVAL;
+            goto fail;
+        }
+
+        drv = bdrv_probe_all(buf, 512, NULL);
+        if (drv != bs->drv) {
+            ret = -EPERM;
+            goto fail;
+        }
+
+        /* Use the checked buffer, a malicious guest might be overwriting its
+         * original buffer in the background. */
+        qemu_iovec_init(&local_qiov, qiov->niov + 1);
+        qemu_iovec_add(&local_qiov, buf, 512);
+        qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size - 512);
+        qiov = &local_qiov;
+    }
+
     BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);