diff mbox series

[1/9] crypto,fs: Separate out hkdf_extract() and hkdf_expand()

Message ID 20240722142122.128258-2-hare@kernel.org
State New
Headers show
Series nvme: implement secure concatenation | expand

Commit Message

Hannes Reinecke July 22, 2024, 2:21 p.m. UTC
Separate out the HKDF functions into a separate file to make them
available to other callers.

Signed-off-by: Hannes Reinecke <hare@kernel.org>
Cc: Eric Biggers <ebiggers@kernel.org>
Cc: linux-crypto@vger.kernel.org
---
 crypto/Makefile       |   1 +
 crypto/hkdf.c         | 112 ++++++++++++++++++++++++++++++++++++++++++
 fs/crypto/hkdf.c      |  68 ++++---------------------
 include/crypto/hkdf.h |  18 +++++++
 4 files changed, 140 insertions(+), 59 deletions(-)
 create mode 100644 crypto/hkdf.c
 create mode 100644 include/crypto/hkdf.h

Comments

Eric Biggers July 23, 2024, 1:36 a.m. UTC | #1
On Mon, Jul 22, 2024 at 04:21:14PM +0200, Hannes Reinecke wrote:
> diff --git a/crypto/Makefile b/crypto/Makefile
> index edbbaa3ffef5..b77fc360f0ff 100644
> --- a/crypto/Makefile
> +++ b/crypto/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_CRYPTO_ECHAINIV) += echainiv.o
>  
>  crypto_hash-y += ahash.o
>  crypto_hash-y += shash.o
> +crypto_hash-y += hkdf.o
>  obj-$(CONFIG_CRYPTO_HASH2) += crypto_hash.o

This should go under a kconfig option CONFIG_CRYPTO_HKDF that is selected by the
users that need it.  That way the code will be built only when needed.

Including a self-test would also be desirable.

> diff --git a/crypto/hkdf.c b/crypto/hkdf.c
> new file mode 100644
> index 000000000000..22e343851c0b
> --- /dev/null
> +++ b/crypto/hkdf.c
> @@ -0,0 +1,112 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Implementation of HKDF ("HMAC-based Extract-and-Expand Key Derivation
> + * Function"), aka RFC 5869.  See also the original paper (Krawczyk 2010):
> + * "Cryptographic Extraction and Key Derivation: The HKDF Scheme".
> + *
> + * This is used to derive keys from the fscrypt master keys.

This is no longer in fs/crypto/, so the part about fscrypt should be removed.

> +/*
> + * HKDF consists of two steps:
> + *
> + * 1. HKDF-Extract: extract a pseudorandom key of length HKDF_HASHLEN bytes from
> + *    the input keying material and optional salt.

It doesn't make sense to refer to HKDF_HASHLEN here since it is specific to
fs/crypto/.

> +/* HKDF-Extract (RFC 5869 section 2.2), unsalted */
> +int hkdf_extract(struct crypto_shash *hmac_tfm, const u8 *ikm,
> +		 unsigned int ikmlen, u8 *prk)

Needs kerneldoc now that this is a library interface.

> +{
> +	unsigned int prklen = crypto_shash_digestsize(hmac_tfm);
> +	u8 *default_salt;
> +	int err;
> +
> +	default_salt = kzalloc(prklen, GFP_KERNEL);
> +	if (!default_salt)
> +		return -ENOMEM;

Now that this is a library interface, it should take the salt as a parameter,
and the users who want the default salt should explicitly specify that.  If we
only provide support for unsalted use, that might inadventently discourage the
use of a salt in future code.  As the function is named hkdf_extract(), people
might also overlook that it's unsalted and doesn't actually match the RFC's
definition of HKDF-Extract.

The use of kzalloc here is also inefficient, as the maximum length of a digest
is known (HKDF_HASHLEN in fs/crypto/ case, HASH_MAX_DIGESTSIZE in general).

> +	err = crypto_shash_setkey(hmac_tfm, default_salt, prklen);
> +	if (!err)
> +		err = crypto_shash_tfm_digest(hmac_tfm, ikm, ikmlen, prk);
> +
> +	kfree(default_salt);
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(hkdf_extract);
> +
> +/*
> + * HKDF-Expand (RFC 5869 section 2.3).
> + * This expands the pseudorandom key, which was already keyed into @hmac_tfm,
> + * into @okmlen bytes of output keying material parameterized by the
> + * application-specific @info of length @infolen bytes.
> + * This is thread-safe and may be called by multiple threads in parallel.
> + */
> +int hkdf_expand(struct crypto_shash *hmac_tfm,
> +		const u8 *info, unsigned int infolen,
> +		u8 *okm, unsigned int okmlen)

Needs kerneldoc now that this is a library interface.

> diff --git a/fs/crypto/hkdf.c b/fs/crypto/hkdf.c
> index 5a384dad2c72..9c2f9aca9412 100644
> --- a/fs/crypto/hkdf.c
> +++ b/fs/crypto/hkdf.c
>// SPDX-License-Identifier: GPL-2.0
>/*
> * Implementation of HKDF ("HMAC-based Extract-and-Expand Key Derivation
> * Function"), aka RFC 5869.  See also the original paper (Krawczyk 2010):
> * "Cryptographic Extraction and Key Derivation: The HKDF Scheme".
> *
> * This is used to derive keys from the fscrypt master keys.
> *
> * Copyright 2019 Google LLC
> */

The above file comment should be adjusted now that this file doesn't contain the
actual HKDF implementation.

> @@ -118,61 +105,24 @@ int fscrypt_hkdf_expand(const struct fscrypt_hkdf *hkdf, u8 context,
>  			u8 *okm, unsigned int okmlen)
>  {
>  	SHASH_DESC_ON_STACK(desc, hkdf->hmac_tfm);
> -	u8 prefix[9];
> -	unsigned int i;
> +	u8 *prefix;
>  	int err;
> -	const u8 *prev = NULL;
> -	u8 counter = 1;
> -	u8 tmp[HKDF_HASHLEN];
>  
>  	if (WARN_ON_ONCE(okmlen > 255 * HKDF_HASHLEN))
>  		return -EINVAL;
>  
> +	prefix = kzalloc(okmlen + 9, GFP_KERNEL);
> +	if (!prefix)
> +		return -ENOMEM;
>  	desc->tfm = hkdf->hmac_tfm;
>  
>  	memcpy(prefix, "fscrypt\0", 8);
>  	prefix[8] = context;
> +	memcpy(prefix + 9, info, infolen);

This makes the variable called 'prefix' no longer be the prefix, but rather the
full info string.  A better name for it would be 'full_info'.

Also, it's being allocated with the wrong length.  It should be 9 + infolen.

> +	err = hkdf_expand(hkdf->hmac_tfm, prefix, infolen + 8,
> +			  okm, okmlen);
> +	kfree(prefix);

kfree_sensitive()

> diff --git a/include/crypto/hkdf.h b/include/crypto/hkdf.h
> new file mode 100644
> index 000000000000..bf06c080d7ed
> --- /dev/null
> +++ b/include/crypto/hkdf.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * HKDF: HMAC-based Key Derivation Function (HKDF), RFC 5869
> + *
> + * Extracted from fs/crypto/hkdf.c, which has
> + * Copyright 2019 Google LLC
> + */

If this is keeping the copyright of fs/crypto/hkdf.c, the license needs to stay
the same (GPL-2.0, not GPL-2.0-or-later).

- Eric
Hannes Reinecke July 23, 2024, 6:24 a.m. UTC | #2
On 7/23/24 03:36, Eric Biggers wrote:
> On Mon, Jul 22, 2024 at 04:21:14PM +0200, Hannes Reinecke wrote:
>> diff --git a/crypto/Makefile b/crypto/Makefile
>> index edbbaa3ffef5..b77fc360f0ff 100644
>> --- a/crypto/Makefile
>> +++ b/crypto/Makefile
>> @@ -29,6 +29,7 @@ obj-$(CONFIG_CRYPTO_ECHAINIV) += echainiv.o
>>   
>>   crypto_hash-y += ahash.o
>>   crypto_hash-y += shash.o
>> +crypto_hash-y += hkdf.o
>>   obj-$(CONFIG_CRYPTO_HASH2) += crypto_hash.o
> 
> This should go under a kconfig option CONFIG_CRYPTO_HKDF that is selected by the
> users that need it.  That way the code will be built only when needed.
> 
Okay.

> Including a self-test would also be desirable.
> 
First time for me, but ok.

>> diff --git a/crypto/hkdf.c b/crypto/hkdf.c
>> new file mode 100644
>> index 000000000000..22e343851c0b
>> --- /dev/null
>> +++ b/crypto/hkdf.c
>> @@ -0,0 +1,112 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Implementation of HKDF ("HMAC-based Extract-and-Expand Key Derivation
>> + * Function"), aka RFC 5869.  See also the original paper (Krawczyk 2010):
>> + * "Cryptographic Extraction and Key Derivation: The HKDF Scheme".
>> + *
>> + * This is used to derive keys from the fscrypt master keys.
> 
> This is no longer in fs/crypto/, so the part about fscrypt should be removed.
> 
Will be removing this line.

>> +/*
>> + * HKDF consists of two steps:
>> + *
>> + * 1. HKDF-Extract: extract a pseudorandom key of length HKDF_HASHLEN bytes from
>> + *    the input keying material and optional salt.
> 
> It doesn't make sense to refer to HKDF_HASHLEN here since it is specific to
> fs/crypto/.
> 
Ok.

>> +/* HKDF-Extract (RFC 5869 section 2.2), unsalted */
>> +int hkdf_extract(struct crypto_shash *hmac_tfm, const u8 *ikm,
>> +		 unsigned int ikmlen, u8 *prk)
> 
> Needs kerneldoc now that this is a library interface.
> 
Indeed, will be updating the comment.

>> +{
>> +	unsigned int prklen = crypto_shash_digestsize(hmac_tfm);
>> +	u8 *default_salt;
>> +	int err;
>> +
>> +	default_salt = kzalloc(prklen, GFP_KERNEL);
>> +	if (!default_salt)
>> +		return -ENOMEM;
> 
> Now that this is a library interface, it should take the salt as a parameter,
> and the users who want the default salt should explicitly specify that.  If we
> only provide support for unsalted use, that might inadventently discourage the
> use of a salt in future code.  As the function is named hkdf_extract(), people
> might also overlook that it's unsalted and doesn't actually match the RFC's
> definition of HKDF-Extract.
> 
> The use of kzalloc here is also inefficient, as the maximum length of a digest
> is known (HKDF_HASHLEN in fs/crypto/ case, HASH_MAX_DIGESTSIZE in general).
> 
I was trying to keep the changes to the actual code to a minimum, so I 
didn't modify the calling sequence of the original code.
But sure, passing in the 'salt' as a parameter is sensible.

>> +	err = crypto_shash_setkey(hmac_tfm, default_salt, prklen);
>> +	if (!err)
>> +		err = crypto_shash_tfm_digest(hmac_tfm, ikm, ikmlen, prk);
>> +
>> +	kfree(default_salt);
>> +	return err;
>> +}
>> +EXPORT_SYMBOL_GPL(hkdf_extract);
>> +
>> +/*
>> + * HKDF-Expand (RFC 5869 section 2.3).
>> + * This expands the pseudorandom key, which was already keyed into @hmac_tfm,
>> + * into @okmlen bytes of output keying material parameterized by the
>> + * application-specific @info of length @infolen bytes.
>> + * This is thread-safe and may be called by multiple threads in parallel.
>> + */
>> +int hkdf_expand(struct crypto_shash *hmac_tfm,
>> +		const u8 *info, unsigned int infolen,
>> +		u8 *okm, unsigned int okmlen)
> 
> Needs kerneldoc now that this is a library interface.
> 
Ok.

>> diff --git a/fs/crypto/hkdf.c b/fs/crypto/hkdf.c
>> index 5a384dad2c72..9c2f9aca9412 100644
>> --- a/fs/crypto/hkdf.c
>> +++ b/fs/crypto/hkdf.c
>> // SPDX-License-Identifier: GPL-2.0
>> /*
>> * Implementation of HKDF ("HMAC-based Extract-and-Expand Key Derivation
>> * Function"), aka RFC 5869.  See also the original paper (Krawczyk 2010):
>> * "Cryptographic Extraction and Key Derivation: The HKDF Scheme".
>> *
>> * This is used to derive keys from the fscrypt master keys.
>> *
>> * Copyright 2019 Google LLC
>> */
> 
> The above file comment should be adjusted now that this file doesn't contain the
> actual HKDF implementation.
> 
Ok.

>> @@ -118,61 +105,24 @@ int fscrypt_hkdf_expand(const struct fscrypt_hkdf *hkdf, u8 context,
>>   			u8 *okm, unsigned int okmlen)
>>   {
>>   	SHASH_DESC_ON_STACK(desc, hkdf->hmac_tfm);
>> -	u8 prefix[9];
>> -	unsigned int i;
>> +	u8 *prefix;
>>   	int err;
>> -	const u8 *prev = NULL;
>> -	u8 counter = 1;
>> -	u8 tmp[HKDF_HASHLEN];
>>   
>>   	if (WARN_ON_ONCE(okmlen > 255 * HKDF_HASHLEN))
>>   		return -EINVAL;
>>   
>> +	prefix = kzalloc(okmlen + 9, GFP_KERNEL);
>> +	if (!prefix)
>> +		return -ENOMEM;
>>   	desc->tfm = hkdf->hmac_tfm;
>>   
>>   	memcpy(prefix, "fscrypt\0", 8);
>>   	prefix[8] = context;
>> +	memcpy(prefix + 9, info, infolen);
> 
> This makes the variable called 'prefix' no longer be the prefix, but rather the
> full info string.  A better name for it would be 'full_info'.
> 
> Also, it's being allocated with the wrong length.  It should be 9 + infolen.
> 
Will fix it up.

>> +	err = hkdf_expand(hkdf->hmac_tfm, prefix, infolen + 8,
>> +			  okm, okmlen);
>> +	kfree(prefix);
> 
> kfree_sensitive()
> 
>> diff --git a/include/crypto/hkdf.h b/include/crypto/hkdf.h
>> new file mode 100644
>> index 000000000000..bf06c080d7ed
>> --- /dev/null
>> +++ b/include/crypto/hkdf.h
>> @@ -0,0 +1,18 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * HKDF: HMAC-based Key Derivation Function (HKDF), RFC 5869
>> + *
>> + * Extracted from fs/crypto/hkdf.c, which has
>> + * Copyright 2019 Google LLC
>> + */
> 
> If this is keeping the copyright of fs/crypto/hkdf.c, the license needs to stay
> the same (GPL-2.0, not GPL-2.0-or-later).
> 
Will be modifying it.

Thanks a lot for your review!

Cheers,

Hannes
diff mbox series

Patch

diff --git a/crypto/Makefile b/crypto/Makefile
index edbbaa3ffef5..b77fc360f0ff 100644
--- a/crypto/Makefile
+++ b/crypto/Makefile
@@ -29,6 +29,7 @@  obj-$(CONFIG_CRYPTO_ECHAINIV) += echainiv.o
 
 crypto_hash-y += ahash.o
 crypto_hash-y += shash.o
+crypto_hash-y += hkdf.o
 obj-$(CONFIG_CRYPTO_HASH2) += crypto_hash.o
 
 obj-$(CONFIG_CRYPTO_AKCIPHER2) += akcipher.o
diff --git a/crypto/hkdf.c b/crypto/hkdf.c
new file mode 100644
index 000000000000..22e343851c0b
--- /dev/null
+++ b/crypto/hkdf.c
@@ -0,0 +1,112 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Implementation of HKDF ("HMAC-based Extract-and-Expand Key Derivation
+ * Function"), aka RFC 5869.  See also the original paper (Krawczyk 2010):
+ * "Cryptographic Extraction and Key Derivation: The HKDF Scheme".
+ *
+ * This is used to derive keys from the fscrypt master keys.
+ *
+ * Copyright 2019 Google LLC
+ */
+
+#include <crypto/hash.h>
+#include <crypto/sha2.h>
+#include <crypto/hkdf.h>
+
+/*
+ * HKDF consists of two steps:
+ *
+ * 1. HKDF-Extract: extract a pseudorandom key of length HKDF_HASHLEN bytes from
+ *    the input keying material and optional salt.
+ * 2. HKDF-Expand: expand the pseudorandom key into output keying material of
+ *    any length, parameterized by an application-specific info string.
+ *
+ */
+
+/* HKDF-Extract (RFC 5869 section 2.2), unsalted */
+int hkdf_extract(struct crypto_shash *hmac_tfm, const u8 *ikm,
+		 unsigned int ikmlen, u8 *prk)
+{
+	unsigned int prklen = crypto_shash_digestsize(hmac_tfm);
+	u8 *default_salt;
+	int err;
+
+	default_salt = kzalloc(prklen, GFP_KERNEL);
+	if (!default_salt)
+		return -ENOMEM;
+	err = crypto_shash_setkey(hmac_tfm, default_salt, prklen);
+	if (!err)
+		err = crypto_shash_tfm_digest(hmac_tfm, ikm, ikmlen, prk);
+
+	kfree(default_salt);
+	return err;
+}
+EXPORT_SYMBOL_GPL(hkdf_extract);
+
+/*
+ * HKDF-Expand (RFC 5869 section 2.3).
+ * This expands the pseudorandom key, which was already keyed into @hmac_tfm,
+ * into @okmlen bytes of output keying material parameterized by the
+ * application-specific @info of length @infolen bytes.
+ * This is thread-safe and may be called by multiple threads in parallel.
+ */
+int hkdf_expand(struct crypto_shash *hmac_tfm,
+		const u8 *info, unsigned int infolen,
+		u8 *okm, unsigned int okmlen)
+{
+	SHASH_DESC_ON_STACK(desc, hmac_tfm);
+	unsigned int i, hashlen = crypto_shash_digestsize(hmac_tfm);
+	int err;
+	const u8 *prev = NULL;
+	u8 counter = 1;
+	u8 *tmp;
+
+	if (WARN_ON(okmlen > 255 * hashlen))
+		return -EINVAL;
+
+	tmp = kzalloc(hashlen, GFP_KERNEL);
+	if (!tmp)
+		return -ENOMEM;
+
+	desc->tfm = hmac_tfm;
+
+	for (i = 0; i < okmlen; i += hashlen) {
+
+		err = crypto_shash_init(desc);
+		if (err)
+			goto out;
+
+		if (prev) {
+			err = crypto_shash_update(desc, prev, hashlen);
+			if (err)
+				goto out;
+		}
+
+		err = crypto_shash_update(desc, info, infolen);
+		if (err)
+			goto out;
+
+		BUILD_BUG_ON(sizeof(counter) != 1);
+		if (okmlen - i < hashlen) {
+			err = crypto_shash_finup(desc, &counter, 1, tmp);
+			if (err)
+				goto out;
+			memcpy(&okm[i], tmp, okmlen - i);
+			memzero_explicit(tmp, sizeof(tmp));
+		} else {
+			err = crypto_shash_finup(desc, &counter, 1, &okm[i]);
+			if (err)
+				goto out;
+		}
+		counter++;
+		prev = &okm[i];
+	}
+	err = 0;
+out:
+	if (unlikely(err))
+		memzero_explicit(okm, okmlen); /* so caller doesn't need to */
+	shash_desc_zero(desc);
+	kfree(tmp);
+	return err;
+}
+EXPORT_SYMBOL_GPL(hkdf_expand);
diff --git a/fs/crypto/hkdf.c b/fs/crypto/hkdf.c
index 5a384dad2c72..9c2f9aca9412 100644
--- a/fs/crypto/hkdf.c
+++ b/fs/crypto/hkdf.c
@@ -11,6 +11,7 @@ 
 
 #include <crypto/hash.h>
 #include <crypto/sha2.h>
+#include <crypto/hkdf.h>
 
 #include "fscrypt_private.h"
 
@@ -44,20 +45,6 @@ 
  * there's no way to persist a random salt per master key from kernel mode.
  */
 
-/* HKDF-Extract (RFC 5869 section 2.2), unsalted */
-static int hkdf_extract(struct crypto_shash *hmac_tfm, const u8 *ikm,
-			unsigned int ikmlen, u8 prk[HKDF_HASHLEN])
-{
-	static const u8 default_salt[HKDF_HASHLEN];
-	int err;
-
-	err = crypto_shash_setkey(hmac_tfm, default_salt, HKDF_HASHLEN);
-	if (err)
-		return err;
-
-	return crypto_shash_tfm_digest(hmac_tfm, ikm, ikmlen, prk);
-}
-
 /*
  * Compute HKDF-Extract using the given master key as the input keying material,
  * and prepare an HMAC transform object keyed by the resulting pseudorandom key.
@@ -118,61 +105,24 @@  int fscrypt_hkdf_expand(const struct fscrypt_hkdf *hkdf, u8 context,
 			u8 *okm, unsigned int okmlen)
 {
 	SHASH_DESC_ON_STACK(desc, hkdf->hmac_tfm);
-	u8 prefix[9];
-	unsigned int i;
+	u8 *prefix;
 	int err;
-	const u8 *prev = NULL;
-	u8 counter = 1;
-	u8 tmp[HKDF_HASHLEN];
 
 	if (WARN_ON_ONCE(okmlen > 255 * HKDF_HASHLEN))
 		return -EINVAL;
 
+	prefix = kzalloc(okmlen + 9, GFP_KERNEL);
+	if (!prefix)
+		return -ENOMEM;
 	desc->tfm = hkdf->hmac_tfm;
 
 	memcpy(prefix, "fscrypt\0", 8);
 	prefix[8] = context;
+	memcpy(prefix + 9, info, infolen);
 
-	for (i = 0; i < okmlen; i += HKDF_HASHLEN) {
-
-		err = crypto_shash_init(desc);
-		if (err)
-			goto out;
-
-		if (prev) {
-			err = crypto_shash_update(desc, prev, HKDF_HASHLEN);
-			if (err)
-				goto out;
-		}
-
-		err = crypto_shash_update(desc, prefix, sizeof(prefix));
-		if (err)
-			goto out;
-
-		err = crypto_shash_update(desc, info, infolen);
-		if (err)
-			goto out;
-
-		BUILD_BUG_ON(sizeof(counter) != 1);
-		if (okmlen - i < HKDF_HASHLEN) {
-			err = crypto_shash_finup(desc, &counter, 1, tmp);
-			if (err)
-				goto out;
-			memcpy(&okm[i], tmp, okmlen - i);
-			memzero_explicit(tmp, sizeof(tmp));
-		} else {
-			err = crypto_shash_finup(desc, &counter, 1, &okm[i]);
-			if (err)
-				goto out;
-		}
-		counter++;
-		prev = &okm[i];
-	}
-	err = 0;
-out:
-	if (unlikely(err))
-		memzero_explicit(okm, okmlen); /* so caller doesn't need to */
-	shash_desc_zero(desc);
+	err = hkdf_expand(hkdf->hmac_tfm, prefix, infolen + 8,
+			  okm, okmlen);
+	kfree(prefix);
 	return err;
 }
 
diff --git a/include/crypto/hkdf.h b/include/crypto/hkdf.h
new file mode 100644
index 000000000000..bf06c080d7ed
--- /dev/null
+++ b/include/crypto/hkdf.h
@@ -0,0 +1,18 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * HKDF: HMAC-based Key Derivation Function (HKDF), RFC 5869
+ *
+ * Extracted from fs/crypto/hkdf.c, which has
+ * Copyright 2019 Google LLC
+ */
+
+#ifndef _CRYPTO_HKDF_H
+#define _CRYPTO_HKDF_H
+
+int hkdf_extract(struct crypto_shash *hmac_tfm, const u8 *ikm,
+		 unsigned int ikmlen, u8 *prk);
+int hkdf_expand(struct crypto_shash *hmac_tfm,
+		const u8 *info, unsigned int infolen,
+		u8 *okm, unsigned int okmlen);
+
+#endif