force local wc_nnn hash struct variables to zeros by gojimmypi · Pull Request #6068 · wolfSSL/wolfssl

gojimmypi · 2023-02-08T02:15:17Z

Description

Related to the #5948 ED25519 failure, this update forces the local SHA struct values to all zeros before using them. This is typically only important to hash hardware acceleration where existing HW state may be stored in the ctx and otherwise have potentially bad data if not properly initialized or worse: copied from another SHA ctx that may have active hardware acceleration computation state values.

Recall only one given HW acceleration computation can be in process at a given time on the ESP32. Although the ESP32-C3 does seem to have the ability for concurrent HW encryption, that's beyond the scope of this PR and in any case we'd still need a properly initialized variable anyhow.

Note this PR is not a substitute for proper SHA ctx self validation to check for creation from a copy as noted in #5948 (comment), and is not meant to be a final fix to #5948 . A separate PR will be created soon.

There's a small performance hit when assigning zeros at every use. I'm open to suggestions if this should only be applied for relevant platforms, such as the Espressif ESP32 with Hardware Acceleration. I chose to do it all the time as a Best Practice to have variables always safely initialized.

Fixes zd# n/a

Testing

How did you test? Ran the wolfcrypt test for both embedded ESP32 and Linux.

Checklist

added tests
updated/added doxygen
updated appropriate READMEs
Updated manual and documentation

dgarske

Jim please consider properly fixing the initialization it in each of the hash init routines. Some of these already do a memset and this is now duplicated. Also the ForceZero should be memset.

dgarske · 2023-02-08T16:52:19Z

wolfcrypt/src/hash.c

+        /* Typically only important to hardware acceleration, but a good
+        ** practice: we'll make sure the md5 we start with does not have
+        ** any unexpected values in any of the properties. */
+        ForceZero(md5, sizeof(wc_Md5));


Just use XMEMSET here please. The ForceZero is possible not as fast and is only required in places where there might be secrets that need cleared after use. This is just ensuring the data is zero'd, there shouldn't be any secrets left behind. Also this now duplicates work already done in some of the init functions. If using memset the compiler could optimize away the duplicate (not if ForceZero is used).

This will have performance implications. My preference is to fix it in wc_InitMd5.

I've reverted the changes to hash.c and moved the memory zeroize, now with XMEMSET instead of ForceZero into the respective inits.

duplicates work already done in some of the init functions

@dgarske regarding this topic,: Although the ESP32 SHA tests pass, the RSA fails when the apparently duplicate XMEMSET here which should be ok to omit, as it is also here in the call to InitSha256. Clearly there's more research to be done.

dgarske · 2023-02-08T16:52:26Z

wolfcrypt/src/hash.c

+        /* Typically only important to hardware acceleration, but a good
+        ** practice: we'll make sure the sha we start with does not have
+        ** any unexpected values in any of the properties */
+        ForceZero(sha, sizeof(wc_Sha));


moved to md5.c

dgarske · 2023-02-08T16:52:31Z

wolfcrypt/src/hash.c

+        /* Typically only important to hardware acceleration, but a good
+        ** practice: we'll make sure the sha224 we start with does not have
+        ** any unexpected values in any of the properties */
+        ForceZero(sha224, sizeof(wc_Sha224));


moved to sha256.c

dgarske · 2023-02-08T16:52:37Z

wolfcrypt/src/hash.c

+        /* Typically only important to hardware acceleration, but a good
+        ** practice: we'll make sure the sha256 we start with does not have
+        ** any unexpected values in any of the properties */
+        ForceZero(sha256, sizeof(wc_Sha256));


moved to sha256.c

dgarske · 2023-02-08T16:54:58Z

wolfcrypt/src/hash.c

+        /* Typically only important to hardware acceleration, but a good
+        ** practice: we'll make sure the sha512 we start with does not have
+        ** any unexpected values in any of the properties. */
+        ForceZero(sha512, sizeof(wc_Sha512));


moved to sha512.c

dgarske · 2023-02-08T16:55:17Z

wolfcrypt/src/hash.c

+        /* Typically only important to hardware acceleration, but a good
+        ** practice: we'll make sure the sha3 we start with does not have
+        ** any unexpected values in any of the properties. */
+        ForceZero(sha3, sizeof(wc_Sha3));


moved to sha3c

dgarske · 2023-02-08T16:55:21Z

wolfcrypt/src/hash.c

+        /* Typically only important to hardware acceleration, but a good
+        ** practice: we'll make sure the sha3 we start with does not have
+        ** any unexpected values in any of the properties. */
+        ForceZero(sha3, sizeof(wc_Sha3));


moved to sha3c

dgarske · 2023-02-08T16:55:25Z

wolfcrypt/src/hash.c

+        /* Typically only important to hardware acceleration, but a good
+        ** practice: we'll make sure the sha3 we start with does not have
+        ** any unexpected values in any of the properties. */
+        ForceZero(sha3, sizeof(wc_Sha3));


moved to sha3c

dgarske · 2023-02-08T16:55:29Z

wolfcrypt/src/hash.c

+        /* Typically only important to hardware acceleration, but a good
+        ** practice: we'll make sure the shake we start with does not have
+        ** any unexpected values in any of the properties. */
+        ForceZero(shake, sizeof(wc_Shake));


moved to sha3c

dgarske · 2023-02-08T16:55:33Z

wolfcrypt/src/hash.c

+        /* Typically only important to hardware acceleration, but a good
+        ** practice: we'll make sure the shake we start with does not have
+        ** any unexpected values in any of the properties. */
+        ForceZero(shake, sizeof(wc_Shake));


moved to sha3c

gojimmypi · 2023-02-09T03:47:45Z

Reverted hash.c and moved XMEMSET into various files for initializtions. I'll squash merge after review if desired.

Rechecked with wolftest:

make[2]: Leaving directory '/mnt/c/temp/wolfssl_PR6068_test'
make[1]: Leaving directory '/mnt/c/temp/wolfssl_PR6068_test'
------------------------------------------------------------------------------
 wolfSSL version 5.5.4
------------------------------------------------------------------------------
error    test passed!
MEMORY   test passed!
base64   test passed!
asn      test passed!
RANDOM   test passed!
MD5      test passed!
SHA      test passed!
SHA-224  test passed!
SHA-256  test passed!
SHA-384  test passed!
SHA-512  test passed!
SHA-3    test passed!
Hash     test passed!
HMAC-MD5 test passed!
HMAC-SHA test passed!
HMAC-SHA224 test passed!
HMAC-SHA256 test passed!
HMAC-SHA384 test passed!
HMAC-SHA512 test passed!
HMAC-SHA3   test passed!
HMAC-KDF    test passed!
TLSv1.3 KDF test passed!
GMAC     test passed!
Chacha   test passed!
POLY1305 test passed!
ChaCha20-Poly1305 AEAD test passed!
AES      test passed!
AES192   test passed!
AES256   test passed!
AES-GCM  test passed!
RSA      test passed!
DH       test passed!
PWDBASED test passed!
ECC      test passed!
logging  test passed!
time test passed!
mutex    test passed!
memcb    test passed!
Test complete
stack used = 2097152
total   Allocs   =      1079
total   Deallocs =      1079
total   Bytes    =   2948064
peak    Bytes    =     59930
current Bytes    =         0
Exiting main with return code: 0

dgarske · 2023-02-09T16:40:52Z

wolfcrypt/src/sha512.c

 #if !defined(WOLFSSL_NOSHA512_224)
 int wc_InitSha512_224(wc_Sha512* sha)
 {
+    if (sha == NULL)


This is redundant. Already done in wc_InitSha512_224_ex -> InitSha512_Family.

agreed. good catch. updated.

dgarske

Thanks Jim. FYI: I will squash this on merge.

wolfSSL-Bot · 2023-02-09T20:58:11Z

Can one of the admins verify this patch?

SparkiDev · 2023-02-09T23:04:51Z

wolfcrypt/src/sha256.c

            return BAD_FUNC_ARG;
+        }
+
+        XMEMSET(sha224, 0, sizeof(wc_Sha224));


Missed wc_InitSha256_ex()

@SparkiDev thanks for your review! Would you recommend that I do a XMEMSET in each of these?

I'm not entirely convinced it is a good idea for software-only hashes. The only reason I found this and started to do the initializations was due to undesired hardware encryption state data being copied into a new sha ctx instance.

Please advise. Thank you.

Edit: Here's a code explorer view for conversation sake:

If the PR is to memset the data structure when using hardware and they are the implementations for hardware, then yes.
If you can't find a common for each function, then it needs to be done in each.

SparkiDev · 2023-02-09T23:06:33Z

wolfcrypt/src/sha512.c

        return BAD_FUNC_ARG;
+    }
+
+    XMEMSET(sha512, 0, sizeof(wc_Sha512));


Missed wc_InitSha383_ex().

gojimmypi · 2023-02-13T17:59:39Z

I have a higher performance, less brute-force method of initialization in the works. Closing this PR; New PR will be created from my WIP ED25519_SHA2_fix Branch.

gojimmypi · 2023-05-03T15:27:06Z

For reference, the root cause of the ED25519 failures are noted in #5948 (comment) and fixed in #6287.

The root cause was the use of in-progress ctx copies during active SHA HW process.

force local wc_nnn hash struct variables to zeros

ccb5abb

gojimmypi added the enhancement label Feb 8, 2023

gojimmypi requested review from SparkiDev and dgarske February 8, 2023 02:15

gojimmypi self-assigned this Feb 8, 2023

dgarske self-assigned this Feb 8, 2023

dgarske requested changes Feb 8, 2023

View reviewed changes

dgarske removed their assignment Feb 8, 2023

gojimmypi marked this pull request as draft February 8, 2023 22:59

gojimmypi added 2 commits February 8, 2023 19:30

revert hash.c changes

3ad7e29

set hash sha and md5 struct to zero during init

6a17a14

gojimmypi marked this pull request as ready for review February 9, 2023 03:47

gojimmypi requested a review from dgarske February 9, 2023 03:47

dgarske self-assigned this Feb 9, 2023

dgarske requested changes Feb 9, 2023

View reviewed changes

dgarske removed their assignment Feb 9, 2023

remove redundant memset

3656703

gojimmypi requested a review from dgarske February 9, 2023 17:11

dgarske approved these changes Feb 9, 2023

View reviewed changes

dgarske self-assigned this Feb 9, 2023

dgarske assigned SparkiDev and unassigned gojimmypi Feb 9, 2023

dgarske mentioned this pull request Feb 9, 2023

Fix ESP32 HW ED25519 test #5997

Closed

4 tasks

SparkiDev requested changes Feb 9, 2023

View reviewed changes

dgarske assigned gojimmypi and unassigned dgarske and SparkiDev Feb 9, 2023

gojimmypi marked this pull request as draft February 10, 2023 23:38

gojimmypi closed this Feb 13, 2023

Conversation

gojimmypi commented Feb 8, 2023

Description

Testing

Checklist

Uh oh!

dgarske 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!

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!

gojimmypi commented Feb 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgarske left a comment

Choose a reason for hiding this comment

Uh oh!

wolfSSL-Bot commented Feb 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gojimmypi Feb 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

gojimmypi commented Feb 13, 2023

Uh oh!

gojimmypi commented May 3, 2023

Uh oh!

Reviewers

Assignees

gojimmypi Feb 11, 2023 •

edited

Loading