Conversation
Update the vendored third_party/wycheproof_testvectors/rsa_oaep_misc_test
JSON to current upstream and regenerate the .txt via convert_wycheproof.
Upstream reclassified three ciphertext-in-{0,1} vectors from "valid" to
"acceptable" with a new "SmallIntegerCiphertext" flag (C2SP/wycheproof
df4e933), citing FIPS 800-56B Rev. 2 s7.1.2.1 (1 < c < n-1) -- the same
rationale AWS-LC already enforces.
Because the reclassified vectors are now "acceptable" and also carry other
flags (e.g. "Constructed"), WycheproofResult::IsValid returns false for
them, so the manual 1 < c < (n-1) range check in RunWycheproofDecryptTest
is redundant. Remove it and add "SmallIntegerCiphertext" to the accepted
flags. Verified no "acceptable" vector has a flag set that is a subset of
the allowlist and no "valid" vector has c in {0,1}, so decryption behaviour
is unchanged.
sim: aws#3097
Re-sync third_party/vectors from current C2SP/wycheproof main (third_party/vectors/sync.py). This pulls the ECDSA "s is a large power of 2" vectors (s = 2^128 and s = n - 2^128) added in C2SP/wycheproof#238 into every ecdsa_* file the EVP verify tests consume, and refreshes the EdDSA, RSA-PKCS1, DSA, ML-DSA and ML-KEM vectors. The refreshed ML-DSA and ML-KEM files introduce new per-case attributes and optional fields that the strict FileTest parser rejected as unused/missing. Adapt the harnesses: - WycheproofMLDSATest.Verify: stop requiring "flags" (it was read into an unused local and is absent on some new cases; GetWycheproofResult already consumes it when present). - WycheproofMLDSATest.SignWithSeed/SignWithoutSeed: ignore the new "rnd" attribute; skip mu-only internal KATs that carry no "msg" (the message EVP_DigestSign path cannot drive them); make "publicKey" optional in SignWithSeed (unused, omitted for some invalid-key groups); treat "IncorrectPrivateKeyLength" as an expected key-import failure. - WycheproofKEMTest.DecapsNoSeed: read the new "K" (expected shared secret, present only on valid cases) and assert the decapsulated secret matches it; ignore the unused "ek". All 391 EVP/PQDSA/KEM/HPKE tests pass and sync.py --check is clean.
The refreshed ML-DSA sign vectors include NIST ACVP-derived cases that carry only the pre-hashed message representative "mu" (no "msg"). These were being skipped because the message-based EVP_DigestSign path cannot drive them. Exercise them via AWS-LC's external-mu API: for ML-DSA the raw EVP_PKEY_sign / EVP_PKEY_verify path treats its input as the 64-byte external mu. For each such case, verify the known-good KAT signature over mu and round-trip a freshly produced signature over mu, asserting the outcome matches the vector's expected result. This turns 69 previously-skipped mu-only cases across MLDSA-44/65/87 (both the seed and expanded private-key files) into actively verified tests.
|
🔒 Security Review — View Report Please review before merging. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3549 +/- ##
==========================================
+ Coverage 78.24% 78.26% +0.01%
==========================================
Files 700 700
Lines 126282 126305 +23
Branches 17440 17439 -1
==========================================
+ Hits 98810 98850 +40
+ Misses 26594 26579 -15
+ Partials 878 876 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| ASSERT_TRUE(t->GetInstructionBytes(&pk, "publicKey")); | ||
| } | ||
| ASSERT_TRUE(t->GetInstructionBytes(&sk_expanded, "privateKey")); | ||
| ASSERT_TRUE(t->GetBytes(&msg, "msg")); |
There was a problem hiding this comment.
assuming this and flags above were just "present but never used" so safe to remove?
There was a problem hiding this comment.
This refresh isn't strictly additive: the misc file drops from 775 to 395 cases while the 17 dedicated OAEP files remain unchanged. Some of that reduction removes distinct inputs we currently exercise. For example, 2048-bit OAEP with SHA-256/MGF1-SHA-384 previously tested message lengths 0, 1, 4, 6, 7, 20, 32, and 190; it now tests only 0, 6, and 190, with no replacement in the dedicated files. The refresh also adds parameter combinations and preserves the constructed cases, so I'd describe this as a coverage tradeoff rather than a strict improvement. Can we account for the removed cases and retain any coverage we still want?
Can we also bring the refreshed OAEP files into the managed vectors tree and record the upstream revision used? This file remains under third_party/wycheproof_testvectors, whose METADATA still identifies the 2019 snapshot, and is outside sync.py's update/check scope. Refreshing the OAEP family together would avoid maintaining this mixed snapshot. Alternatively, we could split the OAEP change out of this PR.
| // BoringSSL does not enforce policies on weak keys and leaves it to the | ||
| // caller. | ||
| bool is_valid = result.IsValid({"SmallModulus"}); | ||
|
|
||
| // AWS-LC enforces FIPS 800-56B Rev. 2 §7.1.2.1 which requires 1 < c < (n-1). | ||
| // But Wycheproof mistakenly marks some vectors with c values outside this range as valid. | ||
| if (is_valid) { | ||
| const RSA *rsa = EVP_PKEY_get0_RSA(key.get()); | ||
| const BIGNUM *n = RSA_get0_n(rsa); | ||
| bssl::UniquePtr<BIGNUM> c(BN_bin2bn(ct.data(), ct.size(), nullptr)); | ||
| bssl::UniquePtr<BIGNUM> n_minus_one(BN_dup(n)); | ||
| ASSERT_TRUE(c && n_minus_one); | ||
| ASSERT_TRUE(BN_sub_word(n_minus_one.get(), 1)); | ||
| if (BN_is_zero(c.get()) || BN_is_one(c.get()) || | ||
| BN_cmp(c.get(), n_minus_one.get()) >= 0) { | ||
| is_valid = false; | ||
| } | ||
| } | ||
| // caller. AWS-LC enforces FIPS 800-56B Rev. 2 §7.1.2.1, which requires | ||
| // 1 < c < (n-1); Wycheproof marks the out-of-range ciphertexts as | ||
| // "acceptable" with the "SmallIntegerCiphertext" flag. Those vectors also | ||
| // carry other flags (e.g. "Constructed"), so IsValid still returns false | ||
| // for them and AWS-LC is expected to reject, matching our enforcement. | ||
| bool is_valid = result.IsValid({"SmallModulus", "SmallIntegerCiphertext"}); |
There was a problem hiding this comment.
Can we leave SmallIntegerCiphertext out of the allowlist? AWS-LC intentionally rejects these ciphertexts. The three current cases still expect failure only because Constructed and EncryptionWithLabel are not allowlisted; if upstream later removes those additional flags, this would incorrectly expect success.
Keeping the original allowlist expresses our policy directly and still lets us remove the local range-check workaround:
bool is_valid = result.IsValid({"SmallModulus"});| } else { | ||
| EXPECT_FALSE(verify_kat) | ||
| << "External-mu verification succeeded for an invalid KAT signature"; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Can we reproduce the signing KAT here, rather than only verify its signature and perform a randomized round-trip? These vectors include cases specifically constructed to exercise signing rejection conditions and long rejection loops. All 69 mu-only cases omit rnd, which Wycheproof defines as 32 zero bytes. EVP_PKEY_sign instead generates fresh randomness, so it does not reliably exercise the signing path targeted by the vector. Verifying the supplied signature does not exercise that path either.
We already have internal signing APIs that accept rnd, and ACVPSigGen demonstrates the pattern. Could we use the vector's randomness, defaulting to zeroes, and compare the generated signature with sig? The EVP round-trip is useful additional coverage, but not a substitute for the signing KAT.
This branch has not been deployed
4 participants
Addresses #3097 and more
Refresh the vendored Wycheproof vectors to current upstream and drop the stale
RSA-OAEP workaround in
evp_test.cc. Adapt the ML-DSA/ML-KEM harnesses for the reshaped vectors, including the new external-mu vectors.Only modifies test code.
crypto_testpasses. It strictly improves test coverage.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.