PKCS#7: add ML-DSA support#10759
Conversation
|
retest this please |
|
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10759
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
Frauschi
force-pushed
the
pkcs7_mldsa
branch
2 times, most recently
from
443710e to
aaaf1b3
Compare
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-securityOverall recommendation: REQUEST_CHANGES
Findings: 5 total — 5 posted, 0 skipped
5 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [High] [review] ML-DSA verify references wc_PKCS7_CertMatchesSignerInfo which is not compiled in RSA-less/ECC-less builds —
wolfcrypt/src/pkcs7.c:5396 - [Low] [review+review-security] RSA raw-sign callback output buffer tightened to modulus size with no post-write bounds check —
wolfcrypt/src/pkcs7.c:2973-2978 - [Low] [review] InitWithCert no longer size-checks non-RSA/ECC public keys —
wolfcrypt/src/pkcs7.c:1315-1340 - [Info] [review-security] wc_OidGetHash SHAKE128/SHAKE256 mapping affects OCSP/CRL/PKCS12/RSA-PSS beyond the stated PKCS#7 scope —
wolfcrypt/src/hash.c:356-369 - [Info] [review] ML-DSA verify masks hard internal errors from key decode as SIG_VERIFY_E —
wolfcrypt/src/pkcs7.c:5416-5449
Review generated by Skoll
Frauschi
force-pushed
the
pkcs7_mldsa
branch
2 times, most recently
from
edab80f to
ab350ec
Compare
|
Jenkins retest this please |
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-securityOverall recommendation: COMMENT
Findings: 3 total — 3 posted, 0 skipped
1 finding(s) posted as inline comments (see file-level comments below)
2 finding(s) not tied to a diff line (full detail below)
Posted findings
- [Medium] [review] scratch set-but-unused in wc_PKCS7_CheckPublicKeyDer for ML-DSA-sign-only + no-RSA/no-ECC build —
wolfcrypt/src/pkcs7.c:1094
Findings not tied to a diff line
Redundant content digest computed on ML-DSA verify path
File: wolfcrypt/src/pkcs7.c:5799
Function: wc_PKCS7_SignedDataVerifySignature
Severity: Low
For the ML-DSA case, wc_PKCS7_BuildSignedDataDigest (line 5799) still computes pkcs7Digest/plainDigest, but the ML-DSA branch ignores them and rebuilds the pure-mode message via wc_PKCS7_BuildPureSigMessage. This is harmless (and the digest is genuinely needed for the earlier wc_PKCS7_VerifyContentMessageDigest when signed attributes are present), so it is not a correctness issue - just a small amount of unused work for the no-signed-attributes ML-DSA case. Noting only for awareness; no change required.
Recommendation: Optional: no action needed. Left as-is keeps the shared verify flow simple and preserves the messageDigest binding check.
Referenced code: wolfcrypt/src/pkcs7.c:5799-5803 (5 lines)
Detached ML-DSA SignedData over empty content is rejected (functional gap, not a security defect)
File: wolfcrypt/src/pkcs7.c:4305-4310, 3447-3453
Function: wc_PKCS7_EncodeSignedData / PKCS7_EncodeSigned
Severity: Info
Category: Logic
wc_PKCS7_EncodeSignedData pre-computes a content hash and calls PKCS7_EncodeSigned with a non-NULL hashBuf for three cases, including (pkcs7->detached && pkcs7->contentSz == 0) (the SCEP empty-content case). The PR adds a guard in PKCS7_EncodeSigned (line 3447) that rejects any ML-DSA signer when hashBuf != NULL with BAD_FUNC_ARG. As a result, a detached ML-DSA SignedData over empty content (pkcs7->detached set, contentSz == 0) is unconditionally rejected, whereas RSA/ECDSA support it. This is a new-feature limitation (ML-DSA was previously unsupported in PKCS#7, so there is no regression) and is a clean, safe error return rather than a memory-safety issue. Detached/non-empty and embedded ML-DSA both work correctly. Worth documenting the limitation, but it carries no security impact.
Recommendation: No security action required. Optionally document that detached-over-empty-content SignedData is unsupported for ML-DSA, or special-case the detached && contentSz==0 path for ML-DSA so the pure-mode signature is computed over the (empty) eContent instead of taking the pre-hash branch.
Referenced code: wolfcrypt/src/pkcs7.c:4305-4310, 3447-3453 (13 lines)
Review generated by Skoll
|
Jenkins retest this please |
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-securityOverall recommendation: COMMENT
Findings: 6 total — 3 posted, 3 skipped
2 finding(s) posted as inline comments (see file-level comments below)
1 finding(s) not tied to a diff line (full detail below)
Posted findings
- [Low] [review] Unused DigestInfo buffer allocated for ML-DSA signing —
wolfcrypt/src/pkcs7.c:3015-3033 - [Low] [review] Content digest recomputed then discarded on ML-DSA verify —
wolfcrypt/src/pkcs7.c:5945-6071
Findings not tied to a diff line
ML-DSA 'no certificates in bundle' verify path stores an unusable digest
File: wolfcrypt/src/pkcs7.c:5954-6015
Function: wc_PKCS7_SignedDataVerifySignature
Severity: Medium
Category: correctness
When a SignedData bundle contains no certificates, the verifier stores pkcs7->signature, pkcs7->plainDigest, and pkcs7->pkcs7Digest and returns PKCS7_SIGNEEDS_CHECK so the caller can verify externally with their own key. This design assumes the signature is over a digest. ML-DSA is used in 'pure' mode (RFC 9882) and signs the complete message (the re-tagged SET OF signed attributes, or the eContent), not a digest. For an ML-DSA signer with no cert in the bundle, the stored plainDigest/pkcs7Digest is insufficient to verify the signature -- the caller has no way to reconstruct the exact octet string that was signed. This runs before the switch (pkcs7->publicKeyOID) dispatch, so it applies to ML-DSA too. It fails closed (no false-accept), but silently hands back data that cannot complete an ML-DSA verification.
Recommendation: For ML_DSA_44k/65k/87k, either return a clear 'unsupported' error in the no-cert case, or store the reconstructed pure-mode message (from wc_PKCS7_BuildPureSigMessage) instead of a digest so an external verifier can use it. Document the limitation or special-case ML-DSA in the no-cert path so callers do not receive verification material they cannot use.
Referenced code: wolfcrypt/src/pkcs7.c:5954-5958 (5 lines)
Skipped findings
- [Medium]
No test coverage for ML-DSA no-certificate external-verify path - [Low]
configure.ac PKCS7 check does not account for WOLFSSL_MLDSA_NO_ASN1 - [Info]
wc_OidGetHash() SHAKE OID mapping widens accepted digests beyond PKCS#7 (blast radius)
Review generated by Skoll
Add ML-DSA signing and verification for CMS/PKCS#7 SignedData, following RFC 9882. ML-DSA is used in CMS "pure" mode: the signature is computed over the complete message (the DER SET OF signed attributes, or the eContent when none are present) with an empty context string and absent signatureAlgorithm parameters, rather than over a pre-computed DigestInfo as with RSA/ECDSA. wolfcrypt/src/pkcs7.c: - New ML-DSA helpers: wc_PKCS7_MlDsaLevelFromOID, wc_PKCS7_BuildPureSigMessage, wc_PKCS7_MlDsaSign and wc_PKCS7_MlDsaVerify, wired into the per-algorithm switch sites (GetSignSize, SignedDataGetEncAlgoId, SetPublicKeyOID, CheckPublicKeyDer) and the sign/verify dispatchers. Only the final FIPS 204 ML-DSA OIDs are accepted; pre-standard draft Dilithium OIDs are not. - GetSignSize derives the ML-DSA signature length from the parameter set. - InitWithCert copies the signer public key into the RSA-sized publicKey buffer only for RSA/ECC certs (the raw-sign callback consumers); large PQC keys such as ML-DSA would overflow it and are never read back, so publicKeySz stays 0. - wc_MlDsaKey is always heap allocated (it embeds multi-KB key buffers); the accompanying DecodedCert uses the WC_DECLARE_VAR/WC_ALLOC_VAR_EX macros for stack-vs-heap handling under WOLFSSL_SMALL_STACK. - wc_PKCS7_SignedDataBuildSignature skips building the DigestInfo for ML-DSA, which signs the full message in pure mode and never consumes it. - wc_PKCS7_MlDsaSign wraps the ML-DSA private-key decode in PRIVATE_KEY_UNLOCK/PRIVATE_KEY_LOCK. Unlike RSA/ECC, the FIPS module gates wc_MlDsaKey_PrivateKeyDecode behind the private-key read lock, so signing would otherwise fail with FIPS_PRIVATE_KEY_LOCKED_E under --enable-fips. The macros are no-ops in non-FIPS builds. wolfssl/wolfcrypt/pkcs7.h: - Document that the fixed-size signer public key buffer (publicKey/publicKeySz) holds only RSA/ECC keys; it stays RSA-sized. wolfcrypt/src/hash.c: - Map the SHAKE128/SHAKE256 OIDs to their hash types in wc_OidGetHash(). certs/mldsa: - Add expanded-only PKCS#8 DER private keys (mldsa44/65/87-key.der) matching the self-signed ML-DSA certificates, with README and include.am updates. The expanded-only shape (no seed) decodes via wc_MlDsaKey_ImportPrivRaw without keygen-from-seed or the ASN template, so pkcs7signed_mldsa_test also passes in WOLFSSL_MLDSA_NO_MAKE_KEY and non-WOLFSSL_ASN_TEMPLATE builds. certs/renewcerts.sh: - Generate the mldsa<N>-key.der files from the matching mldsa<N>-key.pem in the expanded-only shape (openssl pkey -provparam ml-dsa.output_formats=priv), so a regeneration keeps the DER key in step with the cert. The OpenSSL detection probe now requires both ML-DSA keygen and that conversion across all three levels, so the block runs fully (matched cert+key) or is skipped entirely rather than aborting mid-way. wolfcrypt/test/test.c: - Add pkcs7signed_mldsa_test(): round-trip encode/verify of SignedData across ML-DSA-44/65/87, with and without signed attributes, including a check that the digest algorithm parameters are encoded as expected. The message-digest OID is selected from the enabled hash set (SHA-512, else SHA-256, else SHA-1) so the test builds when SHA-512 is disabled. A negative case confirms ML-DSA rejects a caller-supplied pre-computed content hash with BAD_FUNC_ARG.
|
I fixed the latest "ML-DSA 'no certificates in bundle' verify path" issue. |
4 participants
Summary
Add ML-DSA as a signature algorithm for PKCS#7/CMS
SignedData, for both signing and verification, following RFC 9882 ("Use of ML-DSA in the Cryptographic Message Syntax").Unlike RSA/ECDSA, ML-DSA is used in CMS "pure" mode: the signature is computed over the complete message - the
DER SET OFsigned attributes, or theeContentdirectly when no signed attributes are present - rather than over a pre-computedDigestInfo, with an emptycontextstring and absentsignatureAlgorithmparameters.Changes
wolfcrypt/src/pkcs7.c- core implementationwc_PKCS7_MlDsaLevelFromOID,wc_PKCS7_BuildPureSigMessage,wc_PKCS7_MlDsaSign,wc_PKCS7_MlDsaVerify, wired into the existing per-algorithm switch sites (GetSignSize,SignedDataGetEncAlgoId,SetPublicKeyOID,CheckPublicKeyDer) and the sign/verify dispatchers.wc_MlDsaKeyis always heap-allocated (due to its size), matching the ML-DSA handling inasn.c; the accompanyingDecodedCertuses theWC_DECLARE_VAR/WC_ALLOC_VAR_EXmacros forWOLFSSL_SMALL_STACKhandling.wc_PKCS7only for RSA/ECC (consumed by the raw-sign callback paths), with the buffer-size bound retained; large PQC keys are not stored.wolfcrypt/src/hash.c- map theSHAKE128/SHAKE256OIDs to their hash types inwc_OidGetHash()(RFC 8702 digest algorithms).certs/mldsa/- add expanded-only PKCS#8 DER private keys (mldsa44/65/87-key.der) matching the existing self-signed ML-DSA certificates.wolfcrypt/test/test.c-pkcs7signed_mldsa_test(): round-trip encode→verify across every enabled ML-DSA level with SHA-512 and (where available) SHAKE digests, asserting:SIG_VERIFY_E(negative path).Configuration
Gated behind existing ML-DSA macros - no new public config option. Active when
WOLFSSL_HAVE_MLDSAis enabled with ASN.1 support; signing and verification compile independently based on the ML-DSA backend's private-/public-key availability (WOLFSSL_MLDSA_NO_SIGN/WOLFSSL_MLDSA_NO_VERIFY). No change to default builds.Compatibility
Testing
./configure --enable-all --enable-debug && make && ./wolfcrypt/test/testwolfcrypt-PKCS7signed test passed!, full suite green.NO_SIGN,NO_VERIFY, both, andNO_MAKE_KEY+NO_SIGN+NO_VERIFY) with the project's-Werrorflag set.