Re: [PATCH v3 2/4] crypto: add SP800-108 counter key derivation function

[Stephan Mueller <smueller@chronox.de>]

Am Mittwoch, 17. November 2021, 20:11:03 CET schrieb Eric Biggers:

Hi Eric,

thanks for your comments.

> On Mon, Nov 15, 2021 at 09:43:13AM +0100, Stephan Müller wrote:
> > SP800-108 defines three KDFs - this patch provides the counter KDF
> > implementation.
> > 
> > The KDF is implemented as a service function where the caller has to
> > maintain the hash / HMAC state. Apart from this hash/HMAC state, no
> > additional state is required to be maintained by either the caller or
> > the KDF implementation.
> > 
> > The key for the KDF is set with the crypto_kdf108_setkey function which
> > is intended to be invoked before the caller requests a key derivation
> > operation via crypto_kdf108_ctr_generate.
> > 
> > SP800-108 allows the use of either a HMAC or a hash as crypto primitive
> > for the KDF. When a HMAC primtive is intended to be used,
> > crypto_kdf108_setkey must be used to set the HMAC key. Otherwise, for a
> > hash crypto primitve crypto_kdf108_ctr_generate can be used immediately
> > after allocating the hash handle.
> > 
> > Signed-off-by: Stephan Mueller <smueller@chronox.de>
> > ---
> > 
> >  crypto/Kconfig                |   7 ++
> >  crypto/Makefile               |   5 ++
> >  crypto/kdf_sp800108.c         | 149 ++++++++++++++++++++++++++++++++++
> >  include/crypto/kdf_sp800108.h |  61 ++++++++++++++
> >  4 files changed, 222 insertions(+)
> >  create mode 100644 crypto/kdf_sp800108.c
> >  create mode 100644 include/crypto/kdf_sp800108.h
> > 
> > diff --git a/crypto/Kconfig b/crypto/Kconfig
> > index 285f82647d2b..09c393a57b58 100644
> > --- a/crypto/Kconfig
> > +++ b/crypto/Kconfig
> > @@ -1845,6 +1845,13 @@ config CRYPTO_JITTERENTROPY
> > 
> >  	  random numbers. This Jitterentropy RNG registers with
> >  	  the kernel crypto API and can be used by any caller.
> > 
> > +config CRYPTO_KDF800108_CTR
> > +	tristate "Counter KDF (SP800-108)"
> > +	select CRYPTO_HASH
> > +	help
> > +	  Enable the key derivation function in counter mode compliant to
> > +	  SP800-108.
> 
> These are just some library functions, so they shouldn't be user-selectable.

Ok, I will remove the user-visible entry in the kernel configuration.

> > +/*
> > + * The seeding of the KDF
> > + */
> > +int crypto_kdf108_setkey(struct crypto_shash *kmd,
> > +			 const u8 *key, size_t keylen,
> > +			 const u8 *ikm, size_t ikmlen)
> > +{
> > +	unsigned int ds = crypto_shash_digestsize(kmd);
> > +
> > +	/* SP800-108 does not support IKM */
> > +	if (ikm || ikmlen)
> > +		return -EINVAL;
> 
> Why have the ikm parameter if it's not supported?

The original idea is that we have a common function declaration for SP800-108 
and HKDF. I am still thinking that in the long run, a KDF template support may 
make sense. In this case, a common function declaration would be needed for 
all KDF implementations.

Furthermore, the test code can be shared between the different KDFs when we 
allow the ikm/ikmlen parameter for this function.
> 
> > +	/*
> > +	 * We require that we operate on a MAC -- if we do not operate on a
> > +	 * MAC, this function returns an error.
> > +	 */
> > +	return crypto_shash_setkey(kmd, key, keylen);
> > +}
> > +EXPORT_SYMBOL(crypto_kdf108_setkey);
> 
> Well, crypto_shash_setkey() will succeed if the hash algorithm takes a
> "key". That doesn't necessarily mean that it's a MAC.	It could be crc32 or
> xxhash64, for example; those interpret the "key" as the initial value.

Agreed. But I am not sure a check in this regard would be needed considering 
that this KDF is only an internal service function.

I have updated the comment accordingly.
> 
> > +static int __init crypto_kdf108_init(void)
> > +{
> > +	int ret = kdf_test(&kdf_ctr_hmac_sha256_tv_template[0], 
"hmac(sha256)",
> > +			   crypto_kdf108_setkey, crypto_kdf108_ctr_generate);
> > +
> > +	if (ret)
> > +		pr_warn("alg: self-tests for CTR-KDF (hmac(sha256)) failed 
(rc=%d)\n",
> > +			ret);
> 
> This should be a WARN() since it indicates a kernel bug.

Changed. Considering that the test result behavior should be identical to 
testmgr.c, I have added also the panic() call in case of fips_enabled.

Thanks a lot for your review.
> 
> - Eric


Ciao
Stephan