diff mbox series

[01/11] crypto: sanity check that LUKS header strings are NUL-terminated

Message ID 20220906084147.1423045-2-berrange@redhat.com (mailing list archive)
State New, archived
Headers show
Series crypto: improve robustness of LUKS metadata validation | expand

Commit Message

Daniel P. Berrangé Sept. 6, 2022, 8:41 a.m. UTC 
The LUKS spec requires that header strings are NUL-terminated, and our
code relies on that. Protect against maliciously crafted headers by
adding validation.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/block-luks.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Richard W.M. Jones Sept. 6, 2022, 9:30 a.m. UTC | #1 
On Tue, Sep 06, 2022 at 09:41:37AM +0100, Daniel P. Berrangé wrote:
> The LUKS spec requires that header strings are NUL-terminated, and our
> code relies on that. Protect against maliciously crafted headers by
> adding validation.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  crypto/block-luks.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index f62be6836b..27d1b34c1d 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -554,6 +554,24 @@ qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp)
>          return -1;
>      }
>  
> +    if (!memchr(luks->header.cipher_name, '\0',
> +                sizeof(luks->header.cipher_name))) {
> +        error_setg(errp, "LUKS header cipher name is not NUL terminated");
> +        return -1;
> +    }
> +
> +    if (!memchr(luks->header.cipher_mode, '\0',
> +                sizeof(luks->header.cipher_mode))) {
> +        error_setg(errp, "LUKS header cipher mode is not NUL terminated");
> +        return -1;
> +    }
> +
> +    if (!memchr(luks->header.hash_spec, '\0',
> +                sizeof(luks->header.hash_spec))) {
> +        error_setg(errp, "LUKS header hash spec is not NUL terminated");
> +        return -1;
> +    }
> +
>      /* Check all keyslots for corruption  */
>      for (i = 0 ; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; i++) {

I think this was the error I originally wrote to you about, and I
think it's the most important fix because non-terminated strings seem
(possibly) exploitable.

FWIW nbdkit does this which is slightly different:

  char cipher_name[33], cipher_mode[33], hash_spec[33];

  /* Copy the header fields locally and ensure they are \0 terminated. */
  memcpy (cipher_name, h->phdr.cipher_name, 32);
  cipher_name[32] = 0;
  memcpy (cipher_mode, h->phdr.cipher_mode, 32);
  cipher_mode[32] = 0;
  memcpy (hash_spec, h->phdr.hash_spec, 32);
  hash_spec[32] = 0;

Anyway the change above looks good so:

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

Rich.
diff mbox series

Patch

diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index f62be6836b..27d1b34c1d 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -554,6 +554,24 @@  qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp)
         return -1;
     }
 
+    if (!memchr(luks->header.cipher_name, '\0',
+                sizeof(luks->header.cipher_name))) {
+        error_setg(errp, "LUKS header cipher name is not NUL terminated");
+        return -1;
+    }
+
+    if (!memchr(luks->header.cipher_mode, '\0',
+                sizeof(luks->header.cipher_mode))) {
+        error_setg(errp, "LUKS header cipher mode is not NUL terminated");
+        return -1;
+    }
+
+    if (!memchr(luks->header.hash_spec, '\0',
+                sizeof(luks->header.hash_spec))) {
+        error_setg(errp, "LUKS header hash spec is not NUL terminated");
+        return -1;
+    }
+
     /* Check all keyslots for corruption  */
     for (i = 0 ; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; i++) {