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 <kw...@redhat.com>
> > ---
> > 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
