Re: [PATCH v2 3/7] crypto: add RFC5869 HKDF

[Eric Biggers <ebiggers@kernel.org>]

On Sun, Jan 24, 2021 at 03:03:28PM +0100, Stephan Müller wrote:
> RFC5869 specifies an extract and expand two-step key derivation
> function. The HKDF implementation is provided as a service function that
> operates on a caller-provided HMAC handle. The caller has to allocate
> the HMAC shash handle and then can invoke the HKDF service functions.
> The HKDF implementation ensures that the entire state is kept with the
> HMAC shash handle which implies that no additional state is required to
> be maintained by the HKDF implementation.
> 
> The extract function is invoked via the crypto_hkdf_extract call. RFC5869
> allows two optional parameters to be provided to the extract operation:
> the salt and input key material (IKM). Both are to be provided with the
> seed parameter where the salt is the first entry of the seed parameter
> and all subsequent entries are handled as IKM. If the caller intends to
> invoke the HKDF without salt, it has to provide a NULL/0 entry as first
> entry in seed.

The part about the "seed parameter" is outdated.

> The expand function is invoked via crypto_hkdf_expand and can be
> invoked multiple times. This function allows the caller to provide a
> context for the key derivation operation. As specified in RFC5869, it is
> optional. In case such context is not provided, the caller must provide
> NULL / 0 for the info / info_nvec parameters.

This commit message doesn't actually mention of *why* this patch is useful.  Is
it because there are going to be more uses of HKDF in the kernel besides the one
in fs/crypto/?  If so, what are those planned uses?

> diff --git a/crypto/Kconfig b/crypto/Kconfig
> index 9f375c2350f5..661287d7283b 100644
> --- a/crypto/Kconfig
> +++ b/crypto/Kconfig
> @@ -1862,6 +1862,13 @@ config CRYPTO_JITTERENTROPY
>  	  random numbers. This Jitterentropy RNG registers with
>  	  the kernel crypto API and can be used by any caller.
>  
> +config CRYPTO_HKDF
> +	tristate "Extract and Expand HKDF (RFC 5869)"
> +	select CRYPTO_HASH
> +	help
> +	  Enable the extract and expand key derivation function compliant
> +	  to RFC 5869.

This is just a library function, so it shouldn't be user-selectable.
It should just be selected by the kconfig options that need it.

> diff --git a/crypto/hkdf.c b/crypto/hkdf.c
> new file mode 100644
> index 000000000000..8e80eca202e7
> --- /dev/null
> +++ b/crypto/hkdf.c
> @@ -0,0 +1,199 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * HMAC-based Extract-and-Expand Key Derivation Function (conformant to RFC5869)
> + *
> + * Copyright (C) 2020, Stephan Mueller <smueller@chronox.de>
> + */
> +
> +#include <linux/module.h>
> +#include <crypto/hkdf.h>
> +#include <crypto/internal/kdf_selftest.h>
> +
> +/*
> + * HKDF expand phase
> + */
> +int crypto_hkdf_expand(struct crypto_shash *kmd,
> +		       const struct kvec *info, unsigned int info_nvec,
> +		       u8 *dst, unsigned int dlen)
> +{
> +	SHASH_DESC_ON_STACK(desc, kmd);
> +	const unsigned int h = crypto_shash_digestsize(kmd), dlen_orig = dlen;
> +	unsigned int i;
> +	int err = 0;
> +	u8 *dst_orig = dst;
> +	const u8 *prev = NULL;
> +	u8 ctr = 0x01;
> +
> +	if (dlen > h * 255)
> +		return -EINVAL;
> +
> +	desc->tfm = kmd;
> +
> +	/* T(1) and following */
> +	while (dlen) {
> +		err = crypto_shash_init(desc);
> +		if (err)
> +			goto out;
> +
> +		if (prev) {
> +			err = crypto_shash_update(desc, prev, h);
> +			if (err)
> +				goto out;
> +		}
> +
> +		for (i = 0; i < info_nvec; i++) {
> +			err = crypto_shash_update(desc, info[i].iov_base,
> +						  info[i].iov_len);
> +			if (err)
> +				goto out;
> +		}
> +
> +		if (dlen < h) {
> +			u8 tmpbuffer[HASH_MAX_DIGESTSIZE];
> +
> +			err = crypto_shash_finup(desc, &ctr, 1, tmpbuffer);
> +			if (err)
> +				goto out;
> +			memcpy(dst, tmpbuffer, dlen);
> +			memzero_explicit(tmpbuffer, h);
> +			goto out;
> +		}
> +
> +		err = crypto_shash_finup(desc, &ctr, 1, dst);
> +		if (err)
> +			goto out;
> +
> +		prev = dst;
> +		dst += h;
> +		dlen -= h;
> +		ctr++;
> +	}
> +
> +out:
> +	if (err)
> +		memzero_explicit(dst_orig, dlen_orig);
> +	shash_desc_zero(desc);
> +	return err;
> +}
> +EXPORT_SYMBOL(crypto_hkdf_expand);

EXPORT_SYMBOL_GPL?

> +
> +/*
> + * HKDF extract phase.
> + */
> +int crypto_hkdf_extract(struct crypto_shash *kmd,
> +			const u8 *salt, size_t saltlen,
> +			const u8 *ikm, size_t ikmlen)
> +{
> +	SHASH_DESC_ON_STACK(desc, kmd);
> +	unsigned int h = crypto_shash_digestsize(kmd);
> +	int err;
> +	static const u8 null_salt[HASH_MAX_DIGESTSIZE] = { 0 };
> +	u8 prk[HASH_MAX_DIGESTSIZE];
> +
> +	desc->tfm = kmd;
> +
> +	if (salt && saltlen) {

Checking 'salt && saltlen' like this is poor practice, as then if people
accidentally use salt == NULL && saltlen != 0 when they intended to use a salt,
the bug won't be detected.  Just doing 'if (saltlen)' would be better.

> +
> +	/* Extract the PRK */
> +	err = crypto_shash_init(desc);
> +	if (err)
> +		goto err;
> +
> +	err = crypto_shash_finup(desc, ikm, ikmlen, prk);
> +	if (err)
> +		goto err;

This should use crypto_shash_digest() instead of crypto_shash_init() +
crypto_shash_finup().

> +/* Test vectors from RFC 5869 appendix A */
> +static const struct kdf_testvec hkdf_hmac_sha256_tv_template[] = {
> +	{
> +		/* salt */
> +		.key = "\x00\x01\x02\x03\x04\x05\x06\x07"
> +		       "\x08\x09\x0a\x0b\x0c",
> +		.keylen  = 13,
> +		.ikm = "\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b"
> +		       "\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b"
> +		       "\x0b\x0b\x0b\x0b\x0b\x0b",
> +		.ikmlen = 22,
> +		.info = {
> +			.iov_base = "\xf0\xf1\xf2\xf3\xf4\xf5\xf6\xf7"
> +				    "\xf8\xf9",
> +			.iov_len  = 10
> +		},
> +		.expected	  = "\x3c\xb2\x5f\x25\xfa\xac\xd5\x7a"
> +				    "\x90\x43\x4f\x64\xd0\x36\x2f\x2a"
> +				    "\x2d\x2d\x0a\x90\xcf\x1a\x5a\x4c"
> +				    "\x5d\xb0\x2d\x56\xec\xc4\xc5\xbf"
> +				    "\x34\x00\x72\x08\xd5\xb8\x87\x18"
> +				    "\x58\x65",
> +		.expectedlen	  = 42
> +	}, {
> +		/* salt */
> +		.key = NULL,
> +		.keylen  = 0,
> +		.ikm = "\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b"
> +		       "\x0b\x0b\x0b\x0b\x0b\x0b\x0b\x0b"
> +		       "\x0b\x0b\x0b\x0b\x0b\x0b",
> +		.ikmlen  = 22,
> +		.info = {
> +			.iov_base = NULL,
> +			.iov_len  = 0
> +		},
> +		.expected	  = "\x8d\xa4\xe7\x75\xa5\x63\xc1\x8f"
> +				    "\x71\x5f\x80\x2a\x06\x3c\x5a\x31"
> +				    "\xb8\xa1\x1f\x5c\x5e\xe1\x87\x9e"
> +				    "\xc3\x45\x4e\x5f\x3c\x73\x8d\x2d"
> +				    "\x9d\x20\x13\x95\xfa\xa4\xb6\x1a"
> +				    "\x96\xc8",
> +		.expectedlen	  = 42
> +	}
> +};

The case of multiple entries in the 'info' kvec isn't being tested.

Also, it is confusing having both 'key' and 'ikm'.  'key' really should be
'salt'.

> diff --git a/include/crypto/hkdf.h b/include/crypto/hkdf.h
> new file mode 100644
> index 000000000000..c6989f786860
> --- /dev/null
> +++ b/include/crypto/hkdf.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/*
> + * Copyright (C) 2020, Stephan Mueller <smueller@chronox.de>
> + */
> +
> +#ifndef _CRYPTO_HKDF_H
> +#define _CRYPTO_HKDF_H
> +
> +#include <crypto/hash.h>
> +#include <linux/uio.h>
> +
> +/**
> + * RFC 5869 HKDF expand operation
> + *
> + * @kmd Keyed message digest whose key was set with crypto_hkdf_extract
> + * @info optional context and application specific information - this may be
> + *	 NULL
> + * @info_vec number of optional context/application specific information entries
> + * @dst destination buffer that the caller already allocated
> + * @dlen length of the destination buffer - the KDF derives that amount of
> + *	 bytes.
> + *
> + * @return 0 on success, < 0 on error
> + */
> +int crypto_hkdf_expand(struct crypto_shash *kmd,
> +		       const struct kvec *info, unsigned int info_nvec,
> +		       u8 *dst, unsigned int dlen);
> +
> +/**
> + * RFC 5869 HKDF extract operation
> + *
> + * @kmd Keyed message digest allocated by the caller. The key should not have
> + *	been set.
> + * @salt The salt used for the KDF. It is permissible to provide NULL as salt
> + *	 which implies that the default salt is used.
> + * @saltlen Length of the salt buffer.
> + * @ikm The input key material (IKM). It is permissible to provide NULL as IKM.
> + * @ikmlen Length of the IKM buffer
> + * @seed_nvec number of seed entries (must be at least 1)

seed_nvec no longer exists.

- Eric