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

[Daniel P . Berrangé]

On Fri, Mar 09, 2018 at 06:27:11PM +0100, 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.
> 
> 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 
> ---
>  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);
> -assert(offset < len);
> +
> +if (offset > len) {
> +   return -EIO;
> +}
>  
>  len -= offset;

Reviewed-by: Daniel P. Berrangé 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [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 
> > ---
> >   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

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 
---
  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

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

[Kevin Wolf]

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.

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 
---
 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);
-assert(offset < len);
+
+if (offset > len) {
+   return -EIO;
+}
 
 len -= offset;
 
-- 
2.13.6