Re: [Qemu-devel] [PATCH v2 03/17] crypto: add support for PBKDF2 algorithm

[Eric Blake]

On 01/20/2016 10:38 AM, Daniel P. Berrange wrote:
> The LUKS data format includes use of PBKDF2 (Password-Based
> Key Derivation Function). The Nettle library can provide
> an implementation of this, but we don't want code directly
> depending on a specific crypto library backend. Introduce
> a include/crypto/pbkdf.h header which defines a QEMU

'an include/...', or maybe 'a new include/...'?

> API for invoking PBKDK2. The initial implementations are
> backed by nettle & gcrypt, which are commonly available
> with distros shipping GNUTLS.
> 
> The test suite data is taken from the cryptsetup codebase
> under the LGPLv2.1+ license. This merely aims to verify
> that whatever backend we provide for this function in QEMU
> will comply with the spec.
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---

In addition to Fam's review,

> +++ b/crypto/pbkdf-gcrypt.c

> +int qcrypto_pbkdf2(QCryptoHashAlgorithm hash,
> +                   const uint8_t *key, size_t nkey,
> +                   const uint8_t *salt, size_t nsalt,
> +                   unsigned int iterations,
> +                   uint8_t *out, size_t nout,
> +                   Error **errp)
> +{
> +    static const int hash_map[QCRYPTO_HASH_ALG__MAX] = {
> +        [QCRYPTO_HASH_ALG_MD5] = GCRY_MD_MD5,
> +        [QCRYPTO_HASH_ALG_SHA1] = GCRY_MD_SHA1,
> +        [QCRYPTO_HASH_ALG_SHA256] = GCRY_MD_SHA256,
> +    };

If QCRYPTO_HASH_ gains future enum values, those elements of the array
will be 0-initialized.

> +    int ret;
> +
> +    if (hash > G_N_ELEMENTS(hash_map)) {
> +        error_setg(errp, "Unexpected hash algorithm %d", hash);
> +        return -1;
> +    }

This checks for beyond the bounds of the array, but not for an element
that was 0-initialized.  Is that a problem we need to worry about?

> +int qcrypto_pbkdf2(QCryptoHashAlgorithm hash,
> +                   const uint8_t *key, size_t nkey,
> +                   const uint8_t *salt, size_t nsalt,
> +                   unsigned int iterations,
> +                   uint8_t *out, size_t nout,
> +                   Error **errp);
> +
> +/**
> + * qcrypto_pbkdf2_count_iters:
> + * @hash: the hash algorithm to use
> + * @key: the user password / key
> + * @nkey: the length of @key in bytes
> + * @salt: a random salt
> + * @nsalt: length of @salt in bytes
> + * @errp: pointer to a NULL-initialized error object
> + *
> + * Time the PBKDF2 algorithm to determine how many
> + * iterations are required to derive an encryption
> + * key from a user password provided in @key in 1
> + * second of compute time. The result of this can
> + * be used as a the @iterations parameter of a later
> + * call to qcrypto_pbkdf2().

As machines get faster, will 2^31 still be enough, or do we want a
64-bit iterations counter?


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature