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

Message ID 20190814202219.1870-7-mlevitsk@redhat.com
State New
Headers show
Series
  • RFC: luks/encrypted qcow2 key management
Related show

Commit Message

Maxim Levitsky Aug. 14, 2019, 8:22 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 doens't really matter.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 crypto/block-luks.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

Comments

Daniel P. Berrangé Aug. 22, 2019, 11:04 a.m. UTC | #1
On Wed, Aug 14, 2019 at 11:22:12PM +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 doens't really matter.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  crypto/block-luks.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index 336e633df4..1997e92fe1 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -551,6 +551,8 @@ static int
>  qcrypto_block_luks_check_header(QCryptoBlockLUKS *luks, Error **errp)
>  {
>      int ret;
> +    int i, j;
> +
>  
>      if (memcmp(luks->header.magic, qcrypto_block_luks_magic,
>                 QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) {
> @@ -566,6 +568,46 @@ qcrypto_block_luks_check_header(QCryptoBlockLUKS *luks, Error **errp)
>          goto fail;
>      }
>  
> +    /* Check all keyslots for corruption  */
> +    for (i = 0 ; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; i++) {
> +
> +        QCryptoBlockLUKSKeySlot *slot1 = &luks->header.key_slots[i];
> +        uint start1 = slot1->key_offset;
> +        uint len1 = splitkeylen_sectors(luks, slot1->stripes);

Using 'uint' is not normal QEMU style.

Either use 'unsigned int'  or if a specific size is needed
then one of the 'guintNN' types from glib.

This applies elsewhere in this patch series too, but
I'll only comment here & let you find the other cases.

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

Redundant blank line

> +            error_setg(errp, "Keyslot %i is corrupted", i);

I'd do a separate check for stripes and active fields, and then give a
specific error message for each. That way if this does ever trigger
in practice will immediately understand which check failed.

Also using '%d' rather than '%i' is more common convention


> +            ret = -EINVAL;
> +            goto fail;
> +        }
> +
> +        if (start1 + len1 > luks->header.payload_offset) {
> +            error_setg(errp,
> +                       "Keyslot %i is overlapping with the encrypted payload",
> +                       i);
> +            ret = -EINVAL;
> +            goto fail;
> +        }
> +
> +        for (j = i + 1 ; j < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; j++) {
> +

Redundant blank

> +            QCryptoBlockLUKSKeySlot *slot2 = &luks->header.key_slots[j];
> +            uint start2 = slot2->key_offset;
> +            uint len2 = splitkeylen_sectors(luks, slot2->stripes);
> +
> +            if (start1 + len1 > start2 && start2 + len2 > start1) {
> +                error_setg(errp,
> +                           "Keyslots %i and %i are overlapping in the header",

%d

> +                           i, j);
> +                ret = -EINVAL;
> +                goto fail;
> +            }
> +        }
> +
> +    }
>      return 0;
>  fail:
>      return ret;
> -- 
> 2.17.2
> 

Regards,
Daniel
Maxim Levitsky Aug. 25, 2019, 3:40 p.m. UTC | #2
On Thu, 2019-08-22 at 12:04 +0100, Daniel P. Berrangé wrote:
> On Wed, Aug 14, 2019 at 11:22:12PM +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 doens't really matter.
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  crypto/block-luks.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> > 
> > diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> > index 336e633df4..1997e92fe1 100644
> > --- a/crypto/block-luks.c
> > +++ b/crypto/block-luks.c
> > @@ -551,6 +551,8 @@ static int
> >  qcrypto_block_luks_check_header(QCryptoBlockLUKS *luks, Error **errp)
> >  {
> >      int ret;
> > +    int i, j;
> > +
> >  
> >      if (memcmp(luks->header.magic, qcrypto_block_luks_magic,
> >                 QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) {
> > @@ -566,6 +568,46 @@ qcrypto_block_luks_check_header(QCryptoBlockLUKS *luks, Error **errp)
> >          goto fail;
> >      }
> >  
> > +    /* Check all keyslots for corruption  */
> > +    for (i = 0 ; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; i++) {
> > +
> > +        QCryptoBlockLUKSKeySlot *slot1 = &luks->header.key_slots[i];
> > +        uint start1 = slot1->key_offset;
> > +        uint len1 = splitkeylen_sectors(luks, slot1->stripes);
> 
> Using 'uint' is not normal QEMU style.
> 
> Either use 'unsigned int'  or if a specific size is needed
> then one of the 'guintNN' types from glib.
> 
> This applies elsewhere in this patch series too, but
> I'll only comment here & let you find the other cases.

Fixed. Sorry for the noise.

> 
> > +
> > +        if (slot1->stripes == 0 ||
> > +                (slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED &&
> > +                slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED)) {
> > +
> 
> Redundant blank line
Fixed
> 
> > +            error_setg(errp, "Keyslot %i is corrupted", i);
> 
> I'd do a separate check for stripes and active fields, and then give a
> specific error message for each. That way if this does ever trigger
> in practice will immediately understand which check failed.
> 
> Also using '%d' rather than '%i' is more common convention
Done.
> 
> 
> > +            ret = -EINVAL;
> > +            goto fail;
> > +        }
> > +
> > +        if (start1 + len1 > luks->header.payload_offset) {
> > +            error_setg(errp,
> > +                       "Keyslot %i is overlapping with the encrypted payload",
> > +                       i);
> > +            ret = -EINVAL;
> > +            goto fail;
> > +        }
> > +
> > +        for (j = i + 1 ; j < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; j++) {
> > +
> 
> Redundant blank
> 
> > +            QCryptoBlockLUKSKeySlot *slot2 = &luks->header.key_slots[j];
> > +            uint start2 = slot2->key_offset;
> > +            uint len2 = splitkeylen_sectors(luks, slot2->stripes);
> > +
> > +            if (start1 + len1 > start2 && start2 + len2 > start1) {
> > +                error_setg(errp,
> > +                           "Keyslots %i and %i are overlapping in the header",
> 
> %d
Fixed.
> 
> > +                           i, j);
> > +                ret = -EINVAL;
> > +                goto fail;
> > +            }
> > +        }
> > +
> > +    }
> >      return 0;
> >  fail:
> >      return ret;
> > -- 
> > 2.17.2
> > 
> 
> Regards,
> Daniel


Best regards,
	Maxim Levitsky
Maxim Levitsky Aug. 25, 2019, 4:08 p.m. UTC | #3
On Sun, 2019-08-25 at 18:40 +0300, Maxim Levitsky wrote:
> On Thu, 2019-08-22 at 12:04 +0100, Daniel P. Berrangé wrote:
> > On Wed, Aug 14, 2019 at 11:22:12PM +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 doens't really matter.
> > > 
> > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > > ---
> > >  crypto/block-luks.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 42 insertions(+)
> > > 
> > > diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> > > index 336e633df4..1997e92fe1 100644
> > > --- a/crypto/block-luks.c
> > > +++ b/crypto/block-luks.c
> > > @@ -551,6 +551,8 @@ static int
> > >  qcrypto_block_luks_check_header(QCryptoBlockLUKS *luks, Error **errp)
> > >  {
> > >      int ret;
> > > +    int i, j;
> > > +
> > >  
> > >      if (memcmp(luks->header.magic, qcrypto_block_luks_magic,
> > >                 QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) {
> > > @@ -566,6 +568,46 @@ qcrypto_block_luks_check_header(QCryptoBlockLUKS *luks, Error **errp)
> > >          goto fail;
> > >      }
> > >  
> > > +    /* Check all keyslots for corruption  */
> > > +    for (i = 0 ; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; i++) {
> > > +
> > > +        QCryptoBlockLUKSKeySlot *slot1 = &luks->header.key_slots[i];
> > > +        uint start1 = slot1->key_offset;
> > > +        uint len1 = splitkeylen_sectors(luks, slot1->stripes);
> > 
> > Using 'uint' is not normal QEMU style.
> > 
> > Either use 'unsigned int'  or if a specific size is needed
> > then one of the 'guintNN' types from glib.
> > 
> > This applies elsewhere in this patch series too, but
> > I'll only comment here & let you find the other cases.
> 
> Fixed. Sorry for the noise.
> 
> > 
> > > +
> > > +        if (slot1->stripes == 0 ||
> > > +                (slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED &&
> > > +                slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED)) {
> > > +
> > 
> > Redundant blank line
> 
> Fixed
> > 
> > > +            error_setg(errp, "Keyslot %i is corrupted", i);
> > 
> > I'd do a separate check for stripes and active fields, and then give a
> > specific error message for each. That way if this does ever trigger
> > in practice will immediately understand which check failed.
> > 
> > Also using '%d' rather than '%i' is more common convention
> 
> Done.

Note that I switched i,j to be size_t since you said that you prefer this,
and to print this I apparently need %lu.


[...]

Best regards,
	Maxim Levitsky
Eric Blake Aug. 26, 2019, 1:31 p.m. UTC | #4
On 8/25/19 11:08 AM, Maxim Levitsky wrote:

>>> I'd do a separate check for stripes and active fields, and then give a
>>> specific error message for each. That way if this does ever trigger
>>> in practice will immediately understand which check failed.
>>>
>>> Also using '%d' rather than '%i' is more common convention
>>
>> Done.
> 
> Note that I switched i,j to be size_t since you said that you prefer this,
> and to print this I apparently need %lu.

Actually, for size_t, you need %zu. %lu/size_t will cause warnings on
32-bit platforms.
Maxim Levitsky Aug. 26, 2019, 1:39 p.m. UTC | #5
On Mon, 2019-08-26 at 08:31 -0500, Eric Blake wrote:
> On 8/25/19 11:08 AM, Maxim Levitsky wrote:
> 
> > > > I'd do a separate check for stripes and active fields, and then give a
> > > > specific error message for each. That way if this does ever trigger
> > > > in practice will immediately understand which check failed.
> > > > 
> > > > Also using '%d' rather than '%i' is more common convention
> > > 
> > > Done.
> > 
> > Note that I switched i,j to be size_t since you said that you prefer this,
> > and to print this I apparently need %lu.
> 
> Actually, for size_t, you need %zu. %lu/size_t will cause warnings on
> 32-bit platforms.
> 
> 
Thank you!
I have read something like that on the internet, but I wondered,
what actually is the most portable way.

Best regards,
	Maxim Levitsky
Daniel P. Berrangé Aug. 27, 2019, 8:56 a.m. UTC | #6
On Sun, Aug 25, 2019 at 07:08:00PM +0300, Maxim Levitsky wrote:
> On Sun, 2019-08-25 at 18:40 +0300, Maxim Levitsky wrote:
> > On Thu, 2019-08-22 at 12:04 +0100, Daniel P. Berrangé wrote:
> > > On Wed, Aug 14, 2019 at 11:22:12PM +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 doens't really matter.
> > > > 
> > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > > > ---
> > > >  crypto/block-luks.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 42 insertions(+)
> > > > 
> > > > diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> > > > index 336e633df4..1997e92fe1 100644
> > > > --- a/crypto/block-luks.c
> > > > +++ b/crypto/block-luks.c
> > > > @@ -551,6 +551,8 @@ static int
> > > >  qcrypto_block_luks_check_header(QCryptoBlockLUKS *luks, Error **errp)
> > > >  {
> > > >      int ret;
> > > > +    int i, j;
> > > > +
> > > >  
> > > >      if (memcmp(luks->header.magic, qcrypto_block_luks_magic,
> > > >                 QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) {
> > > > @@ -566,6 +568,46 @@ qcrypto_block_luks_check_header(QCryptoBlockLUKS *luks, Error **errp)
> > > >          goto fail;
> > > >      }
> > > >  
> > > > +    /* Check all keyslots for corruption  */
> > > > +    for (i = 0 ; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; i++) {
> > > > +
> > > > +        QCryptoBlockLUKSKeySlot *slot1 = &luks->header.key_slots[i];
> > > > +        uint start1 = slot1->key_offset;
> > > > +        uint len1 = splitkeylen_sectors(luks, slot1->stripes);
> > > 
> > > Using 'uint' is not normal QEMU style.
> > > 
> > > Either use 'unsigned int'  or if a specific size is needed
> > > then one of the 'guintNN' types from glib.
> > > 
> > > This applies elsewhere in this patch series too, but
> > > I'll only comment here & let you find the other cases.
> > 
> > Fixed. Sorry for the noise.
> > 
> > > 
> > > > +
> > > > +        if (slot1->stripes == 0 ||
> > > > +                (slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED &&
> > > > +                slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED)) {
> > > > +
> > > 
> > > Redundant blank line
> > 
> > Fixed
> > > 
> > > > +            error_setg(errp, "Keyslot %i is corrupted", i);
> > > 
> > > I'd do a separate check for stripes and active fields, and then give a
> > > specific error message for each. That way if this does ever trigger
> > > in practice will immediately understand which check failed.
> > > 
> > > Also using '%d' rather than '%i' is more common convention
> > 
> > Done.
> 
> Note that I switched i,j to be size_t since you said that you prefer this,
> and to print this I apparently need %lu.

For size_t, you need %zu to be 32/64-bit agnostic  (ssize_t is %zd)


Regards,
Daniel

Patch
diff mbox series

diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 336e633df4..1997e92fe1 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -551,6 +551,8 @@  static int
 qcrypto_block_luks_check_header(QCryptoBlockLUKS *luks, Error **errp)
 {
     int ret;
+    int i, j;
+
 
     if (memcmp(luks->header.magic, qcrypto_block_luks_magic,
                QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) {
@@ -566,6 +568,46 @@  qcrypto_block_luks_check_header(QCryptoBlockLUKS *luks, Error **errp)
         goto fail;
     }
 
+    /* Check all keyslots for corruption  */
+    for (i = 0 ; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; i++) {
+
+        QCryptoBlockLUKSKeySlot *slot1 = &luks->header.key_slots[i];
+        uint start1 = slot1->key_offset;
+        uint len1 = splitkeylen_sectors(luks, slot1->stripes);
+
+        if (slot1->stripes == 0 ||
+                (slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED &&
+                slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED)) {
+
+            error_setg(errp, "Keyslot %i is corrupted", i);
+            ret = -EINVAL;
+            goto fail;
+        }
+
+        if (start1 + len1 > luks->header.payload_offset) {
+            error_setg(errp,
+                       "Keyslot %i is overlapping with the encrypted payload",
+                       i);
+            ret = -EINVAL;
+            goto fail;
+        }
+
+        for (j = i + 1 ; j < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; j++) {
+
+            QCryptoBlockLUKSKeySlot *slot2 = &luks->header.key_slots[j];
+            uint start2 = slot2->key_offset;
+            uint len2 = splitkeylen_sectors(luks, slot2->stripes);
+
+            if (start1 + len1 > start2 && start2 + len2 > start1) {
+                error_setg(errp,
+                           "Keyslots %i and %i are overlapping in the header",
+                           i, j);
+                ret = -EINVAL;
+                goto fail;
+            }
+        }
+
+    }
     return 0;
 fail:
     return ret;