[sw/crypto] Clear HMAC ctx struct #26990

h-filali · 2025-04-16T09:53:57Z

This PR depends on #26961.

This PR adds functionality for wiping the HMAC ctx struct using randomness for the sensitive struct members.

Part of #26941.

nasahlpa

LGTM, thanks Hakim!

BTW, would you mind updating your mail in the git commit to your work mail?

nasahlpa · 2025-04-25T05:46:30Z

sw/device/lib/crypto/drivers/hmac.c

+ */
+static void wipe_buffer(uint32_t *buf, uint32_t buf_len) {
+  for (int i = 0; i < buf_len; ++i) {
+    buf[i] = ibex_rnd_uint32();


What happens if the random data inside Ibex never gets ready, e.g., when the EDN is turned off? I guess then we would loop forever in the ibex_rnd_uint32() function?

Might be a good idea to use cryptolib's entropy_complex_check() to ensure the EDN is on and in cryptolib's expected configuration 🙂 cryptolib is supposed to be stateless, so it shouldn't need an initialization step, but it does expect ROM_EXT to leave the entropy complex in a certain state for it and operations should generally always run the check function before using the entropy complex.

Thanks, I'll add the check in my next amendment.

nasahlpa · 2025-04-25T05:50:51Z

sw/device/lib/crypto/impl/hash.c

+    return OTCRYPTO_BAD_ARGS;
+  }
+
+  for (int i = 0; i < kOtcryptoHashCtxStructWords; ++i) {


I would use size_t whenever possible in for loops.

jadephilipoom · 2025-04-25T10:43:17Z

sw/device/lib/crypto/drivers/hmac.c

+ */
+static void wipe_buffer(uint32_t *buf, uint32_t buf_len) {
+  for (int i = 0; i < buf_len; ++i) {
+    buf[i] = ibex_rnd_uint32();


Might be a good idea to use cryptolib's entropy_complex_check() to ensure the EDN is on and in cryptolib's expected configuration 🙂 cryptolib is supposed to be stateless, so it shouldn't need an initialization step, but it does expect ROM_EXT to leave the entropy complex in a certain state for it and operations should generally always run the check function before using the entropy complex.

jadephilipoom · 2025-04-25T10:46:56Z

sw/device/lib/crypto/drivers/hmac.c

+ * @param buf The incoming buffer to be wiped with random data.
+ * @param buf_len The length of `buf` in words.
+ */
+static void wipe_buffer(uint32_t *buf, uint32_t buf_len) {


I think we should probably use hardened_memshred here, that's basically what it's meant for and it has some heavier-weight anti-FI/SCA mechanisms:

opentitan/sw/device/lib/base/hardened_memory.h

Line 60 in 29d8474

void hardened_memshred(uint32_t *dest, size_t word_len);

However, it currently uses a hardcoded constant value to clear, which should probably be replaced with ibex RND reads now that the Ibex driver exists:

opentitan/sw/device/lib/base/hardened_memory.c

Line 81 in 29d8474

// The source of randomness for shred, which may be replaced at link-time.

Sure I can use the hardened_memshred function. However, the entropy complex check can't be done in there since it doesn't have a return value. I suggest we forget the check in this PR and I do a follow up PR where I change ibex_rnd_uint to return the cycle count whenever the entropy complex is not set up properly. What do you think?

jadephilipoom · 2025-04-25T10:47:32Z

sw/device/lib/crypto/drivers/hmac.c

@@ -531,6 +581,5 @@ status_t hmac(const hmac_mode_t hmac_mode, const uint32_t *key,
  // Clean up HMAC HWIP so it can be reused by other driver calls.
  hmac_hwip_clear();

-  // TODO(#23191): Destroy sensitive values in the ctx object.


Looks like this was deleted but hmac_context_wipe wasn't called?

There is no ctx obj here. I could wipe the key and the message though. So either we leave the wiping of the input key and data to the caller or wipe them in this function.

jadephilipoom · 2025-04-25T10:48:36Z

sw/device/lib/crypto/impl/hash.c

+ *
+ * @param[out] ctx Initialized context object.
+ */
+static otcrypto_status_t hash_context_wipe(otcrypto_hash_context_t *const ctx) {


I'd suggest hardened_memshred here too.

Sorry, this comment wasn't clear -- I mean I think the function should be replaced with hardened_memshred. The null check isn't needed, since it's an internal function and I expect the cryptolib will always have checked if that pointer is null before getting to the point of wiping the context. So this function isn't necessary; you can just call hardened_memshred inline. This is actually an important distinction for code size; the HARDENED_TRY macro introduces several instructions and kind of blocks the compiler from just inlining the function call.

h-filali · 2025-05-09T13:17:08Z

@nasahlpa @jadephilipoom I rebased to #27145 and removed the overlapping parts. I addressed all the requested changes. Do you think we can merge this whenever #27145 is merged?

nasahlpa

LGTM with one question.

nasahlpa · 2025-07-11T14:48:10Z

sw/device/lib/crypto/drivers/hmac.c

-  // TODO(#23191): Use a random value from EDN to wipe.
-  abs_mmio_write32(kHmacBaseAddr + HMAC_WIPE_SECRET_REG_OFFSET, UINT32_MAX);
+  // Use a random value from EDN to wipe HMAC.
+  HARDENED_TRY(entropy_complex_check());


It seems that, for some functions, we check multiple times whether the entropy complex is enabled or not.

Would it make sense to move these checks at the beginning of the functions, e.g., in hmac_final() and remove it in clear() and hmac_context_wipe()?

Thanks Pascal, that's a good point. I'll move the check out of clear() and hmac_context_wipe(). Especially since they are static functions and we have a good overview from where the functions are called.

At certain points the HMAC ctx struct has to be wiped to avoid leaking secrets. The sensitive fields need to be overwritten by randomness and the non-sensitive fields are just zeroized. Signed-off-by: Hakim Filali <[email protected]>

h-filali · 2025-07-16T14:06:14Z

All the comments on this PR should be addressed and if there are no further suggestions this is ready to be merged.

h-filali requested a review from a team as a code owner April 16, 2025 09:53

h-filali requested review from jon-flatley, jadephilipoom and nasahlpa and removed request for a team and jon-flatley April 16, 2025 09:53

h-filali force-pushed the crypto-clear-hmac-ctx branch 11 times, most recently from 358feb1 to bc1ea57 Compare April 17, 2025 16:22

This was referenced Apr 17, 2025

[sw/crypto] Randomize write address offset #27007

Open

[cryptolib] Open SCA hardening TODOs #26941

Open

nasahlpa approved these changes Apr 25, 2025

View reviewed changes

jadephilipoom reviewed Apr 25, 2025

View reviewed changes

h-filali force-pushed the crypto-clear-hmac-ctx branch 2 times, most recently from 8eb6a55 to 7961de7 Compare April 29, 2025 12:16

h-filali added the CI:Rerun Rerun failed CI jobs label Apr 29, 2025

github-actions bot removed the CI:Rerun Rerun failed CI jobs label Apr 29, 2025

h-filali force-pushed the crypto-clear-hmac-ctx branch 2 times, most recently from 8597600 to f381b57 Compare April 29, 2025 12:59

h-filali added the CI:Rerun Rerun failed CI jobs label Apr 29, 2025

github-actions bot removed the CI:Rerun Rerun failed CI jobs label Apr 29, 2025

h-filali added the CI:Rerun Rerun failed CI jobs label Apr 30, 2025

h-filali force-pushed the crypto-clear-hmac-ctx branch 6 times, most recently from ff5ea08 to 0fd10b3 Compare May 9, 2025 09:50

nasahlpa self-requested a review May 9, 2025 10:04

h-filali force-pushed the crypto-clear-hmac-ctx branch 2 times, most recently from dd247ef to 76977ff Compare May 9, 2025 13:05

h-filali force-pushed the crypto-clear-hmac-ctx branch 2 times, most recently from cfce754 to 0b41fef Compare May 13, 2025 14:16

h-filali force-pushed the crypto-clear-hmac-ctx branch 3 times, most recently from 0df0040 to c1a9857 Compare July 11, 2025 13:17

nasahlpa approved these changes Jul 11, 2025

View reviewed changes

h-filali force-pushed the crypto-clear-hmac-ctx branch 4 times, most recently from 3ab44f4 to 9c45dce Compare July 15, 2025 11:17

h-filali added the CI:Rerun Rerun failed CI jobs label Jul 15, 2025

github-actions bot removed the CI:Rerun Rerun failed CI jobs label Jul 15, 2025

h-filali force-pushed the crypto-clear-hmac-ctx branch 3 times, most recently from 54e9a4b to b358f8d Compare July 15, 2025 14:16

h-filali added the CI:Rerun Rerun failed CI jobs label Jul 15, 2025

github-actions bot removed the CI:Rerun Rerun failed CI jobs label Jul 15, 2025

[sw/crypto] Clear HMAC ctx struct

2e087e8

At certain points the HMAC ctx struct has to be wiped to avoid leaking secrets. The sensitive fields need to be overwritten by randomness and the non-sensitive fields are just zeroized. Signed-off-by: Hakim Filali <[email protected]>

h-filali force-pushed the crypto-clear-hmac-ctx branch from b358f8d to 2e087e8 Compare July 15, 2025 14:36

[sw/crypto] Clear HMAC ctx struct #26990

Are you sure you want to change the base?

[sw/crypto] Clear HMAC ctx struct #26990

Uh oh!

Conversation

h-filali commented Apr 16, 2025

Uh oh!

nasahlpa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

h-filali commented May 9, 2025

Uh oh!

nasahlpa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

h-filali commented Jul 16, 2025

Uh oh!

Uh oh!