sys/hashes: add SHA-512 support

[mguetschow]

Contribution description

• software implementation of SHA-512 in sys/hashes
• corresponding tests in tests/unittests/test-hashes

TODO

• discuss code duplication between sha2xx_common and sha512_common
• implement support for SHA-384, SHA-512/224 and SHA-512/256 (straightforward)
• add wrapper code to support SHA-512 via PSA crypto API

Testing procedure

make -C tests/unittests tests-hashes term, already locally tested for native and nRF52840dk

Comments

• The code is based on the existing SHA-256 implementation. The functions are virtually identical, but use different constants (notably the ones defined by NIST.FIPS.180-4), different block sizes and different datatypes (uint64_t instead of uint32_t). I considered factoring out common logic, but especially the last difference made me refrain from it. However, this leads to some code duplication, mainly between sha2xx_common.{c,h} and sha512_common.{c,h}. Any takes on this?

• I'm still planning to add support for truncated hashes based on SHA-512 and integration with the PSA crypto API. Would you prefer those in separate PRs or as additional commits here?

• I had to rename the elementary function defines in sha2xx_common.h to avoid name clashes in case both SHA-256 and SHA-512 headers would be included in the same C file. This made me wonder if those constants should really be part of the header file, or rather belong to the c file? Aka, do we want them to be accessible for RIOT users?

[riot-ci]

Murdock results

✔️ PASSED

6935ea2 sys/hashes: add SHA-512 support

Success	Failures	Total	Runtime
7956	0	7957	09m:59s

Artifacts

• Documentation preview

[mguetschow]

/opt/esp/xtensa-esp32s3-elf/bin/../lib/gcc/xtensa-esp32s3-elf/12.2.0/../../../../xtensa-esp32s3-elf/bin/ld: /tmp/dwq.0.7626877122765472/150848647c18bce86790131246439682/build/hashes/sha512.o: in function `sha512_init':
sha512.c:(.text.sha512_init+0x40): multiple definition of `sha512_init'; /tmp/dwq.0.7626877122765472/150848647c18bce86790131246439682/build/esp_idf_wpa_supplicant_crypto/components/wpa_supplicant/src/crypto/sha512-internal.o:sha512-internal.c:(.text.sha512_init+0x40): first defined here

Interesting. Which one should I rename?

[miri64]

/opt/esp/xtensa-esp32s3-elf/bin/../lib/gcc/xtensa-esp32s3-elf/12.2.0/../../../../xtensa-esp32s3-elf/bin/ld: /tmp/dwq.0.7626877122765472/150848647c18bce86790131246439682/build/hashes/sha512.o: in function `sha512_init':
sha512.c:(.text.sha512_init+0x40): multiple definition of `sha512_init'; /tmp/dwq.0.7626877122765472/150848647c18bce86790131246439682/build/esp_idf_wpa_supplicant_crypto/components/wpa_supplicant/src/crypto/sha512-internal.o:sha512-internal.c:(.text.sha512_init+0x40): first defined here

Interesting. Which one should I rename?

Given that we do not have this clash with other hash functions, I suspect @gschorcht did something so that for them it does not happen. Maybe have a look at what was done for the other hash functions?

[gschorcht]

Given that we do not have this clash with other hash functions, I suspect @gschorcht did something so that for them it does not happen. Maybe have a look at what was done for the other hash functions?

Yeah, we had to patch the esp32_sdk package to rename such functions, see pkg/esp32_sdk/patches/0002-wpa_supplicant-add-prefix-wpa_-to-crypto-functions.patch. We use prefix wpa_ for all these functions, for example wpa_sha256_init instead of sha256_init.

[gschorcht]

If I'm right, we are talking only about

sha512_init

[gschorcht]

Could you try attached patch as file pkg/esp32_sdk/patches/0033-wpa_supplicant-add-prefix-wpa_-to-sha512_init.patch?

From 241861e24ad628946e3257317549a70c6f90aeec Mon Sep 17 00:00:00 2001
From: Gunar Schorcht <gunar@schorcht.net>
Date: Tue, 10 Oct 2023 17:54:52 +0200
Subject: [PATCH 33/33] wpa_supplicant: add prefix wpa_ to sha512_init

Prefix `_wpa` added to `sha512_init` function of `wpa_suppplicant` to avoid name conflicts with RIOT modules `crypto` and `hashes`.
---
 components/wpa_supplicant/src/crypto/crypto_internal.c | 2 +-
 components/wpa_supplicant/src/crypto/sha512-internal.c | 4 ++--
 components/wpa_supplicant/src/crypto/sha512_i.h        | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/components/wpa_supplicant/src/crypto/crypto_internal.c b/components/wpa_supplicant/src/crypto/crypto_internal.c
index d1426a8feb7..7ff588cbb40 100644
--- a/components/wpa_supplicant/src/crypto/crypto_internal.c
+++ b/components/wpa_supplicant/src/crypto/crypto_internal.c
@@ -67,7 +67,7 @@ struct crypto_hash * crypto_hash_init(enum crypto_hash_alg alg, const u8 *key,
 #endif /* CONFIG_INTERNAL_SHA384 */
 #ifdef CONFIG_INTERNAL_SHA512
 	case CRYPTO_HASH_ALG_SHA512:
-		sha512_init(&ctx->u.sha512);
+		wpa_sha512_init(&ctx->u.sha512);
 		break;
 #endif /* CONFIG_INTERNAL_SHA512 */
 	case CRYPTO_HASH_ALG_HMAC_MD5:
diff --git a/components/wpa_supplicant/src/crypto/sha512-internal.c b/components/wpa_supplicant/src/crypto/sha512-internal.c
index c0263941c12..1e816867faf 100644
--- a/components/wpa_supplicant/src/crypto/sha512-internal.c
+++ b/components/wpa_supplicant/src/crypto/sha512-internal.c
@@ -27,7 +27,7 @@ int sha512_vector(size_t num_elem, const u8 *addr[], const size_t *len,
 	struct sha512_state ctx;
 	size_t i;
 
-	sha512_init(&ctx);
+	wpa_sha512_init(&ctx);
 	for (i = 0; i < num_elem; i++)
 		if (sha512_process(&ctx, addr[i], len[i]))
 			return -1;
@@ -161,7 +161,7 @@ static int sha512_compress(struct sha512_state *md, unsigned char *buf)
    @param md   The hash state you wish to initialize
    @return CRYPT_OK if successful
 */
-void sha512_init(struct sha512_state *md)
+void wpa_sha512_init(struct sha512_state *md)
 {
 	md->curlen = 0;
 	md->length = 0;
diff --git a/components/wpa_supplicant/src/crypto/sha512_i.h b/components/wpa_supplicant/src/crypto/sha512_i.h
index 108958911ef..e451e48fcfd 100644
--- a/components/wpa_supplicant/src/crypto/sha512_i.h
+++ b/components/wpa_supplicant/src/crypto/sha512_i.h
@@ -17,7 +17,7 @@ struct sha512_state {
 	u8 buf[SHA512_BLOCK_SIZE];
 };
 
-void sha512_init(struct sha512_state *md);
+void wpa_sha512_init(struct sha512_state *md);
 int sha512_process(struct sha512_state *md, const unsigned char *in,
 		   unsigned long inlen);
 int sha512_done(struct sha512_state *md, unsigned char *out);
-- 
2.34.1

0033-wpa_supplicant-add-prefix-wpa_-to-sha512_init.txt

[mguetschow]

Thanks @gschorcht, that worked!

The remaining failing static test is dependent on the decision made on the last point in the initial post.

[mguetschow]

I've now proceeded (in two separate commits) to

1. add wrapper code for psa_crypto (@Einhornhool)
2. move the SHA macros and constants to the respective common .c files (referring to the last bullet point above)

I would leave the implementation of the truncated versions to a future PR and claim the PR to be ready for review. (pinging some former contributors to sys/hash: @mtausig @OlegHahm @PeterKietzmann).

[Einhornhool]

The PSA Crypto part looks good to me. Did you test this with some application?

[mguetschow]

The PSA Crypto part looks good to me. Did you test this with some application?

I've been using it for a while now without problems in a custom application. Do you want me to add an example to examples/psa_crypto?

[Teufelchen1]

Nice work!
I can't comment on the math but the code looks clean. I found some minor minor styling issues.
One thing that stood out is the static buffer in sha512(), which can lead to racy conditions (Don't believe it is an issue, just wanted to highlight.).

Nit:
The code always assumes that *ctx is not NULL without checking: Do we want to highlight that in the api comments or add sanity asserts?

I did like the unit-tests but you missed some behaviors:

• Can sha512_update be called multiple times before calling sha512_final? If so please provide a test.
• All tests use strings. If sha512 can handle 'binary' data, please provide a test.
• sha512_final states "... and clears the context state." but there is no test that ensures this behavior.
• The static-buffer behavior of sha512 is not checked.

(ofc, these tests are not for finding bugs in this pr - but finding regressions in the future™)

🚂

[mguetschow]

Thanks @Teufelchen1 for your thorough review! I've added all the test cases that you mentioned in 3b237f6.

The code always assumes that *ctx is not NULL without checking: Do we want to highlight that in the api comments or add sanity asserts?

iirc, the assertions are only checked in debug builds, i.e. without runtime overhead? In that case I would do both?

[Teufelchen1]

I'm almost happy!

Regarding assert:

The @ref assert() function is only evaluated with `DEVELHELP=1` (or when
 *   `FORCE_ASSERTS=1`). Otherwise, the build system sets `NDEBUG` and thus
 *   skips assertions entirely.

See:

RIOT/doc.txt

Line 52 in a36817c

* * The @ref assert() function is only evaluated with `DEVELHELP=1` (or when

[mguetschow]

I'm done from my side and would postpone the implementation of the truncated versions (SHA-384 et al) to a later PR. The CI tells me to rebase and squash, should I do that now?

My issue has been resolved

[mguetschow]

I've recreated test/unittests/Makefile.ci with ../../dist/tools/insufficient_memory/create_makefile.ci.sh, fingers crossed it works now.