Support non-empty context strings in ML-DSA EVP sign/verify#3135
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3135 +/- ##
==========================================
+ Coverage 78.11% 78.14% +0.03%
==========================================
Files 689 689
Lines 122932 123057 +125
Branches 17105 17110 +5
==========================================
+ Hits 96030 96165 +135
+ Misses 26002 25994 -8
+ Partials 900 898 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
de832df to
8676c3d
Compare
Add sign_with_context and verify_sig_with_context methods to PqdsaKeyPair and PqdsaVerificationAlgorithm respectively, allowing callers to specify FIPS 204 context strings (up to 255 bytes) for ML-DSA operations. These methods use the existing EVP_PKEY_CTX_set_signature_context FFI binding (already available in aws-lc-sys) via the EVP_PKEY_CTX_consumer closure pattern. Empty contexts are equivalent to the existing context-free sign/verify methods. Update the mldsa_sigver_test macro to exercise verify_sig_with_context with test vector context strings. Add dedicated test covering round-trip sign+verify with context, mismatched context failure, empty context backward compatibility, >255 byte rejection, and max-length (255 byte) acceptance. Depends on aws/aws-lc#3135.
| GUARD_PTR(dctx); | ||
| GUARD_PTR(sctx); | ||
|
|
||
| dctx->pqdsa = sctx->pqdsa; |
There was a problem hiding this comment.
This does a shallow copy. It works in this case because the object has static-storage (returned by PQDSA_find_dsa_by_nid()). But in general this pattern doesn't work.
Can leave as-is, but it's worth making a code comment about it - which would also help guide agents and silence bad tooling.
|
FWIW in boringssl this is named |
Plumb FIPS 204 context strings through the EVP_DigestSign/EVP_DigestVerify code path for ML-DSA. The lower-level ml_dsa_*_sign/verify functions already accept ctx_string parameters but they were hardcoded to (NULL, 0) in the EVP layer. Changes: - Extend PQDSA_PKEY_CTX with a context[255] buffer and context_len - Add pkey_pqdsa_ctrl handling EVP_PKEY_CTRL_SIGNING_CONTEXT and EVP_PKEY_CTRL_GET_SIGNING_CONTEXT (reusing the existing generic EVP_PKEY_CTX_set_signature_context API from Ed25519ph) - Add pkey_pqdsa_copy to support EVP_PKEY_CTX_dup with context state - Pass dctx->context/context_len to pqdsa_sign_message and pqdsa_verify_message instead of NULL, 0 - Update Wycheproof test helpers to use EVP_PKEY_CTX_set_signature_context via EVP_DigestSign/Verify instead of manually computing ExternalMu - Add ContextString unit test covering: round-trip sign+verify with context, mismatched context failure, empty-context backward compatibility, >255 byte rejection, and max-length (255 byte) acceptance All three ML-DSA variants (44, 65, 87) are covered. Default behavior (empty context string) is unchanged.
- Add NULL check for params->context in pkey_pqdsa_ctrl - Add EVP_PKEY_CTX_get0_signature_context round-trip tests - Document that context string is ignored for EVP_PKEY_sign/verify - Rename EVP_PKEY_CTX_set_signature_context to EVP_PKEY_CTX_set1_signature_context_string to match BoringSSL
- Retain backward-compatible EVP_PKEY_CTX_set_signature_context as a wrapper around the renamed EVP_PKEY_CTX_set1_signature_context_string to avoid breaking existing Ed25519ph consumers. - Improve NULL-safety in SIGNING_CONTEXT ctrl handlers for both PQDSA and Ed25519ph: allow zero-length context with NULL pointer, cleanse the buffer before copying, and use EVP_R_INVALID_PARAMETERS error code. - Extend max-length context test (255 bytes) to perform a full sign and verify round-trip instead of only testing the setter. - Add test for EVP_PKEY_CTX_dup preserving the context string. - Fix long line wrapping in pqdsa_sign_message and pqdsa_verify_message call sites. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Jake Massimo <jakemas@amazon.com>
8c700cc to
15fe4a3
Compare
- Retain backward-compatible EVP_PKEY_CTX_set_signature_context as a wrapper around the renamed EVP_PKEY_CTX_set1_signature_context_string to avoid breaking existing Ed25519ph consumers. - Improve NULL-safety in SIGNING_CONTEXT ctrl handlers for both PQDSA and Ed25519ph: allow zero-length context with NULL pointer, cleanse the buffer before copying, and use EVP_R_INVALID_PARAMETERS error code. - Extend max-length context test (255 bytes) to perform a full sign and verify round-trip instead of only testing the setter. - Add test for EVP_PKEY_CTX_dup preserving the context string. - Fix long line wrapping in pqdsa_sign_message and pqdsa_verify_message call sites. Signed-off-by: Jake Massimo <jakemas@amazon.com>
15fe4a3 to
b21b107
Compare
| // Note: the context string is only used with |EVP_DigestSign|/|EVP_DigestVerify| | ||
| // (message signing). It is ignored when using |EVP_PKEY_sign|/|EVP_PKEY_verify| | ||
| // (digest signing). |
There was a problem hiding this comment.
NP: This is accurate for ML-DSA, but for Ed25519ph the context is used in the EVP_PKEY_sign/EVP_PKEY_verify path. Should the note be narrowed to ML-DSA?
// Note: for ML-DSA, the context string is only used with
// |EVP_DigestSign|/|EVP_DigestVerify| (message signing). It is ignored when
// using |EVP_PKEY_sign|/|EVP_PKEY_verify| (digest signing).There was a problem hiding this comment.
Narrowed the note to ML-DSA in 848ed20. The comment now explicitly calls out that the EVP_PKEY_sign/EVP_PKEY_verify exclusion applies to ML-DSA because the pre-hashed mu input already encodes the context per FIPS 204 §5.3.
| // | ||
| // It returns one on success and zero on error. | ||
| static int SignMLDSAWithContext(EVP_PKEY *pkey, std::vector<uint8_t> &sig, | ||
| const std::vector<uint8_t> &pk, |
There was a problem hiding this comment.
NP: I don't think pk is used now.
There was a problem hiding this comment.
Dropped the unused pk parameter from SignMLDSAWithContext (and updated all callers) in 848ed20.
| // | ||
| // It returns one on success and zero on error. | ||
| static int VerifyMLDSAWithContext(EVP_PKEY *pkey, | ||
| const std::vector<uint8_t> &pk, |
There was a problem hiding this comment.
NP: I don't think pk is used any more.
There was a problem hiding this comment.
Dropped the unused pk parameter from VerifyMLDSAWithContext (and updated all callers) in 848ed20.
| int EVP_PKEY_CTX_set_signature_context(EVP_PKEY_CTX *ctx, | ||
| const uint8_t *context, | ||
| size_t context_len) { | ||
| return EVP_PKEY_CTX_set1_signature_context_string(ctx, context, context_len); | ||
| } |
There was a problem hiding this comment.
NP: Could one of the tests call EVP_PKEY_CTX_set_signature_context instead of EVP_PKEY_CTX_set1_signature_context_string just to ensure coverage for it?
There was a problem hiding this comment.
Switched the verify-side setup in the ContextString test to call EVP_PKEY_CTX_set_signature_context in 848ed20, so both setter names are covered in the round-trip.
| else { | ||
| if (message_len != pqdsa->digest_len) { |
There was a problem hiding this comment.
NP: Should we insert another error condition here?
// For ML-DSA, the digest-sign path (EVP_PKEY_sign) takes a pre-hashed
// |mu| input which already encodes the context string per FIPS 204
// section 5.3. Applying a separately-configured context here would be
// silently ignored and produce a signature inconsistent with the
// caller's intent, so reject the combination explicitly.
if (dctx->context_len > 0) {
OPENSSL_PUT_ERROR(EVP, EVP_R_INVALID_OPERATION);
return 0;
}
There was a problem hiding this comment.
Added the guard on the digest-sign path in 848ed20. A non-empty dctx->context_len now returns EVP_R_INVALID_OPERATION with a comment explaining that the pre-hashed mu input already encodes the context per FIPS 204 §5.3, so a separately-configured context would otherwise be silently ignored.
| else { | ||
| if (message_len != pqdsa->digest_len) { |
There was a problem hiding this comment.
NP: Similar error condition here:
if (dctx->context_len > 0) {
OPENSSL_PUT_ERROR(EVP, EVP_R_INVALID_OPERATION);
return 0;
}
There was a problem hiding this comment.
Added the symmetric guard on the digest-verify path in 848ed20, mirroring the sign-side check.
- Narrow the context-string note in evp.h to ML-DSA since Ed25519ph does use the context in the |EVP_PKEY_sign|/|EVP_PKEY_verify| path. - Reject a non-empty context on the ML-DSA digest-sign/verify path with |EVP_R_INVALID_OPERATION|: the pre-hashed |mu| input already encodes the context per FIPS 204 section 5.3, so a separately-configured context would be silently ignored and produce output inconsistent with caller intent. - Exercise the backward-compatible |EVP_PKEY_CTX_set_signature_context| wrapper in the |ContextString| verify-side setup for coverage. - Drop the unused |pk| parameter from |SignMLDSAWithContext| and |VerifyMLDSAWithContext| test helpers and update all callers. Signed-off-by: Jake Massimo <jakemas@amazon.com>
justsmth
left a comment
There was a problem hiding this comment.
Looks good. Just a couple of minor concerns...
| // |EVP_DigestSign|/|EVP_DigestVerify| (message signing). It is ignored when | ||
| // using |EVP_PKEY_sign|/|EVP_PKEY_verify| (digest signing), because the pre- |
There was a problem hiding this comment.
NP: Suggested tweak: s/is ignored/is not permitted and will return an error/ (or similar).
There was a problem hiding this comment.
Applied in df71998. The note on |EVP_PKEY_CTX_set1_signature_context_string| now reads "is not permitted and will return an error" to match the actual digest-path guard behavior.
| &read_ctx_len)); | ||
| ASSERT_TRUE(read_ctx); | ||
| ASSERT_EQ(Bytes(ctx_bytes, sizeof(ctx_bytes)), Bytes(read_ctx, read_ctx_len)); | ||
| } |
There was a problem hiding this comment.
Also coverage for a couple more negative cases:
// ---- 9. Context is rejected on the EVP_PKEY_sign/verify digest path ----
// For ML-DSA, the pre-hashed |mu| input already encodes the context per
// FIPS 204 section 5.3, so combining a configured context with the digest
// path must fail rather than be silently ignored.
bssl::UniquePtr<EVP_PKEY_CTX> raw_sign_ctx(
EVP_PKEY_CTX_new(pkey.get(), nullptr));
ASSERT_TRUE(raw_sign_ctx);
ASSERT_TRUE(EVP_PKEY_sign_init(raw_sign_ctx.get()));
ASSERT_TRUE(EVP_PKEY_CTX_set1_signature_context_string(
raw_sign_ctx.get(), ctx_bytes, sizeof(ctx_bytes)));
// |mu| is the 64-byte SHAKE256 output expected by the digest path; the
// context guard fires before any cryptographic check, so the contents
// don't need to be a valid mu for this test.
uint8_t mu[64] = {0};
size_t raw_sig_len = 0;
// Query the signature size (|sig == NULL| short-circuits before the guard).
ASSERT_TRUE(EVP_PKEY_sign(raw_sign_ctx.get(), nullptr, &raw_sig_len, mu,
sizeof(mu)));
std::vector<uint8_t> raw_sig(raw_sig_len);
ASSERT_FALSE(EVP_PKEY_sign(raw_sign_ctx.get(), raw_sig.data(), &raw_sig_len,
mu, sizeof(mu)));
bssl::UniquePtr<EVP_PKEY_CTX> raw_verify_ctx(
EVP_PKEY_CTX_new(pkey.get(), nullptr));
ASSERT_TRUE(raw_verify_ctx);
ASSERT_TRUE(EVP_PKEY_verify_init(raw_verify_ctx.get()));
ASSERT_TRUE(EVP_PKEY_CTX_set1_signature_context_string(
raw_verify_ctx.get(), ctx_bytes, sizeof(ctx_bytes)));
std::vector<uint8_t> dummy_sig(raw_sig_len, 0);
ASSERT_FALSE(EVP_PKEY_verify(raw_verify_ctx.get(), dummy_sig.data(),
dummy_sig.size(), mu, sizeof(mu)));There was a problem hiding this comment.
Added the suggested negative coverage in df71998. The ContextString test now exercises the digest-path guard on both EVP_PKEY_sign and EVP_PKEY_verify (section 9 of the test), so a configured context combined with the pre-hashed mu input fails loudly rather than being silently dropped.
… guard - Update |EVP_PKEY_CTX_set1_signature_context_string| note so the ML-DSA restriction says the context is "not permitted and will return an error" on the EVP_PKEY_sign/verify path instead of "ignored". - Extend the ContextString test with a negative case that exercises the new guard on both EVP_PKEY_sign and EVP_PKEY_verify.
Issues:
Addresses:
Description of changes:
Plumb FIPS 204 context strings through the
EVP_DigestSign/EVP_DigestVerifycode path for ML-DSA. The lower-levelml_dsa_*_sign/verifyfunctions already acceptctx_stringparameters but they were hardcoded to(NULL, 0)in the EVP layer.Call-outs:
PQDSA_PKEY_CTXwith acontext[255]buffer andcontext_lenpkey_pqdsa_ctrlhandlingEVP_PKEY_CTRL_SIGNING_CONTEXTandEVP_PKEY_CTRL_GET_SIGNING_CONTEXT(reusing and renaming the existing genericEVP_PKEY_CTX_set_signature_contextAPI from Ed25519ph toEVP_PKEY_CTX_set1_signature_context_stringto be consistent with bssl)pkey_pqdsa_copyto supportEVP_PKEY_CTX_dupwith context statedctx->context/context_lentopqdsa_sign_messageandpqdsa_verify_messageinstead ofNULL, 0EVP_PKEY_CTX_set_signature_contextvia EVP_DigestSign/Verify instead of manually computing ExternalMuContextStringunit test covering: round-trip sign+verify with context, mismatched context failure, empty-context backward compatibility, >255 byte rejection, and max-length (255 byte) acceptanceAll three ML-DSA variants (44, 65, 87) are covered. Default behavior (empty context string) is unchanged.
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.