Re: [Qemu-devel] [PATCH 3/4] raw: Prohibit dangerous writes for probed images

[Stefan Hajnoczi]

On Thu, Oct 30, 2014 at 01:26:15PM +0100, Kevin Wolf wrote:
> 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 <address@hidden>
> ---
>  block.c                   |  5 +++--
>  block/raw_bsd.c           | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>  include/block/block_int.h |  3 +++
>  3 files changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 8006685..bd7b2b5 100644
> --- a/block.c
> +++ b/block.c
> @@ -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) {
> diff --git a/block/raw_bsd.c b/block/raw_bsd.c
> index 401b967..80f3a50 100644
> --- a/block/raw_bsd.c
> +++ b/block/raw_bsd.c
> @@ -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);
> -    return bdrv_co_writev(bs->file, sector_num, nb_sectors, qiov);
> +    ret = bdrv_co_writev(bs->file, sector_num, nb_sectors, qiov);
> +
> +fail:
> +    if (qiov == &local_qiov) {
> +        qemu_iovec_destroy(&local_qiov);
> +    }
> +    g_free(buf);
> +    return ret;
>  }

Looking at this patch, I'm surprised to say I actually kind of like it.

Although I think it's problematic to restrict the guest from doing
something that a real machine is allowed to do, it only happens in
limited circumstances:
1. The image format was probed
2. The format was determined to be raw

I'd be okay with this.  Less invasive than I imagined.

Stefan

pgpSJCP2HOkCW.pgp
Description: PGP signature