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

[Kevin Wolf]

Am 09.03.2018 um 21:19 hat Eric Blake geschrieben:
> 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?

In qcrypto_block_luks_open():

    block->sector_size = QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
    block->payload_offset = luks->header.payload_offset *
            block->sector_size

The sector size is 512LL, and luks->header.payload_offset is 32 bit.

But I just saw that block_crypto_truncate() has another wrong assertion.
Maybe I should fix that and write a test case for it. Not sure if I'll
add it to this series or as a follow-up during the freeze.

Kevin