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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/14/24 21:38, Eric Biggers wrote:
On Fri, Oct 11, 2024 at 05:54:22PM +0200, Hannes Reinecke wrote:
Separate out the HKDF functions into a separate module to
to make them available to other callers.
And add a testsuite to the module with test vectors
from RFC 5869 to ensure the integrity of the algorithm.

integrity => correctness

Okay.

+config CRYPTO_HKDF
+	tristate
+	select CRYPTO_SHA1 if !CONFIG_CRYPTO_MANAGER_DISABLE_TESTS
+	select CRYPTO_SHA256 if !CONFIG_CRYPTO_MANAGER_DISABLE_TESTS
+	select CRYPTO_HASH2

Any thoughts on including SHA512 tests instead of SHA1, given that SHA1 is
obsolete and should not be used?

Hmm. The original implementation did use SHA1, so I used that.
But sure I can look into changing that.

+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[HASH_MAX_DIGESTSIZE];
+
+	if (WARN_ON(okmlen > 255 * hashlen ||
+		    hashlen > HASH_MAX_DIGESTSIZE))
+		return -EINVAL;

The crypto API guarantees HASH_MAX_DIGESTSIZE, so checking that again is not
very useful.

Okay, will be removing that check.

+
+	memzero_explicit(tmp, HASH_MAX_DIGESTSIZE);

The zeroization above is unnecessary.  If it's done anyway, it is just an
initialization, so it should use an initializer '= {}' instead of
memzero_explicit() which is intended for "destruction".

Ok.

+MODULE_ALIAS_CRYPTO("hkdf");

This alias does not make sense and is unnecessary.  These are library functions
that are not exposed by name through the crypto API, so there is no need to wire
them up to the module autoloading accordingly.

Ok.

diff --git a/include/crypto/hkdf.h b/include/crypto/hkdf.h
new file mode 100644
index 000000000000..c1f23a32a6b6
--- /dev/null
+++ b/include/crypto/hkdf.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * 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, const u8 *salt, unsigned int saltlen,
+		 u8 *prk);
+int hkdf_expand(struct crypto_shash *hmac_tfm,
+		const u8 *info, unsigned int infolen,
+		u8 *okm, unsigned int okmlen);
+#endif

This needs to include <crypto/hash.h>.

Otherwise there will be errors if someone includes this as their first header.

Will do.

Thanks for the review!

Cheers,

Hannes
--
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@xxxxxxx                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich




[Index of Archives]     [Kernel]     [Gnu Classpath]     [Gnu Crypto]     [DM Crypt]     [Netfilter]     [Bugtraq]
  Powered by Linux