Skip to content
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 runtime options to break the pairwise consistency test for Ed, ML-KEM, and ML-DSA #2192

Merged
merged 1 commit into from
Feb 17, 2025

Conversation

andrewhop
Copy link
Contributor

Description of changes:

To easily demonstrate the pairwise consistency tests function this change adds support to the existing BORINGSSL_FIPS_BREAK_TEST environment variable.

Testing:

This adds a new test file for all the runtime tests and runs them from the run_fips_test.sh script on the break-able build.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@andrewhop andrewhop requested a review from a team as a code owner February 12, 2025 20:15
@codecov-commenter
Copy link

codecov-commenter commented Feb 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.05%. Comparing base (154f998) to head (762bc9d).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2192   +/-   ##
=======================================
  Coverage   79.05%   79.05%           
=======================================
  Files         612      612           
  Lines      106159   106159           
  Branches    15002    15002           
=======================================
+ Hits        83923    83927    +4     
+ Misses      21582    21579    -3     
+ Partials      654      653    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

smittals2
smittals2 previously approved these changes Feb 14, 2025
torben-hansen
torben-hansen previously approved these changes Feb 15, 2025

if [ ! -f $TEST_FIPS_BIN ]; then
echo "$TEST_FIPS_BIN is missing. Run this script from the top level of a"
echo "BoringSSL checkout and ensure that ./build-fips-break-test-binaries.sh"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
echo "BoringSSL checkout and ensure that ./build-fips-break-test-binaries.sh"
echo "AWS-LC checkout and ensure that ./build-fips-break-test-binaries.sh"

?

@andrewhop andrewhop dismissed stale reviews from torben-hansen and smittals2 via 762bc9d February 15, 2025 19:26
@andrewhop andrewhop force-pushed the pct_cast_all_the_things branch from 1be4999 to 762bc9d Compare February 15, 2025 19:26
@@ -36,6 +36,7 @@

#include "../../crypto/fipsmodule/evp/internal.h"
#include "../../crypto/fipsmodule/kem/internal.h"
#include "../../crypto/fipsmodule/pqdsa/internal.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broader question, what is the purpose of test_fips.c. My recollection is that it is an abomination of a file with a very large main function, and I'm pretty sure doesn't include support for everything that is FIPS approved. Honestly it feels a bit duplicative to what should be in crypto_test?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example I'm pretty sure this doesn't have ed25519ph tested :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct, test_fips is a big file that turns into an executable with some of the algorithms. I think this file was used by BoringSSL to demonstrate certain things to their lab. We now use it in our CI to run/verify the break tests work as expected. In this case I needed to add algorithms to it to trigger the lazy self tests.

The break-tests could use crypto_test, but it's a little hard to write tests for the FIPS failure abort cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need ed25519ph because this change is only adding support to break the pairwise consistency tests and all the Ed stuff uses the same keygen function. However I did need to add the the pre-hash changes to test breaking the self test and that's already checked in https://github.com/aws/aws-lc/blob/main/util/fipstools/test_fips.c#L416-L430.

@andrewhop andrewhop merged commit ca99b6f into aws:main Feb 17, 2025
117 of 119 checks passed
@hanno-becker
Copy link
Contributor

hanno-becker commented Feb 19, 2025

@andrewhop Could BORINGSSL_FIPS_BREAK_TEST be changed from a string to a numeric value? This would remove the need for the runtime getenv + strcmp check, and would instead allow to know statically whether a PCT breakage should be introduced. In mlkem-native, I could then similarly add a MLK_BREAK_PCT, and set that statically in the compatibility layer between BoringSSL<->mlkem-native.

@hanno-becker
Copy link
Contributor

hanno-becker commented Feb 19, 2025

For the time being, I opened we merged pq-code-package/mlkem-native#788 to basically mirror AWS-LC's runtime behaviour; #2176 is updated accordingly. I'd still prefer not having to do this, though, and understand if/why the runtime logic is needed here.

@hanno-becker hanno-becker mentioned this pull request Feb 19, 2025
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants