Add new SHA-512/224 and SHA-512/256 tests #6097

gojimmypi · 2023-02-16T13:18:53Z

Description

Although it would seem if the SHA512 tests pass, that SHA-512/224 and SHA-512/256 would also succeed given the similar nature. Although true with wolfSSL software libraries, these new tests fail with hardware acceleration enabled on the Espressif ESP32.

This was discovered to be the root cause of the OpenSSL failure described in #6028.

These are also two of the missing tests noted in #5989.

The root cause of the Espressif failure is that although there's SHA512 hardware acceleration on the ESP32, there is not hardware acceleration for the SHA512/224 nor SHA512/256. See the Chip Series Comparison.

Note there is considerably more hardware acceleration available in the ESP32-S2/-S3 chips.

Fixes zd# n/a

Testing

Tested in WSL with:

./configure CC=clang --enable-trackmemory CFLAGS=-DHAVE_STACK_SIZE && make && ./wolfcrypt/test/testwolfcrypt

And tested in VisualGDB for Espressif ESP-IDF project in IDE/Espressif/ESP-IDF/examples/wolfssl_test with the following settings:

#define HAVE_AESGCM
#define WOLFSSL_RIPEMD
#define WOLFSSL_SHA224
#define WOLFSSL_SHA3
#define WOLFSSL_SHA384
#define WOLFSSL_SHA512
#define HAVE_ECC
#define HAVE_CURVE25519
#define CURVE25519_SMALL
#define HAVE_ED25519
#define OPENSSL_EXTRA
#define HAVE_PKCS7

Note that for the ESP32, the tests are all successful with this:

/* when you want not to use HW acceleration */
#define NO_ESP32WROOM32_CRYPT
#define NO_WOLFSSL_ESP32WROOM32_CRYPT_HASH
/* #define NO_WOLFSSL_ESP32WROOM32_CRYPT_AES */
/* #define NO_WOLFSSL_ESP32WROOM32_CRYPT_RSA_PRI */

But fail when all hardware encryption is turned on:

/* #define NO_ESP32WROOM32_CRYPT */
/* #define NO_WOLFSSL_ESP32WROOM32_CRYPT_HASH */
/* #define NO_WOLFSSL_ESP32WROOM32_CRYPT_AES */
/* #define NO_WOLFSSL_ESP32WROOM32_CRYPT_RSA_PRI */

Checklist

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

SparkiDev · 2023-02-16T22:15:48Z

wolfcrypt/test/test.c

@@ -19,6 +19,10 @@
 * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335, USA
 */

+/*
+** refer to https://csrc.nist.gov/projects/hash-functions


This comment is not helpful for the test program.
A reference to a document whose test vectors you used, if any, at the test vector, is useful.

The NIST hash functions link has been removed.

To be clear, the link is fine, but it needs to go next to the vectors (assuming you used them from there).

douzzer

This is a good addition. Ran some analyzers on it here and it's all good.

As @SparkiDev says, just move that comment so it's adjacent to the test vectors.

douzzer · 2023-02-17T06:57:54Z

Per Windows FIPS Test there's some missing FIPS gating on the new test routines -- @ejohnstown explains:

        # SHA512-224 and SHA512-256 are SHA-2 algorithms not in our FIPS algorithm list

gojimmypi · 2023-02-17T22:37:23Z

Per Windows FIPS Test there's some missing FIPS gating on the new test routines

@douzzer I added some HAVE_FIPS gating. Please review and confirm it is an appropriate use.

douzzer · 2023-02-18T03:18:56Z

wolfcrypt/src/sha512.c

@@ -1615,7 +1615,7 @@ int wc_Sha512GetFlags(wc_Sha512* sha512, word32* flags)

 #if !defined(HAVE_FIPS) && !defined(HAVE_SELFTEST)

-#if !defined(WOLFSSL_NOSHA512_224)
+#if !defined(WOLFSSL_NOSHA512_224) && !defined(HAVE_FIPS)


This should be the construction we settled on earlier,

#if !defined(WOLFSSL_NOSHA512_224) && (!defined(HAVE_FIPS) || FIPS_VERSION_GE(5, 2)) && !defined(HAVE_SELFTEST)

and similar throughout. As you saw, with FIPS 140-3 the algorithm macros are properly set up by the configure logic. And for fips-dev purposes, we do want to be able to test them in a FIPS build.

gojimmypi · 2023-02-18T20:39:38Z

TL;DR seems to still be bad gate logic for new tests?

Thanks everyone for helping to craft the new SHA512/224 FIPS gate:

#if !defined(WOLFSSL_NOSHA512_224) && (!defined(HAVE_FIPS) || FIPS_VERSION_GE(5, 2)) && !defined(HAVE_SELFTEST)

and SHA512/256 FIPS gate:

#if !defined(WOLFSSL_NOSHA512_256) && (!defined(HAVE_FIPS) || FIPS_VERSION_GE(5, 2)) && !defined(HAVE_SELFTEST)

I've also updated this PR with the NIST example PDF links closer to the test vectors. The new fixed digest values have been slightly reformatted to be a bit more human-readable. They are now separated into 4-byte words as is done in the NIST docs. If this format is acceptable, I can change the others when I next visit the respective files.

However, I've been unable to pass the Windows FIPS test with a bunch of errors related errors such as this one:

unresolved external symbol wc_InitSha512_256_ex
unresolved external symbol wc_InitSha512_224
unresolved external symbol wc_Sha512_256Transform

I tried a more brute-force method of including the new gates in many more places, no joy.

I decided to leave some breadcrumbs, to see exactly which wc_InitSha512_256_ex was failing (see console ). Turns out it is the one in the hash.c file. So perhaps the new gate logic is not quite right?

Clearly I don't understand the FIPS build as well as I should.

I am able to compile the IDE/WIN10/wolfssl-fips.vcxproj project locally, but not able to compile the companion test.vcxproj seeing these errors, even on the master branch:

gojimmypi · 2023-02-19T16:48:35Z

I've reverted the proposed gates on these news tests to just the simple !defined(HAVE_FIPS). The tests are all passing, although I did need to manually re-run several of them in Jenkins.

I propose that this PR be accepted as-is, given that there's no actual change to any FIPS testing as these tests never existed anyhow. We can then review the more elaborate conditional testing as a separate exercise. Perhaps @kaleb-himes can help me better understand the conditional FIPS builds.

I've left all the testing commits in place here, but can squash these if there's agreement to accept this PR.

dgarske · 2023-02-20T20:27:53Z

src/ssl.c

@@ -19,6 +19,7 @@
 * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335, USA
 */

+ /* revert to master test */


What is the purpose of all these comments revert to master test?

During testing, I was seeing linking failures in Jenkins; I copied the master files back and I was using that interim text as a breadcrumb. This text is not intended to be included in the final merge and I can remove upon squash.

dgarske · 2023-02-20T20:28:36Z

wolfcrypt/test/test.c

+    }
+
+#if !defined(WOLFSSL_NOSHA512_224) && \
+    !defined(HAVE_FIPS)


Why doesn't this match the above FIPS logic at line 419?

I was unable to get Jenkins to properly build with the longer FIPS logic FIPS_VERSION_GE(5, 2)), See PRB-FIPS-windows-test-part2-ACVP-v2/3032, so I reverted the FIPS logic to this shorter version until with the longer version.

I was unable to find the source code for that test. If you can point me in the right direction, I will likely be more successful with a local build.

oh, wait: line 419. Let me correct that.

I've reverted all of the logic back to the simple !defined(HAVE_FIPS) as I was not able to get the (!defined(HAVE_FIPS) || FIPS_VERSION_GE(5, 2)) && !defined(HAVE_SELFTEST) to compile in Jenkins.

dgarske · 2023-02-20T20:28:59Z

wolfcrypt/test/test.c

+#endif /* !defined(WOLFSSL_NOSHA512_224) ... */
+
+#if !defined(WOLFSSL_NOSHA512_256) && \
+    !defined(HAVE_FIPS)


Same, why does not not match FIPS logic at line 423?

dgarske · 2023-02-20T20:29:42Z

wolfcrypt/test/test.c

@@ -3003,8 +3035,311 @@ WOLFSSL_TEST_SUBROUTINE int sha512_test(void)

    return ret;
 }
+
+#if !defined(WOLFSSL_NOSHA512_224) && \
+     !defined(HAVE_FIPS)


Same. Why not (!defined(HAVE_FIPS) || FIPS_VERSION_GE(5, 2)) && !defined(HAVE_SELFTEST)?

It failed to compile in Jenkins with many unresolved external symbol linking errors with that logic. Shall I put that back so we can more readily see the errors in Jenkins? Alternatively, I can cleanup this PR commit history for a merge and we can do the conditional FIPS gating as a separate exercise. Either works for me.

dgarske · 2023-02-20T20:29:49Z

wolfcrypt/test/test.c

+
+
+#if !defined(WOLFSSL_NOSHA512_256) && \
+    !defined(HAVE_FIPS)


Same. Why not (!defined(HAVE_FIPS) || FIPS_VERSION_GE(5, 2)) && !defined(HAVE_SELFTEST)?

douzzer

Can we try the desired gate one more time, but using 5.3 as the horizon for SHA512-224/256 being accessible in fips-dev builds?

So that's e.g.

#if !defined(WOLFSSL_NOSHA512_224) && \
   (!defined(HAVE_FIPS) || FIPS_VERSION_GE(5, 3)) && !defined(HAVE_SELFTEST)

In practice this is all we want+need -- looks like we were being greedy expecting it to work right on the 5.2 submission.

If that gate doesn't work right, we have bigger issues, which we would address forthwith.

gojimmypi · 2023-02-21T03:14:18Z

TADA! The FIPS_VERSION_GE(5, 3) was definitely the key.

Thanks for all the help here, as I would not have figured out the FIPS thing on my own.

Note that I've also added the companion benchmarks in #6113

douzzer

OK we finally got it!

gave it a run through the local test harness (wolfssl-multi-test.sh super-quick-check) and the only thing it reported was that the NIST URLs are more than 80 characters long. LGTM!

all suggestions implemented

gojimmypi · 2023-02-21T15:51:10Z

Note to others and future self: For the wolfssl-fips project I needed to set Configuration to DLL Debug and Platform to x64, along with ./fips-check.sh linuxv5 keep.

gojimmypi added the enhancement label Feb 16, 2023

gojimmypi requested review from dgarske, douzzer, kaleb-himes and SparkiDev February 16, 2023 13:18

gojimmypi self-assigned this Feb 16, 2023

gojimmypi mentioned this pull request Feb 16, 2023

Compilation error when defining OPENSSL_ALL or OPENSSL_EXTRA on ESP-IDFv5 : ssl.c:18011:22: error: size of array 'sha_test' is negative #6028

Closed

dgarske self-assigned this Feb 16, 2023

gojimmypi closed this Feb 16, 2023

gojimmypi force-pushed the New_SHA512_Tests branch from 0bf5497 to 0a6deda Compare February 16, 2023 17:31

gojimmypi reopened this Feb 16, 2023

gojimmypi mentioned this pull request Feb 16, 2023

Missing SHA2 test #5989

Closed

SparkiDev previously requested changes Feb 16, 2023

View reviewed changes

dgarske removed their assignment Feb 16, 2023

douzzer requested changes Feb 17, 2023

View reviewed changes

gojimmypi force-pushed the New_SHA512_Tests branch 2 times, most recently from efe3ff8 to d400248 Compare February 17, 2023 18:36

dgarske assigned SparkiDev Feb 18, 2023

dgarske requested review from SparkiDev and douzzer February 18, 2023 02:33

douzzer requested changes Feb 18, 2023

View reviewed changes

gojimmypi force-pushed the New_SHA512_Tests branch from b545f59 to 1b3a5c0 Compare February 18, 2023 17:36

gojimmypi marked this pull request as draft February 18, 2023 21:58

gojimmypi marked this pull request as ready for review February 19, 2023 16:48

gojimmypi requested a review from douzzer February 19, 2023 16:48

dgarske self-assigned this Feb 20, 2023

dgarske unassigned SparkiDev Feb 20, 2023

dgarske previously requested changes Feb 20, 2023

View reviewed changes

dgarske assigned gojimmypi and unassigned dgarske and gojimmypi Feb 20, 2023

gojimmypi mentioned this pull request Feb 20, 2023

Add SHA512/224 and SHA512/256 benchmarks #6113

Merged

4 tasks

gojimmypi requested a review from dgarske February 20, 2023 21:12

gojimmypi marked this pull request as draft February 20, 2023 21:50

douzzer requested changes Feb 20, 2023

View reviewed changes

Add new SHA-512/224 and SHA-512/256 tests

136267d

gojimmypi force-pushed the New_SHA512_Tests branch from d42ebf9 to 136267d Compare February 21, 2023 00:55

gojimmypi requested a review from douzzer February 21, 2023 03:14

gojimmypi marked this pull request as ready for review February 21, 2023 03:14

douzzer approved these changes Feb 21, 2023

View reviewed changes

douzzer merged commit e0abb0e into wolfSSL:master Feb 21, 2023

gojimmypi deleted the New_SHA512_Tests branch October 12, 2023 00:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new SHA-512/224 and SHA-512/256 tests #6097

Add new SHA-512/224 and SHA-512/256 tests #6097

gojimmypi commented Feb 16, 2023 •

edited

Loading

SparkiDev Feb 16, 2023

gojimmypi Feb 17, 2023

dgarske Feb 17, 2023

douzzer left a comment

douzzer commented Feb 17, 2023

gojimmypi commented Feb 17, 2023

douzzer Feb 18, 2023

gojimmypi commented Feb 18, 2023

gojimmypi commented Feb 19, 2023

dgarske Feb 20, 2023

gojimmypi Feb 20, 2023

dgarske Feb 20, 2023

gojimmypi Feb 20, 2023

gojimmypi Feb 20, 2023

gojimmypi Feb 20, 2023

dgarske Feb 20, 2023

dgarske Feb 20, 2023

gojimmypi Feb 20, 2023

dgarske Feb 20, 2023

douzzer left a comment

gojimmypi commented Feb 21, 2023

douzzer left a comment

gojimmypi commented Feb 21, 2023

Add new SHA-512/224 and SHA-512/256 tests #6097

Add new SHA-512/224 and SHA-512/256 tests #6097

Conversation

gojimmypi commented Feb 16, 2023 • edited Loading

Description

Testing

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

douzzer left a comment

Choose a reason for hiding this comment

douzzer commented Feb 17, 2023

gojimmypi commented Feb 17, 2023

Choose a reason for hiding this comment

gojimmypi commented Feb 18, 2023

gojimmypi commented Feb 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

douzzer left a comment

Choose a reason for hiding this comment

gojimmypi commented Feb 21, 2023

douzzer left a comment

Choose a reason for hiding this comment

gojimmypi commented Feb 21, 2023

gojimmypi commented Feb 16, 2023 •

edited

Loading