Re: [Qemu-devel] [PATCH 4/6] luks: Turn invalid assertion into check

[Eric Blake]

On 03/09/2018 11:27 AM, Kevin Wolf wrote:
> The .bdrv_getlength implementation of the crypto block driver asserted
> that the payload offset isn't after EOF. This is an invalid assertion to
> make as the image file could be corrupted. Instead, check it and return
> -EIO if the file is too small for the payload offset.

Good catch.  Probably not a CVE (unless someone can argue some way that 
causing a crash on an attempt to load a maliciously corrupted file can 
be used as a denial of service across a privilege boundary), but 
definitely needs fixing.

> Zero length images are fine, so trigger -EIO only on offset > len, not
> on offset >= len as the assertion did before.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>   block/crypto.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/block/crypto.c b/block/crypto.c
> index 2035f9ab13..4908d8627f 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -518,7 +518,10 @@ static int64_t block_crypto_getlength(BlockDriverState *bs)
>   
>       uint64_t offset = qcrypto_block_get_payload_offset(crypto->block);
>       assert(offset < INT64_MAX);

Umm, if the file can be corrupted, what's to prevent someone from 
sticking in a negative size that fails this assertion?

> -    assert(offset < len);
> +
> +    if (offset > len) {
> +       return -EIO;
> +    }
>   
>       len -= offset;
>   

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org