Re: [PATCH 06/11] crypto: check that LUKS PBKDF2 iterations count is non-zero

[Richard W.M. Jones]

On Tue, Sep 06, 2022 at 09:41:42AM +0100, Daniel P. Berrangé wrote:
> Both the master key and key slot passphrases are run through the PBKDF2
> algorithm. The iterations count is expected to be generally very large
> (many 10's or 100's of 1000s). It is hard to define a low level cutoff,
> but we can certainly say that iterations count should be non-zero. A
> zero count likely indicates an initialization mistake so reject it.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  crypto/block-luks.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index e6ee8506b2..254490c256 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -579,6 +579,11 @@ qcrypto_block_luks_check_header(const QCryptoBlockLUKS 
> *luks, Error **errp)
>          return -1;
>      }
>  
> +    if (luks->header.master_key_iterations == 0) {
> +        error_setg(errp, "LUKS key iteration count is zero");
> +        return -1;
> +    }
> +
>      /* Check all keyslots for corruption  */
>      for (i = 0 ; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; i++) {
>  
> @@ -602,6 +607,12 @@ qcrypto_block_luks_check_header(const QCryptoBlockLUKS 
> *luks, Error **errp)
>              return -1;
>          }
>  
> +        if (slot1->active == QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED &&
> +            slot1->iterations == 0) {
> +            error_setg(errp, "Keyslot %zu iteration count is zero", i);
> +            return -1;
> +        }
> +
>          if (start1 < DIV_ROUND_UP(QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET,
>                                    QCRYPTO_BLOCK_LUKS_SECTOR_SIZE)) {
>              error_setg(errp,

Equivalent checks were missing in nbdkit - I've added them.

I wonder if there's a problem that a very large number here would
cause long delays opening the device.  In general it's not very clear
to me if the aim is to prevent malicious LUKS input, or if we're just
trying to sanity check the device hasn't been corrupted or improperly
prepared.  The test above is the latter, I think.

Nevertheless as this is an improvement over the current situation:

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
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html