Re: [Qemu-devel] [PATCH for-2.0 19/47] qcow2: Check header_length (CVE-2014-0144)

[Max Reitz <mreitz@redhat.com>]

On 26.03.2014 13:05, Stefan Hajnoczi wrote:
> From: Kevin Wolf <kwolf@redhat.com>
>
> This fixes an unbounded allocation for s->unknown_header_fields.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/qcow2.c              | 34 +++++++++++++++++++-------
>   tests/qemu-iotests/080     | 61 ++++++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/080.out |  9 +++++++
>   tests/qemu-iotests/group   |  1 +
>   4 files changed, 96 insertions(+), 9 deletions(-)
>   create mode 100755 tests/qemu-iotests/080
>   create mode 100644 tests/qemu-iotests/080.out
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index b9dc960..c3c88e9 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -460,6 +460,18 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>   
>       s->qcow_version = header.version;
>   
> +    /* Initialise cluster size */
> +    if (header.cluster_bits < MIN_CLUSTER_BITS ||
> +        header.cluster_bits > MAX_CLUSTER_BITS) {
> +        error_setg(errp, "Unsupported cluster size: 2^%i", header.cluster_bits);
> +        ret = -EINVAL;
> +        goto fail;
> +    }
> +
> +    s->cluster_bits = header.cluster_bits;
> +    s->cluster_size = 1 << s->cluster_bits;
> +    s->cluster_sectors = 1 << (s->cluster_bits - 9);
> +
>       /* Initialise version 3 header fields */
>       if (header.version == 2) {
>           header.incompatible_features    = 0;
> @@ -473,6 +485,18 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>           be64_to_cpus(&header.autoclear_features);
>           be32_to_cpus(&header.refcount_order);
>           be32_to_cpus(&header.header_length);
> +
> +        if (header.header_length < 104) {
> +            error_setg(errp, "qcow2 header too short");

I remember having a small discussion about whether to do this check or 
not once. The point is that if the value of this field is less than 104, 
its value is automatically invalid, as it is right at the end of the 
mandatory header fields (bytes 100 to 103); but it's hard to write a 
proper easy-to-understand error message, therefore any discussion about 
this is pretty moot (as far as I remember, this is also the result the 
mentioned discussion yielded). It's basically just nitpicking.

> +            ret = -EINVAL;
> +            goto fail;
> +        }
> +    }
> +
> +    if (header.header_length > s->cluster_size) {
> +        error_setg(errp, "qcow2 header exceeds cluster size");
> +        ret = -EINVAL;
> +        goto fail;
>       }
>   
>       if (header.header_length > sizeof(header)) {
> @@ -529,12 +553,6 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>       }
>       s->refcount_order = header.refcount_order;
>   
> -    if (header.cluster_bits < MIN_CLUSTER_BITS ||
> -        header.cluster_bits > MAX_CLUSTER_BITS) {
> -        error_setg(errp, "Unsupported cluster size: 2^%i", header.cluster_bits);
> -        ret = -EINVAL;
> -        goto fail;
> -    }
>       if (header.crypt_method > QCOW_CRYPT_AES) {
>           error_setg(errp, "Unsupported encryption method: %i",
>                      header.crypt_method);
> @@ -545,9 +563,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>       if (s->crypt_method_header) {
>           bs->encrypted = 1;
>       }
> -    s->cluster_bits = header.cluster_bits;
> -    s->cluster_size = 1 << s->cluster_bits;
> -    s->cluster_sectors = 1 << (s->cluster_bits - 9);
> +
>       s->l2_bits = s->cluster_bits - 3; /* L2 is always one cluster */
>       s->l2_size = 1 << s->l2_bits;
>       bs->total_sectors = header.size / 512;
> diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080
> new file mode 100755
> index 0000000..6512701
> --- /dev/null
> +++ b/tests/qemu-iotests/080
> @@ -0,0 +1,61 @@
> +#!/bin/bash
> +#
> +# qcow2 format input validation tests
> +#
> +# Copyright (C) 2013 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +# creator
> +owner=kwolf@redhat.com
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +
> +_cleanup()
> +{
> +	_cleanup_test_img
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +
> +_supported_fmt qcow2
> +_supported_proto generic
> +_supported_os Linux
> +
> +header_size=104
> +offset_header_size=100
> +offset_ext_magic=$header_size
> +offset_ext_size=$((header_size + 4))
> +
> +echo
> +echo "== Huge header size =="
> +_make_test_img 64M
> +poke_file "$TEST_IMG" "$offset_header_size" "\xff\xff\xff\xff"
> +{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
> +poke_file "$TEST_IMG" "$offset_header_size" "\x7f\xff\xff\xff"
> +{ $QEMU_IO -c "read 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
> +
> +# success, all done
> +echo "*** done"
> +rm -f $seq.full
> +status=0
> diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out
> new file mode 100644
> index 0000000..41a166a
> --- /dev/null
> +++ b/tests/qemu-iotests/080.out
> @@ -0,0 +1,9 @@
> +QA output created by 080
> +
> +== Huge header size ==
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> +qemu-io: can't open device TEST_DIR/t.qcow2: qcow2 header exceeds cluster size
> +no file open, try 'help open'
> +qemu-io: can't open device TEST_DIR/t.qcow2: qcow2 header exceeds cluster size
> +no file open, try 'help open'
> +*** done
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 9c99edc..ed44f35 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -85,6 +85,7 @@
>   077 rw auto quick
>   078 rw auto
>   079 rw auto
> +080 rw auto
>   081 rw auto
>   082 rw auto quick
>   083 rw auto

Reviewed-by: Max Reitz <mreitz@redhat.com>