-
Notifications
You must be signed in to change notification settings - Fork 830
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new SHA-512/224 and SHA-512/256 tests #6097
Conversation
0bf5497
to
0a6deda
Compare
wolfcrypt/test/test.c
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The NIST hash functions link has been removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, the link is fine, but it needs to go next to the vectors (assuming you used them from there).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Per
|
efe3ff8
to
d400248
Compare
@douzzer I added some |
wolfcrypt/src/sha512.c
Outdated
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
b545f59
to
1b3a5c0
Compare
TL;DR seems to still be bad gate logic for new tests? Thanks everyone for helping to craft the new
and
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:
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 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 |
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. |
src/ssl.c
Outdated
@@ -19,6 +19,7 @@ | |||
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335, USA | |||
*/ | |||
|
|||
/* revert to master test */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of all these comments revert to master test
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} | ||
|
||
#if !defined(WOLFSSL_NOSHA512_224) && \ | ||
!defined(HAVE_FIPS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why doesn't this match the above FIPS logic at line 419?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, wait: line 419. Let me correct that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
#endif /* !defined(WOLFSSL_NOSHA512_224) ... */ | ||
|
||
#if !defined(WOLFSSL_NOSHA512_256) && \ | ||
!defined(HAVE_FIPS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, why does not not match FIPS logic at line 423?
@@ -3003,8 +3035,311 @@ WOLFSSL_TEST_SUBROUTINE int sha512_test(void) | |||
|
|||
return ret; | |||
} | |||
|
|||
#if !defined(WOLFSSL_NOSHA512_224) && \ | |||
!defined(HAVE_FIPS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same. Why not (!defined(HAVE_FIPS) || FIPS_VERSION_GE(5, 2)) && !defined(HAVE_SELFTEST)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
|
||
#if !defined(WOLFSSL_NOSHA512_256) && \ | ||
!defined(HAVE_FIPS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same. Why not (!defined(HAVE_FIPS) || FIPS_VERSION_GE(5, 2)) && !defined(HAVE_SELFTEST)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
d42ebf9
to
136267d
Compare
TADA! The 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Note to others and future self: For the wolfssl-fips project I needed to set Configuration to |
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:
And tested in VisualGDB for Espressif ESP-IDF project in IDE/Espressif/ESP-IDF/examples/wolfssl_test with the following settings:
Note that for the ESP32, the tests are all successful with this:
But fail when all hardware encryption is turned on:
Checklist