[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v2 13/13] qcrypto-luks: implement more rigorous
From: |
Daniel P . Berrangé |
Subject: |
Re: [Qemu-block] [PATCH v2 13/13] qcrypto-luks: implement more rigorous header checking |
Date: |
Fri, 6 Sep 2019 14:34:54 +0100 |
User-agent: |
Mutt/1.12.1 (2019-06-15) |
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 <address@hidden>
> ---
> 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 :|
- Re: [Qemu-block] [PATCH v2 13/13] qcrypto-luks: implement more rigorous header checking,
Daniel P . Berrangé <=