Rehaul PQDSA Test Suite #2062

jakemas · 2024-12-17T19:53:37Z

Issues:

Resolves #CryptoAlg-2800

Description of changes:

FIPS 204 states:

Algorithm 3, implementing verification for ML-DSA, and Algorithm 5, implementing verification for HashMLDSA, specify the length of the signature sigma and the public key pk in terms of the parameters described in Table 1 of FIPS 204. If an implementation of ML-DSA can accept inputs for sigma or pk of any other length, it shall return false whenever the lengths of either of these inputs differ from their lengths specified in this standard.

This description outlines how these checks are performed within our implementation of PQDSA in AWS-LC.

Signature Length

When attempting to verify a signature, the user will call:

EVP_DigestVerifyInit(ctx.get(), nullptr, nullptr, nullptr, pkey.get());
EVP_DigestVerify(ctx.get(), signature.data(), sig_len, msg.data(), mlen_int);

This will call into the PKEY function method for ML-DSA verify pkey_pqdsa_verify_signature, defined within p_pqdsa.c. This function performs the check that the signature length provided matches the expected signature length defined for the particular algorithm:

  if (sig_len != pqdsa->signature_len ||
      !pqdsa->method->verify(key->public_key, sig, siglen, message, message_len, NULL, 0)) {
    OPENSSL_PUT_ERROR(EVP, EVP_R_INVALID_SIGNATURE);
    return 0;
  }

We are able to explicitly prove these failure modes within the PQDSA test suite PQDSAParameterTest. The SIGOperations test has been modified to include a check that signature verification fails when providing a signature of invalid length. For example:

  // Check that verification fails upon providing a signature of invalid length
  sig_len = GetParam().signature_len - 1;
  ASSERT_FALSE(EVP_DigestVerify(md_ctx_verify.get(), sig1.data(), sig_len, msg1.data(), msg1.size()));
  err = ERR_get_error();
  EXPECT_EQ(ERR_LIB_EVP, ERR_GET_LIB(err));
  EXPECT_EQ(EVP_R_INVALID_SIGNATURE, ERR_GET_REASON(err));

Public Key Length

As the length of the public key is not provided to the verify API, we are not able to validate the size in the same way as signature length. However, as the public key is provided via a PKEY structure, we can enforce the requirement that prevents public keys of the incorrect length by checking the size of the public key during it’s addition to the PKEY structure.

How do ML-DSA keys enter EVP Verify?

The EVP API for verify init is as follows:

EVP_DigestVerifyInit(&md_ctx_verify, nullptr, nullptr, nullptr, key);

Where the public key either enters explicitly as a PKEY argument key or through the EVP_MD_CTX argument md_ctx_verify as a pctx.

Thus, to show that verify will only accept public key inputs of the correct length, we must evidence that:
we cannot construct a PQDSA PKEY such that the public/private key is not of the size required for that parameter set.

We now verify for every method to construct an PQDSA PKEY that the size of the public/private key are exactly the size required for that parameter set. The methods to construct an PQDSA PKEY are as follows:

use EVP_PKEY_pqdsa_new_raw_public_key
use EVP_PKEY_pqdsa_new_raw_private_key
use EVP_parse_public_key
use EVP_parse_private_key
Create a valid PQDSA keypair, then set the parameters to a different parameter set using EVP_PKEY_CTX_pqdsa_set_params

We now discuss each method, outlining any testing that has been added to provide evidence that this cannot occur (where necessary).

(1) EVP_PKEY_pqdsa_new_raw_public_key

This function takes three inputs; nid, in, len, where:

nid is the algorithm identifier
in is the contents of the public key (raw)
len is the length of in

We test failure modes for all three input parameters within the RawFunctions tests as part of the PQDSA test suite PQDSAParameterTest. In particular, we verify that the length of the input public key len must be exactly equal to the expected length of the associated public key for that parameter set. Explicitly, this is verified within the function EVP_PKEY_pqdsa_new_raw_public_key by the check:

p_pqdsa.c lines 217-220

  if (pqdsa->public_key_len != len) {
    OPENSSL_PUT_ERROR(EVP, EVP_R_INVALID_BUFFER_SIZE);
    goto err;
  }

(2) EVP_PKEY_pqdsa_new_raw_private_key

Similarly to above, this function takes three inputs; nid, in, len, where:

nid is the algorithm identifier
in is the contents of the private key (raw)
len is the length of in

Failure modes are tested within the RawFunctions tests as part of the PQDSA test suite PQDSAParameterTest. In particular, we verify that the length of the input private key len must be exactly equal to the expected length of the associated private key for that parameter set. Explicitly, this is verified within the function EVP_PKEY_pqdsa_new_raw_private_key by the check:

p_pqdsa.c lines 247-250

 if (pqdsa->private_key_len != len) {
    OPENSSL_PUT_ERROR(EVP, EVP_R_INVALID_BUFFER_SIZE);
    goto err;
  }

(3) EVP_parse_public_key

EVP_parse_public_key will call the specific PQDSA ASN.1 decode function pqdsa_pub_decode, which in turn will call PQDSA_KEY_set_raw_public_key. This function performs a lookup of the expected public key size from the parameter set, and duplicates exactly that many bytes from the input buffer to the PKEY’s public key. Disallowing any other size value.

pqdsa.c line 71

key->public_key = OPENSSL_memdup(in, key->pqdsa->public_key_len);

(4) EVP_parse_private_key

EVP_parse_private_key will call the specific PQDSA ASN.1 decode function pqdsa_priv_decode, which in turn will call PQDSA_KEY_set_raw_private_key. This function performs a lookup of the expected private key size from the parameter set, and duplicates exactly that many bytes from the input buffer to the PKEY’s private key. Disallowing any other size value.

pqdsa.c line 80

key->private_key = OPENSSL_memdup(in, key->pqdsa->private_key_len);

(5) EVP_PKEY_CTX_pqdsa_set_params

Theoretically, we could create bad keypairs by misaligning a PKEY with the associated parameter set. The function that sets PQDSA parameter sets is EVP_PKEY_CTX_pqdsa_set_params, that takes the arguments:

ctx the input EVP_PKEY_CTX that we want to set the parameter set on
nidthe algorithm identifier

Failure modes are tested within the KeyGen tests as part of the PQDSA test suite PQDSAParameterTest. In particular, we verify that:

the function fails with error ERR_R_PASSED_NULL_PARAMETER when ctx is null.
the function fails with error ERR_R_PASSED_NULL_PARAMETER when ctx->data is null.
the function fails with error EVP_R_INVALID_OPERATION when ctx->pkey is not null.
the function fails with error EVP_R_UNSUPPORTED_ALGORITHM when nid is not a PQDSA.
calling EVP_PKEY_pqdsa_set_params on an initalized PKEY will null the public/private keys

Call-outs:

As we prepare to remove the enable_dilithium flag, I am pre-empting missing test coverage.
The SIGOperations test has been improved to the following flow:

KeyGen: generate PQDSA key for that NID
Signature Initialization: initialize sign with EVP_DigestSignInit
Signature Allocation: call EVP_DigestSign with signature length as null to get the expected size of the signature
1. Verify that these sizes are as expected
Signature Generation: call EVP_DigestSign with correct signature length
Verification Init: init verification by calling EVP_VerifyInit
Verification: call EVP_DigestVerify to verify signature
Verify the signed message fails upon a different message
verify the signed message fails upon a different public key
sign a second message
check signatures are not equal
check that verify fails when trying to use a different signature
check that signatures are not deterministic: call another signature on the same message
size checks: invalid signature lengths

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.

codecov-commenter · 2024-12-18T18:05:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.78%. Comparing base (7601864) to head (cc57ad1).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2062      +/-   ##
==========================================
+ Coverage   78.77%   78.78%   +0.01%     
==========================================
  Files         598      598              
  Lines      103744   103744              
  Branches    14734    14732       -2     
==========================================
+ Hits        81720    81736      +16     
+ Misses      21371    21357      -14     
+ Partials      653      651       -2

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

nebeid

Thank you for documenting these tests so thoroughly, Jake, and for the detailed PR description. Most comments are Nits. One return value is to be discussed.

nebeid · 2024-12-20T16:39:18Z

crypto/dilithium/p_pqdsa_test.cc

+  bssl::UniquePtr<EVP_PKEY> public_pkey(EVP_PKEY_pqdsa_new_raw_public_key(nid, pkey->pkey.pqdsa_key->public_key, pk_len));
+  bssl::UniquePtr<EVP_PKEY> private_pkey(EVP_PKEY_pqdsa_new_raw_private_key(nid, pkey->pkey.pqdsa_key->private_key, sk_len));


It's better to move the second line for private_key right before the checks so it's more readable.

I actually disagree here, I was inspired by Dusan's extremely clean and readable tests for ML-KEM that order the code systematically, calling similar functionality together, for example see:
https://github.com/jakemas/aws-lc/blob/ab8953b7478f72a2b80497cf54ce0b72ed719cfc/crypto/evp_extra/evp_extra_test.cc#L2528-L2533

(Also, it's used just 7 lines down from where it's defined, which seems more readable personally to me)

nebeid · 2024-12-20T16:50:07Z

crypto/dilithium/p_pqdsa_test.cc

+  ASSERT_FALSE(EVP_PKEY_CTX_pqdsa_set_params(ctx.get(), nid));
+  err = ERR_get_error();
+  EXPECT_EQ(ERR_LIB_EVP, ERR_GET_LIB(err));
+  EXPECT_EQ(ERR_R_PASSED_NULL_PARAMETER, ERR_GET_REASON(err));


I prefer ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED in this case because there is not passed in null parameter

Hmm, we set ctx->data to NULL, then pass in ctx, expecting a failure because of a null value. This is consistant with how we use the error code in ML-KEM at https://github.com/jakemas/aws-lc/blob/ab8953b7478f72a2b80497cf54ce0b72ed719cfc/crypto/evp_extra/evp_extra_test.cc#L2229

It's good to be consistent, I agree. We can later revisit it to see if we want to return another error message. I believe the data would be null if the context wasn't initialised or something.

Shall we leave as is for now then? In my opinion the most useful such a generic error message can be is when it is consistent with other failures cases in the library.

nebeid · 2024-12-20T16:54:15Z

crypto/dilithium/p_pqdsa_test.cc

+// 1. Creates a |EVP_PKEY_CTX| object of type: EVP_PKEY_PQDSA.
+// 2. Sets the specific PQDSA parameters according to the |pqdsa_nid| provided.
+// 3. Generates a key pair.
+// 4. Creates an EVP PKEY object from the generated key (as a bssl::UniquePtr).


Suggested change

// 4. Creates an EVP PKEY object from the generated key (as a bssl::UniquePtr).

// 4. Creates an EVP_PKEY object from the generated key (as a bssl::UniquePtr).

fixed in f7d1dad

nebeid · 2024-12-20T16:55:53Z

crypto/dilithium/p_pqdsa_test.cc

 }

 TEST_P(PQDSAParameterTest, KeyCmp) {
-  // Generate two MLDSA keys are check that they are not equal.
+  // Generate two PQDSA keys are check that they are not equal.


Suggested change

// Generate two PQDSA keys are check that they are not equal.

// Generate two PQDSA keys and check that they are not equal.

fixed in f7d1dad

nebeid · 2024-12-20T17:02:02Z

crypto/dilithium/p_pqdsa_test.cc

+  bssl::UniquePtr<EVP_PKEY> public_pkey(EVP_PKEY_pqdsa_new_raw_public_key(nid, pkey->pkey.pqdsa_key->public_key, pk_len));
+  bssl::UniquePtr<EVP_PKEY> private_pkey(EVP_PKEY_pqdsa_new_raw_private_key(nid, pkey->pkey.pqdsa_key->private_key, sk_len));


Lines length

fixed in f7d1dad

nebeid · 2024-12-20T17:07:20Z

crypto/dilithium/p_pqdsa_test.cc

  uint32_t err = ERR_get_error();
  EXPECT_EQ(ERR_LIB_EVP, ERR_GET_LIB(err));
-  EXPECT_EQ(EVP_R_BUFFER_TOO_SMALL, ERR_GET_REASON(err));
-  OPENSSL_free(buf);
-  buf = nullptr;
+  EXPECT_EQ(ERR_R_PASSED_NULL_PARAMETER, ERR_GET_REASON(err));


If you want, these three lines that repeat can be made into a macro. I think all of them are ERR_LIB_EVP.

I'm not sure I understand exactly what you mean here. Open to suggestions.

These 3 lines

uint32_t err = ERR_get_error(); EXPECT_EQ(ERR_LIB_EVP, ERR_GET_LIB(err)); EXPECT_EQ(ERR_R_PASSED_NULL_PARAMETER, ERR_GET_REASON(err));

repeat throughout the tests and can be replaced by a single-line macro that takes the reason as a parameter (similar to the CMP macros).
It will condense the code a bit.

16f48d6 done

nebeid · 2024-12-20T17:21:18Z

crypto/dilithium/p_pqdsa_test.cc

-  ASSERT_FALSE(EVP_DigestVerify(md_ctx.get(), signature1.data(), sig_len,
-                                msg2.data(), msg2.size()));
+  // ---- 3. Test signature failure modes: incompatible messages/signatures ----
+  // Check that the verification of signature1 failes for a different message; message2


Suggested change

// Check that the verification of signature1 failes for a different message; message2

// Check that the verification of signature1 fails for a different message; message2

fixed in f7d1dad

nebeid · 2024-12-20T17:22:14Z

crypto/dilithium/p_pqdsa_test.cc

-                             msg1.data(), msg1.size()));
+  // ---- 2. Test signature round trip (sign + verify) ----
+
+  // Initalize the signing context |md_ctx| with the |pkey| we generated


If you want, you can replace in the comments
message1 -> msg1
message2 -> msg2

fixed in f7d1dad

nebeid · 2024-12-20T17:27:05Z

crypto/dilithium/p_pqdsa_test.cc

+  md_ctx.Reset();
+  md_ctx_verify.Reset();
+
+  // PQDSA signature schemes can be in either randomized (every signature on a


Suggested change

// PQDSA signature schemes can be in either randomized (every signature on a

// PQDSA signature schemes can be either in randomized (every signature on a

fixed in f7d1dad

…sa-test-suite

nebeid · 2024-12-20T18:17:36Z

crypto/dilithium/p_pqdsa_test.cc

+  bssl::UniquePtr<EVP_PKEY>public_pkey(
+    EVP_PKEY_pqdsa_new_raw_public_key(nid, pkey->pkey.pqdsa_key->public_key, pk_len));
+  bssl::UniquePtr<EVP_PKEY> private_pkey(
+    EVP_PKEY_pqdsa_new_raw_private_key(nid, pkey->pkey.pqdsa_key->private_key, sk_len));

  // check that public key is present and private key is not present


Nit: I suggest then to add to this comment "in public_key" and to the one on l. 1282 "in private_key" because I got a bit confused about what the comment meant.

added in 463aa0d#diff-d043ba12b97b57a0d69c31db07b10cee8ca8c05cf41e1875981b254bc7cbe670L1186-L1189

nebeid · 2024-12-20T18:23:51Z

crypto/dilithium/p_pqdsa_test.cc

  uint32_t err = ERR_get_error();
  EXPECT_EQ(ERR_LIB_EVP, ERR_GET_LIB(err));
-  EXPECT_EQ(EVP_R_BUFFER_TOO_SMALL, ERR_GET_REASON(err));
-  OPENSSL_free(buf);
-  buf = nullptr;
+  EXPECT_EQ(ERR_R_PASSED_NULL_PARAMETER, ERR_GET_REASON(err));


These 3 lines

uint32_t err = ERR_get_error(); EXPECT_EQ(ERR_LIB_EVP, ERR_GET_LIB(err)); EXPECT_EQ(ERR_R_PASSED_NULL_PARAMETER, ERR_GET_REASON(err));

repeat throughout the tests and can be replaced by a single-line macro that takes the reason as a parameter (similar to the CMP macros).
It will condense the code a bit.

crypto/dilithium/p_pqdsa_test.cc

rehaul test suite

1da78fb

jakemas requested a review from a team as a code owner December 17, 2024 19:53

jakemas added 2 commits December 17, 2024 11:53

spacing nit

566d5ce

Merge branch 'main' into mldsa-test-suite

4cb2f9d

jakemas mentioned this pull request Dec 17, 2024

Update BoringSSL benchmark to use C++17 #2063

Merged

Merge branch 'main' into mldsa-test-suite

43e8954

nebeid reviewed Dec 20, 2024

View reviewed changes

jakemas added 2 commits December 20, 2024 09:49

CR fixes

f7d1dad

Merge branch 'mldsa-test-suite' of github.com:jakemas/aws-lc into mld…

b489226

…sa-test-suite

nebeid reviewed Dec 20, 2024

View reviewed changes

added macro, CR fixes

463aa0d

jakemas commented Dec 20, 2024

View reviewed changes

crypto/dilithium/p_pqdsa_test.cc Outdated Show resolved Hide resolved

jakemas added 2 commits December 20, 2024 11:12

added test macro for err, reason

f7f1fbd

added additional test

16f48d6

dkostic previously approved these changes Dec 20, 2024

View reviewed changes

CR

fe9e43a

jakemas dismissed dkostic’s stale review via fe9e43a December 20, 2024 20:19

Merge branch 'main' into mldsa-test-suite

8b02f45

dkostic approved these changes Dec 20, 2024

View reviewed changes

nebeid approved these changes Dec 20, 2024

View reviewed changes

Merge branch 'main' into mldsa-test-suite

cc57ad1

nebeid enabled auto-merge (squash) December 20, 2024 21:39

nebeid merged commit 480206c into aws:main Dec 20, 2024
125 of 128 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rehaul PQDSA Test Suite #2062

Rehaul PQDSA Test Suite #2062

jakemas commented Dec 17, 2024 •

edited

Loading

codecov-commenter commented Dec 18, 2024 •

edited

Loading

nebeid left a comment

nebeid Dec 20, 2024

jakemas Dec 20, 2024

nebeid Dec 20, 2024

jakemas Dec 20, 2024

nebeid Dec 20, 2024

jakemas Dec 20, 2024 •

edited

Loading

nebeid Dec 20, 2024

jakemas Dec 20, 2024

nebeid Dec 20, 2024

jakemas Dec 20, 2024

nebeid Dec 20, 2024

jakemas Dec 20, 2024

nebeid Dec 20, 2024

jakemas Dec 20, 2024

nebeid Dec 20, 2024

jakemas Dec 20, 2024

nebeid Dec 20, 2024

jakemas Dec 20, 2024

nebeid Dec 20, 2024

jakemas Dec 20, 2024

nebeid Dec 20, 2024

jakemas Dec 20, 2024

nebeid Dec 20, 2024

jakemas Dec 20, 2024

nebeid Dec 20, 2024

		bssl::UniquePtr<EVP_PKEY> public_pkey(EVP_PKEY_pqdsa_new_raw_public_key(nid, pkey->pkey.pqdsa_key->public_key, pk_len));
		bssl::UniquePtr<EVP_PKEY> private_pkey(EVP_PKEY_pqdsa_new_raw_private_key(nid, pkey->pkey.pqdsa_key->private_key, sk_len));

	// 4. Creates an EVP PKEY object from the generated key (as a bssl::UniquePtr).
	// 4. Creates an EVP_PKEY object from the generated key (as a bssl::UniquePtr).

	// Generate two PQDSA keys are check that they are not equal.
	// Generate two PQDSA keys and check that they are not equal.

	// Check that the verification of signature1 failes for a different message; message2
	// Check that the verification of signature1 fails for a different message; message2

	// PQDSA signature schemes can be in either randomized (every signature on a
	// PQDSA signature schemes can be either in randomized (every signature on a

Rehaul PQDSA Test Suite #2062

Rehaul PQDSA Test Suite #2062

Conversation

jakemas commented Dec 17, 2024 • edited Loading

Issues:

Description of changes:

Signature Length

Public Key Length

Call-outs:

codecov-commenter commented Dec 18, 2024 • edited Loading

Codecov Report

nebeid left a 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

jakemas Dec 20, 2024 • edited Loading

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

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

jakemas commented Dec 17, 2024 •

edited

Loading

codecov-commenter commented Dec 18, 2024 •

edited

Loading

jakemas Dec 20, 2024 •

edited

Loading