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

ExternalMu mode for pre-hash ML-DSA #2113

Merged
merged 36 commits into from
Jan 17, 2025
Merged

ExternalMu mode for pre-hash ML-DSA #2113

merged 36 commits into from
Jan 17, 2025

Conversation

jakemas
Copy link
Contributor

@jakemas jakemas commented Jan 13, 2025

Issues:

Resolves #CryptoAlg-2829

Description of changes:

This PR adds a new signing and verification mode for ML-DSA known as ExternalMu. This signing/verification mode allows for pre-hashing of large messages that are to be signed with ML-DSA.

This PR focuses mainly on connecting EVP_PKEY_sign and EVP_PKEY_verify to PQDSA ASN.1 functionality, to allow users access to these APIs for pre-hash modes of ML-DSA.

I have added a generic function pkey_pqdsa_sign_generic that bifurcates between calls from the EVP_PKEY_METHOD to call pqdsa->method->pqdsa_verify_message when pkey_pqdsa_verify_message is called and pqdsa->method->pqdsa_verify when pkey_pqdsa_verify is called.

These two new PQDSA_METHOD functions pqdsa_sign and pqdsa_verify are as follows:

  int (*pqdsa_sign)(const uint8_t *private_key,
                          uint8_t *sig,
                          size_t *sig_len,
                          const uint8_t *message,
                          size_t message_len);

  int (*pqdsa_verify)(const uint8_t *public_key,
                      const uint8_t *sig,
                      size_t sig_len,
                      const uint8_t *message,
                      size_t message_len);

These allow for the input message to represent the pre-hashed message (hence why ctx_string is removed, as this should already be in the pre-hash).

We can then define the PQDSA_METHOD as:

static const PQDSA_METHOD sig_ml_dsa_44_method = {
  ml_dsa_44_keypair,
  ml_dsa_44_sign,
  ml_dsa_extmu_44_sign,
  ml_dsa_44_verify,
  ml_dsa_extmu_44_verify
};

where ml_dsa_extmu_44_sign and ml_dsa_extmu_44_verify connect to external mu variants of ML-DSA.

Call-outs:

It is important to note that external-mu ML-DSA is considered the same algorithm as FIPS 204 ML-DSA. As such, external-mu ML-DSA shares the same OIDs as ML-DSA. NIST’s AVCP lab have confirmed that they will allow alternate interfaces for FIPS 204: Algorithm 7 (ML-DSA.Sign) and FIPS 204: Algorithm 7 (ML-DSA.Verify). Specifically, they will accept a variant of ML-DSA.Sign/ML-DSA.Sign_internal that directly accepts the pre-message string, and a variant of ML-DSA.Verify/ML-DSA.Verify_internal that directly accepts the pre-message string.

These are implemented as ml_dsa_extmu_sign, ml_dsa_sign_internal and ml_dsa_verify_internal.

Testing:

Added new test suite:
--gtest_filter=All/PerMLDSA*

This includes a new KAT framework. As official extmu KATs are not yet available, we exercise the APIs through the existing KATs for ML-DSA. Three new KAT files MLDSA_EXTMU_{44/65/87}_hedged_pure.txt have been created. These files are exactly the same KATs as MLDSA_{44/65/87}_hedged_pure.txt with the exception that an additional line has been added to each KAT containing mu the result of computing line 6 of Algorithm 7 ML-DSA.Sign_internal and line 7 of Algorithm 8 ML-DSA.Verify_internal.

To prove that mu is equal to H(tr | M') we construct mu from tr and M' and compare with the value in the KAT (this hashing is timely, and slows down testing).

As the current ML-DSA KATs provide the following parameters: count, xi, rng, seed, pk, sk, msg, mlen, sm, smlen, ctx we are able to construct mu manually by constructing mu = shake_256(shake_256(pk), 0 || ctx || ctx_size || msg. That was done once to generate mu values to insert into the existing KAT files. I then compared this value with the actual value produced within sign.c by printing out the constructed mu as an intermediate value.

I reconstruct the construction of mu in the KAT from the same provided parameters, and compare it against the known result within the new KAT for external mu. This way, when alternative test vectors come out they will be a straight swap.

ACVP NIST KATs came out during this PR (how timely)

We run the ACVP test vectors obtained from https://github.com/usnistgov/ACVP-Server within the three functions PerMLDSATest.ACVPKeyGen, PerMLDSATest.ACVPSigGen and PerMLDSATest.ACVPSigVer. These correspond to the tests found at ML-DSA-keyGen-FIPS204, ML-DSA-sigGen-FIPS204, and ML-DSA-sigVer-FIPS204.

To test ML-DSA pure mode:external_mu = False, deterministic = false, we use tgId = 19, 21, 23 of sigGen and tgId = 7, 9, 11 of sigVer.
To test ML-DSA ExternalMu mode:external_mu = true, deterministic = false, we use tgId = 20, 22, 24 of sigGen and tgId = 8, 10, 12 of sigVer.

We also provide functional testing hat:

  • Generates ML-DSA key for each parameter set
  • Pre-hashes a message M by generating mu = SHAKE256(SHAKE256(pk) || 0 || |ctx| || ctx || M)
  • Signs mu with EVP_PKEY_sign
  • Verifies mu with EVP_PKEY_verify
  • Bonus: verifies raw message M using EVP_DigestVerify to show compatibility between signing modes.

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.

@jakemas jakemas requested a review from a team as a code owner January 13, 2025 20:07
@codecov-commenter
Copy link

codecov-commenter commented Jan 13, 2025

Codecov Report

Attention: Patch coverage is 96.98795% with 5 lines in your changes missing coverage. Please review.

Project coverage is 78.96%. Comparing base (68e861e) to head (36e9ae2).

Files with missing lines Patch % Lines
crypto/evp_extra/p_pqdsa.c 84.61% 4 Missing ⚠️
crypto/ml_dsa/ml_dsa_ref/sign.c 95.65% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2113      +/-   ##
==========================================
+ Coverage   78.93%   78.96%   +0.02%     
==========================================
  Files         610      610              
  Lines      105152   105288     +136     
  Branches    14902    14912      +10     
==========================================
+ Hits        83001    83139     +138     
- Misses      21497    21498       +1     
+ Partials      654      651       -3     

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

@jakemas
Copy link
Contributor Author

jakemas commented Jan 13, 2025

Nevine's previous comments:

Have been addressed in this PR.

@jakemas
Copy link
Contributor Author

jakemas commented Jan 14, 2025

I made codecov happier by adding in some negative testing around signature size (this is actually required by FIPS 204 3.6.2 Public-Key and Signature Length Checks) in bd4e629. Signature algorithms should not accept signature sizes smaller or larger than expected -- and we validate this through the EVP API).

As for the rest:

  • the lines in crypto/ml_dsa/ml_dsa_ref/sign.c are untestable
  • the four new lines in crypto/evp_extra/p_pqdsa.c are from 2 sign and 2 verify functions that now bifurcate around the pure vs pre-hash modes. They are hit when there is no defined method for !pqdsa->method->pqdsa_sign_message or !pqdsa->method->pqdsa_sign, similarly for verify. pqdsa methods are well-defined and I have manually verified that these function methods exist. no need to automate here.

nebeid
nebeid previously approved these changes Jan 14, 2025
include/openssl/evp.h Outdated Show resolved Hide resolved
size_t message_len) {
static int pkey_pqdsa_sign_generic(EVP_PKEY_CTX *ctx, uint8_t *sig,
size_t *sig_len, const uint8_t *message,
size_t message_len, int prehash) {
Copy link
Contributor

Choose a reason for hiding this comment

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

To make it clear that this is not HashML-DSA, I suggest replacing prehash with external_mu.

Copy link
Contributor Author

@jakemas jakemas Jan 14, 2025

Choose a reason for hiding this comment

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

Not a good idea. This is the PQDSA logic for ALL PQDSA types. ExternalMu isn't defined for any PQDSA scheme other than ML-DSA. For example, why would SLH-DSA or even HashML-DSA have a flag for ExternalMu?

I have changed all the ML-DSA specific uses of a pre-hash flag to use external_mu but the ones that remain called prehash are intentional so that we can use the flag for the purposes of indicating any pre-hash method for any PQDSA signature scheme.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree this shouldn't be external_mu, but it isn't clear if prehash means the input has been pre-hashed or if AWS-LC should hash the input before signing it, can this follow the method names and use sign_message or sign_digest 0/1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Andrew, have I understood you correctly with this update: 318f0c1

crypto/evp_extra/p_pqdsa.c Outdated Show resolved Hide resolved
crypto/ml_dsa/ml_dsa.h Outdated Show resolved Hide resolved
size_t ctx_string_len);
uint8_t *sig,
size_t *sig_len,
const uint8_t *message,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you like to refer to it as digest or mu here and all the way down until the internal/generic functions are used?

Copy link
Contributor Author

@jakemas jakemas Jan 14, 2025

Choose a reason for hiding this comment

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

We should use digest because its in the PQDSA internal file, so these cover algorithms outside of ML-DSA for which mu means somthing. added in ba1856b

Copy link
Contributor Author

@jakemas jakemas Jan 14, 2025

Choose a reason for hiding this comment

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

I think I got them all the way down. I couldn't change the one inside pkey_pqdsa_verify_generic as it handles both message and digest options. Found a few more here 49580d6

crypto/fipsmodule/evp/digestsign.c Outdated Show resolved Hide resolved
Copy link
Contributor

@WillChilds-Klein WillChilds-Klein left a comment

Choose a reason for hiding this comment

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

This includes a new KAT framework. As official extmu KATs are not yet
available, we exercise the APIs through the existing KATs for ML-DSA. Three new
KAT files MLDSA_EXTMU_{44/65/87}hedged_pure.txt have been created. These files
are exactly the same KATs as MLDSA
{44/65/87}_hedged_pure.txt with the
exception that an additional line has been added to each KAT containing mu the
result of computing line 6 of Algorithm 7 ML-DSA.Sign_internal and line 7 of
Algorithm 8 ML-DSA.Verify_internal.

How was mu calculated here?

Also, we intend to replace these with NIST KATs once available, right?

crypto/fipsmodule/evp/digestsign.c Show resolved Hide resolved
include/openssl/evp.h Show resolved Hide resolved
@jakemas
Copy link
Contributor Author

jakemas commented Jan 15, 2025

How was mu calculated here?

The plan is to re-use the KATs for ML-DSA to provide KATs for externalmu mode. As the current ML-DSA KATs provide the following parameters: count, xi, rng, seed, pk, sk, msg, mlen, sm, smlen, ctx we are able to construct mu manually by constructing mu = shake_256(shake_256(pk), 0 || ctx || ctx_size || msg. That was done once to generate mu values to insert into the existing KAT files. I then compared this value with the actual value produced within sign.c by printing out the constructed mu as an intermediate value.

I reconstruct the construction of mu in the KAT from the same provided parameters, and compare it against the known result within the new KAT for external mu. This way, when alternative test vectors come out they will be a straight swap.

Also, we intend to replace these with NIST KATs once available, right?

Yes we do.

nebeid
nebeid previously approved these changes Jan 15, 2025
nebeid
nebeid previously approved these changes Jan 16, 2025
nebeid
nebeid previously approved these changes Jan 16, 2025
crypto/ml_dsa/ml_dsa_ref/sign.c Show resolved Hide resolved
@jakemas jakemas dismissed stale reviews from WillChilds-Klein and nebeid via 6762c2a January 17, 2025 17:06
@nebeid nebeid enabled auto-merge (squash) January 17, 2025 17:49
@nebeid nebeid merged commit 3263ce2 into aws:main Jan 17, 2025
123 of 126 checks passed
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.

5 participants