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

["Richard W.M. Jones" <rjones@redhat.com>]

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.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW