Re: [Qemu-devel] [PATCH v2 13/13] qcrypto-luks: implement more rigorous header checking

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Aug 26, 2019 at 04:51:03PM +0300, Maxim Levitsky wrote:
> Check that keyslots don't overlap with the data,
> and check that keyslots don't overlap with each other.
> (this is done using naive O(n^2) nested loops,
> but since there are just 8 keyslots, this doesn't really matter.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  crypto/block-luks.c | 46 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index db0fb764b4..fdf4c41f8a 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -541,12 +541,12 @@ fail:
>  static int
>  qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp)
>  {
> -    int ret;
> +    int ret = -EINVAL;

As before, no need to use errnos, just return -1 immediately.

> +    size_t i, j;
>  
>      if (memcmp(luks->header.magic, qcrypto_block_luks_magic,
>                 QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) {
>          error_setg(errp, "Volume is not in LUKS format");
> -        ret = -EINVAL;
>          goto fail;
>      }
>  
> @@ -557,6 +557,48 @@ qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp)
>          goto fail;
>      }
>  
> +    /* Check all keyslots for corruption  */
> +    for (i = 0 ; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; i++) {
> +
> +        const QCryptoBlockLUKSKeySlot *slot1 = &luks->header.key_slots[i];
> +        unsigned int start1 = slot1->key_offset_sector;
> +        unsigned int len1 =
> +            qcrypto_block_luks_splitkeylen_sectors(luks, slot1->stripes);
> +
> +        if (slot1->stripes == 0) {
> +            error_setg(errp, "Keyslot %zu is corrupted (stripes == 0)", i);
> +            goto fail;
> +        }

How about checking stripes != QCRYPTO_BLOCK_LUKS_STRIPES because
AFAIR, you're required to use 4k stripes in luks v1.

Also how about  checking    iters >= MIN_SLOT_KEY_ITERS

> +
> +        if (slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED &&
> +                slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED) {

Align the two lines with (

> +            error_setg(errp,
> +                       "Keyslot %zu state (active/disable) is corrupted", i);
> +            goto fail;
> +        }
> +
> +        if (start1 + len1 > luks->header.payload_offset_sector) {
> +            error_setg(errp,
> +                       "Keyslot %zu is overlapping with the encrypted payload",
> +                       i);
> +            goto fail;
> +        }
> +
> +        for (j = i + 1 ; j < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; j++) {
> +            const QCryptoBlockLUKSKeySlot *slot2 = &luks->header.key_slots[j];
> +            unsigned int start2 = slot2->key_offset_sector;
> +            unsigned int len2 =
> +                qcrypto_block_luks_splitkeylen_sectors(luks, slot2->stripes);
> +
> +            if (start1 + len1 > start2 && start2 + len2 > start1) {
> +                error_setg(errp,
> +                           "Keyslots %zu and %zu are overlapping in the header",
> +                           i, j);
> +                goto fail;
> +            }
> +        }
> +
> +    }
>      return 0;
>  fail:
>      return ret;
> -- 
> 2.17.2
> 

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