[v2,13/13] qcrypto-luks: implement more rigorous header checking
diff mbox series

Message ID 20190826135103.22410-14-mlevitsk@redhat.com
State New
Headers show
Series
  • RFC crypto/luks: preparation for encryption key managment
Related show

Commit Message

Maxim Levitsky Aug. 26, 2019, 1:51 p.m. UTC
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(-)

Comments

Daniel P. Berrangé Sept. 6, 2019, 1:34 p.m. UTC | #1
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
Maxim Levitsky Sept. 12, 2019, 8:11 a.m. UTC | #2
On Fri, 2019-09-06 at 14:34 +0100, Daniel P. Berrangé wrote:
> 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.
I see that spec does allow for user defined number of stripes.

> 
> Also how about  checking    iters >= MIN_SLOT_KEY_ITERS
Also this is only a suggested minimum

> 
> > +
> > +        if (slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED &&
> > +                slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED) {
> 
> Align the two lines with (
Done
> 
> > +            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

Best regards,
	Maxim Levitsky

Patch
diff mbox series

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;
+    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;
+        }
+
+        if (slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED &&
+                slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED) {
+            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;