OpenSSL compat with ML-DSA - #10962
Conversation
|
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 4 total — 3 posted, 2 skipped
Posted findings
- [High] ML-DSA private key decode in X509 sign/set_pubkey missing PRIVATE_KEY_UNLOCK bracket (fails under FIPS) —
src/x509.c:12827, src/x509.c:16508 - [Medium] WC_MAX_X509_GEN increase puts 20 KB on the stack in wolfSSL_X509_REQ_sign for all key types —
src/x509.c:12953-12962, 16922-16923 - [Medium] wolfSSL_X509_REQ_sign changes lack ML-DSA test coverage —
src/x509.c:16918-16950
Skipped findings
- [Low] fnames[] can become a zero-length array if all ML-DSA levels are disabled
- [Medium] WC_MAX_X509_GEN increase puts 20 KB on the stack in wolfSSL_X509_REQ_sign for all key types
Review generated by Skoll via Claude/Codex
Add ML-DSA test coverage for X509_REQ_sign
PRIVATE_KEY_UNLOCK to X509 sign/set_pubkey
Dynamic buff size based on expected cert size |
|
@kojo1 The CAVP self test failure is legit (undefined references during linking). |
68dd962 to
365a9a0
Compare
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 19 total — 13 posted, 6 skipped
Posted findings
- [High] d2iTryMlDsaKey silently returns a stale EVP_PKEY when the caller reuses an existing key object —
wolfcrypt/src/evp_pk.c:1086-1096 - [Medium] DER buffer sizing ignores the subject public key, only the signing key —
src/x509.c:12961-12974 - [Medium] ML-DSA signing still requires a valid digest; comment claims md is ignored —
src/x509.c:12274-12277 - [Medium] Preprocessor gating mismatch: sigTypeFromPKEY accepts ML-DSA in builds where resign_cert cannot sign —
src/x509.c:12274 - [Medium] MlDsaKey reused across a failed PrivateKeyDecode without Free/Init in X509_set_pubkey —
src/x509.c:16519-16542 - [Medium] d2i_evp_pkey PKCS#8 algorithm cross-check not extended for ML-DSA —
wolfcrypt/src/evp_pk.c:1548-1553 - [Medium] Test coverage limited to ML-DSA-44; the riskiest new paths are untested —
tests/api/test_x509.c:1071-1128 - [Medium] wolfSSL_X509_set_pubkey stores the standard ML-DSA OID for FIPS204-draft keys; the WC_ML_DSA_*_DRAFT cases are unreachable —
src/x509.c:16546-16590 - [Low] New ML-DSA code uses MlDsaKey and mixed heap hints, unlike the rest of x509.c —
src/x509.c:12786 - [Low] WC_MAX_X509_GEN_MLDSA is an unrelated magic number to WC_MAX_X509_GEN —
src/x509.c:12962-12974 - [Low] X509_GEN_BUF_SZ sizes the DER buffer from the signing key, ignoring an ML-DSA subject public key —
src/x509.c:12961-12974 - [Low] New ML-DSA code uses legacy-only LEVEL2/3/5 spellings, breaking WOLFSSL_NO_DILITHIUM_LEGACY_NAMES builds —
src/x509.c:12279-12295 - [Info] ML-DSA key object reused across a failed private-key decode and a public-key decode without re-init —
src/x509.c:16527-16545
Skipped findings
- [Medium] WC_DECLARE_VAR/WC_ALLOC_VAR_EX abstraction dropped in X509_REQ_sign
- [Low] ML-DSA level-to-OID mapping duplicated three times; four-macro guard repeated four times
- [Low] Test guards: zero-length array risk and missing NO_SHA256 condition
- [Info] New test entry breaks the backslash alignment of TEST_X509_DECLS
- [Info] ML-DSA private key DER newly persisted in EVP_PKEY.pkey.ptr, which is freed without ForceZero
- [Info] X509_GEN_BUF_SZ evaluates its argument twice and is never #undef'd
Review generated by Skoll via Claude/Codex
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: REQUEST_CHANGES
Findings: 9 total — 8 posted, 1 skipped
Posted findings
- [Medium] X509_set_pubkey() fails for ML-DSA private keys that carry no seed and no embedded public key —
src/x509.c:16598 - [Medium] Uninitialized MlDsaKey / WC_RNG are freed unconditionally when an earlier Expect short-circuits —
tests/api/test_ossl_x509_pk.c:488 - [Medium] Double XFCLOSE on the same XFILE: the f = XBADFILE reset is missing after the second close —
tests/api/test_evp_pkey.c:2826 - [Medium] New tests still use the legacy-only MlDsaKey typedef, breaking WOLFSSL_NO_DILITHIUM_LEGACY_NAMES builds —
tests/api/test_evp_pkey.c:2846 - [Medium] EVP_PKEY reuse still leaks the previously attached algorithm object, and the new test hides it —
wolfcrypt/src/evp_pk.c:68 - [Low] sigOID is left set to WOLFSSL_FAILURE on the X509/REQ object when the signature-type lookup fails —
src/x509.c:13029 - [Low] Private-key DER is released without zeroization when an EVP_PKEY is reused —
wolfcrypt/src/evp_pk.c:71 - [Low] A non-NULL digest is silently ignored for ML-DSA signing —
src/x509.c:12206
Skipped findings
- [Low] The new reuse block indexes fnames[] unconditionally, which can be a zero-length array
Review generated by Skoll via Claude/Codex
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: COMMENT
Findings: 12 total — 3 posted, 9 skipped
Posted findings
- [Medium] d2i_free_prev_key_obj() does not mirror EVP_PKEY_free() for X25519/X448/HKDF/CMAC, so those objects leak on reuse —
wolfcrypt/src/evp_pk.c:44-107 - [Low] Ed25519/Ed448 d2i reuse leaves the stale type and DER in place - pre-existing, but the prePopulated pattern this PR adds is the fix —
wolfcrypt/src/evp_pk.c:1180 - [Medium] New tests add several multi-KB stack buffers —
tests/api/test_ossl_x509_pk.c:441,470,501,545; tests/api/test_evp_pkey.c:2735-2737,2790-2792
Skipped findings
- [Low] The new ForceZero assumes pkey_sz describes the pkey.ptr allocation - true today, but the invariant is already broken on master (pre-existing, not a blocker)
- [Medium] Unrelated deletion of WOLFSSL_MEM_FAIL_COUNT from .wolfssl_known_macro_extras
- [Info] FALSE POSITIVE - X509_GEN_BUF_SZ / WOLFSSL_MLDSA_X509_SIGN definition block cannot be skipped while a consumer is compiled
- [Medium] No test for the 'sigOID unchanged on rejected key/md' behavior the PR adds
- [Info] ALREADY RESOLVED - non-NULL md silently ignored for ML-DSA
- [Low] X509_GEN_BUF_SZ evaluates its pkey argument twice and drops it entirely in the non-ML-DSA variant
- [Low] Reused EVP_PKEY is left gutted when the d2i copy allocation fails
- [Low] Conditional free of pkey inside the key-file loop is hard to follow
- [Info] Continuation backslash misaligned in TEST_X509_DECLS
Review generated by Skoll via Claude/Codex
4311540 to
7438353
Compare
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 9 total — 4 posted, 5 skipped
Posted findings
- [Low] ML-DSA SPKI buffer size hand-computed instead of using MLDSA_MAX_PUB_KEY_DER_SIZE —
src/x509.c:16657 - [Low] Signature algorithm and signing key level derived from two independent sources without a cross-check —
src/x509.c:12847-12928 - [Info] Doxygen @param inserted between @return lines —
wolfcrypt/src/evp_pk.c:377-382,445-450 - [Info] X509_GEN_BUF_SZ macro evaluates its pkey argument twice —
src/x509.c:13019-13037
Skipped findings
- [Medium] EVP_PKEY reuse repopulates type/object/DER but leaves pkcs8HeaderSz and mldsaOID stale
- [Medium] Non-NULL digest silently ignored for ML-DSA signing
- [Low] Test gap: sigOID-preserved-on-failure and public-only ML-DSA signing key not covered
- [Info] Internal build macro WOLFSSL_MLDSA_X509_SIGN sits in the public WOLFSSL_ namespace and leaks past x509.c
- [Info] Unrelated ML_DSA_LEVEL2k -> ML_DSA_44k rename bundled into this PR
Review generated by Skoll via Claude/Codex
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: REQUEST_CHANGES
Findings: 15 total — 5 posted, 10 skipped
Posted findings
- [High] New typed ML-DSA d2i sets pkcs8HeaderSz, so i2d_PrivateKey() emits a parameter-set-less key that d2i cannot read back —
wolfcrypt/src/evp_pk.c:1696-1704 - [High] MLDSA_MAX_PUB_KEY_DER_SIZE is 2 bytes too small for a FIPS204-draft SPKI at the top compiled level —
src/x509.c:16673-16688 - [Medium] d2i_free_prev_key_obj: duplicates the existing clearEVPPkeyKeys() and leaves scalar metadata stale while repurposing the object —
wolfcrypt/src/evp_pk.c:44-152,172-221 - [Medium] The new pkey.ptr release in d2i_make_pkey: ForceZero trusts pkey_sz, and the free's heap hint and DYNAMIC_TYPE both disagree with the matching allocation —
wolfcrypt/src/evp_pk.c:188-205 - [Medium] PEM read reuse form leaves the caller's EVP_PKEY pointer dangling on failure —
src/pk.c:6592-6608
Skipped findings
- [Medium] A non-NULL md is silently ignored for ML-DSA instead of rejected
- [Medium] Self-fulfilling assertion: the test computes its expected value with the code under test
- [Medium] New d2i behaviour is only half-asserted: 8 of 10 free_prev_key_obj branches, the allowRaw accept path, and the Ed448 half of the prePopulated fix have no test
- [Medium] Test guards exclude the configurations these paths matter most in (PQC-only, 65/87-only), plus two redundant/dead guards
- [Medium] FIPS204-draft handling: the new draft OID cases in both PEM entry points are untested, and the gate is spelled two ways in one file
- [Medium] New error paths in the ML-DSA sign flow are untested, and the REQ path is covered more thinly than the cert path
- [Medium] Signing buffer: the ML-DSA allowance is not derived from the wrapper it must cover, and the switch to a runtime XMALLOC moves 2KB off the stack without comment
- [Low] sigType is computed twice per sign, and the 'only updated on success' comment on sigOID does not hold for the later failure paths
- [Low] Test hygiene in the new blocks: a leak on assertion failure, an over-strict pkey_sz assert the sibling block deliberately relaxed, magic fixture buffer sizes, and a pointless conditional free
- [Info] ML_DSA_LEVEL2k to ML_DSA_44k rename is a no-op alias change that also removes a latent build break
Review generated by Skoll via Claude/Codex
| } | ||
| break; | ||
| #endif /* HAVE_ED448 */ | ||
| #if defined(WOLFSSL_HAVE_MLDSA) |
There was a problem hiding this comment.
🟠 [High] New typed ML-DSA d2i sets pkcs8HeaderSz, so i2d_PrivateKey() emits a parameter-set-less key that d2i cannot read back
🚫 BLOCK bug
d2i_evp_pkey() stores the PKCS#8 header length in local->pkcs8HeaderSz at line 1599 for every type, and the new case WC_EVP_PKEY_DILITHIUM makes ML-DSA reach that line for the first time -- before this PR the switch fell through to default: Unsupported key type and returned NULL, so no ML-DSA EVP_PKEY could ever carry a non-zero pkcs8HeaderSz. Everything downstream is type-agnostic: wolfssl_i_evp_pkey_get_der() (evp_pk.c:2359-2400, verified) returns pkey.ptr + pkcs8HeaderSz for pkey_sz - pkcs8HeaderSz bytes, and it backs wolfSSL_i2d_PrivateKey() (line 2411) and wolfSSL_i2d_PrivateKey_bio() (line 2474). ToTraditionalInline_ex() sets the index to the start of the privateKey OCTET STRING content, so for ML-DSA this strips the AlgorithmIdentifier -- i.e. the parameter set (44/65/87, and the draft-vs-final distinction). The stripped bytes cannot be read back: wolfSSL_d2i_PrivateKey(WC_EVP_PKEY_DILITHIUM, ...) skips the PKCS#8 branch, reaches d2iTryMlDsaKey() with allowRaw=0, and wc_MlDsaKey_PrivateKeyDecode() requires a OneAsymmetricKey structure, so it fails. The i2d/d2i pair the PR adds is asymmetric. It also contradicts an explicit decision already recorded in the same file: wolfSSL_d2i_PKCS8_PKEY() deliberately keeps the whole wrapper for ML-DSA with the comment 'Keep full PKCS#8 wrapper for level recovery from AlgorithmIdentifier parameters' (evp_pk.c:2074) and leaves pkcs8HeaderSz at 0; the auto-detect path never sets it either. So the same ML-DSA key loaded three different ways i2d's back three different ways. pkcs8_encode() (src/pk.c:7383+) has no DILITHIUM branch and returns NOT_COMPILED_IN, confirming the PKCS#8 re-encode path was never taught about ML-DSA.
Suggestion:
| #if defined(WOLFSSL_HAVE_MLDSA) | |
| #ifdef WOLFSSL_HAVE_MLDSA | |
| if (type == WC_EVP_PKEY_DILITHIUM) { | |
| /* ML-DSA has no traditional form - keep the PKCS#8 wrapper so the | |
| * parameter set stays recoverable from the AlgorithmIdentifier, | |
| * matching wolfSSL_d2i_PKCS8_PKEY() and the auto-detect path. */ | |
| local->pkcs8HeaderSz = 0; | |
| } | |
| #endif |
Recommendation: Leave pkcs8HeaderSz at 0 for WC_EVP_PKEY_DILITHIUM (or add an ML-DSA exception in wolfssl_i_evp_pkey_get_der()), and add a regression test: d2i_PrivateKey(WC_EVP_PKEY_DILITHIUM) -> i2d_PrivateKey -> d2i_PrivateKey(WC_EVP_PKEY_DILITHIUM) asserting the round trip succeeds and preserves the level.
| return WOLFSSL_FAILURE; | ||
| } | ||
|
|
||
| derSz = MLDSA_MAX_PUB_KEY_DER_SIZE; |
There was a problem hiding this comment.
🟠 [High] MLDSA_MAX_PUB_KEY_DER_SIZE is 2 bytes too small for a FIPS204-draft SPKI at the top compiled level
🚫 BLOCK bug
The new ML-DSA case sizes the SPKI output buffer with MLDSA_MAX_PUB_KEY_DER_SIZE, which resolves to WC_MLDSA_<max-level>_PUB_KEY_DER_SIZE. Those constants are computed for the final FIPS-204 OIDs only: WC_MLDSA_44_PUB_KEY_DER_SIZE == 1334 == 1312 + 22 (outer SEQUENCE 4 + AlgorithmIdentifier SEQUENCE 2 + OID 11 + BIT STRING 5), confirmed by certs/mldsa/mldsa44_pub-spki.der being exactly 1334 bytes. The FIPS204-draft OIDs in wolfcrypt/src/asn.c:4765-4775 (sigDilithium_Level2Oid = {43,6,1,4,1,2,130,11,12,4,4}) are 11 content bytes versus 9 for sigMlDsa_44Oid, so a draft SPKI needs 24 bytes of overhead -- 2 more than the macro allows. SetAsymKeyDerPublic checks sz > outLen and returns BUFFER_E (asn.c:13412), so this is a clean failure rather than an overflow, but wolfSSL_X509_set_pubkey then returns WOLFSSL_FAILURE for any draft key whose level equals the highest compiled-in level. In the normal --enable-mldsa build (all three levels) that means a draft ML-DSA-87 key can never be installed as a subject public key (2592 + 24 = 2616 > 2614). The PR's own FIPS204-draft test only exercises WC_ML_DSA_44_DRAFT, which fits inside the 87-sized buffer, so CI cannot see it.
Suggestion:
| derSz = MLDSA_MAX_PUB_KEY_DER_SIZE; | |
| /* Ask for the exact size: the FIPS204-draft OID is 2 bytes longer than | |
| * the final one, so MLDSA_MAX_PUB_KEY_DER_SIZE is not an upper bound for | |
| * draft keys at the top compiled-in level. */ | |
| derSz = wc_MlDsaKey_PublicKeyToDer(mldsa, NULL, 0, 1); | |
| if (derSz <= 0) { | |
| wc_MlDsaKey_Free(mldsa); | |
| XFREE(mldsa, cert->heap, DYNAMIC_TYPE_MLDSA); | |
| return WOLFSSL_FAILURE; | |
| } | |
| p = (byte*)XMALLOC((size_t)derSz, cert->heap, DYNAMIC_TYPE_PUBLIC_KEY); |
Recommendation: Query the required length first and allocate exactly that, or add the draft-OID delta when WOLFSSL_MLDSA_FIPS204_DRAFT is enabled. Add a draft case at the highest compiled-in level to test_wolfSSL_X509_set_pubkey. Exact sizing also removes the current over-allocation (2614 bytes for a 1334-byte ML-DSA-44 SPKI).
There was a problem hiding this comment.
Keep full PKCS#8 wrapper for ML-DSA in typed d2i
| * | ||
| * @param [in, out] pkey EVP PKEY being repurposed. | ||
| */ | ||
| static void d2i_free_prev_key_obj(WOLFSSL_EVP_PKEY* pkey) |
There was a problem hiding this comment.
🟡 [Medium] d2i_free_prev_key_obj: duplicates the existing clearEVPPkeyKeys() and leaves scalar metadata stale while repurposing the object
💡 SUGGEST bug
Two aspects of the new helper, both about it being a second implementation of an existing job.
It duplicates clearEVPPkeyKeys() (wolfcrypt/src/evp.c:9202-9235), which already releases owned RSA/DSA/DH/ECC objects and is called from ten places (EVP_PKEY_set1_RSA, set1_EC_KEY, assign_RSA, ...). src/ssl.c includes evp.c at line 407 and evp_pk.c at line 445, so it is already defined ahead of the new helper in the same translation unit. The two differ in a way that will drift: clearEVPPkeyKeys() is type-independent and releases whichever classic objects are owned; the new helper switches on pkey->type, so it depends on that field being accurate.
Scalar metadata is not reset. The per-type object cleanup mirrors wolfSSL_EVP_PKEY_free() faithfully -- every ownership flag, field pointer and DYNAMIC_TYPE matches, pointers are NULLed, no double free is possible. But EVP_PKEY_free() destroys the object while this helper repurposes it, and pkey->type stays stale until d2i_make_pkey() succeeds. On the XMALLOC failure path the caller gets back an object that has been gutted of its key material but still advertises the old type.
Suggestion:
| static void d2i_free_prev_key_obj(WOLFSSL_EVP_PKEY* pkey) | |
| /* in d2i_free_prev_key_obj(), after the switch */ | |
| pkey->pkcs8HeaderSz = 0; | |
| #ifdef WOLFSSL_HAVE_MLDSA | |
| WOLFSSL_ATOMIC_STORE(pkey->mldsaOID, 0); | |
| #endif | |
| /* in d2i_make_pkey(): zero the size when the buffer is released, and | |
| * only assign the real size after the allocation succeeds. */ | |
| pkey->pkey_sz = 0; |
Recommendation: Reuse clearEVPPkeyKeys() for the four classic types and keep the new helper only for what it does not cover (Ed25519/Ed448/X25519/X448/HKDF/CMAC), or state in a comment why a second implementation is needed. Either way, reset pkey->type (and the other scalar metadata) as part of the release so a failed repurpose cannot return a gutted object that still claims its old type.
There was a problem hiding this comment.
Use clearEVPPkeyKeys for d2i EVP_PKEY reuse cleanup
| } | ||
| } | ||
|
|
||
| /* Release key data held from a previous decode when the caller reuses |
There was a problem hiding this comment.
🟡 [Medium] The new pkey.ptr release in d2i_make_pkey: ForceZero trusts pkey_sz, and the free's heap hint and DYNAMIC_TYPE both disagree with the matching allocation
💡 SUGGEST bug
Two problems in the same few lines of new code.
The wipe trusts pkey_sz. ForceZero() writes pkey_sz bytes into pkey.ptr before the free, which is only safe if pkey_sz never exceeds the allocation. Every assignment of pkey_sz and every allocation of pkey.ptr across src/*.c and wolfcrypt/src/*.c was checked; all but one set pkey_sz to the size actually allocated or smaller. The exception is PopulateRSAEvpPkeyDer() (wolfcrypt/src/evp.c:9282-9342).
The free does not match the allocation. The release uses XFREE(pkey->pkey.ptr, pkey->heap, DYNAMIC_TYPE_PUBLIC_KEY) while the allocation five lines below uses XMALLOC(memSz, NULL, priv ? DYNAMIC_TYPE_PRIVATE_KEY : DYNAMIC_TYPE_PUBLIC_KEY) -- a mismatch in both the heap hint and the dynamic type inside a single function. pkey->heap is genuinely non-NULL on real paths: wolfSSL_X509_get_pubkey() builds the key with wolfSSL_EVP_PKEY_new_ex(x509->heap) (src/x509.c:6423).
Suggestion:
| /* Release key data held from a previous decode when the caller reuses | |
| pkey->pkey.ptr = (char*)XMALLOC((size_t)memSz, pkey->heap, | |
| DYNAMIC_TYPE_PUBLIC_KEY); |
Recommendation: Bound the wipe by the allocation rather than by pkey_sz (or fix PopulateRSAEvpPkeyDer() to keep the two in step), and make the free use the same heap hint and DYNAMIC_TYPE as the allocation it pairs with.
There was a problem hiding this comment.
Match alloc/free pairs and keep pkey_sz bounded by allocation
| break; | ||
| #endif | ||
| default: | ||
| type = WOLFSSL_FATAL_ERROR; |
There was a problem hiding this comment.
🟡 [Medium] PEM read reuse form leaves the caller's EVP_PKEY pointer dangling on failure
💡 SUGGEST bug
wolfSSL_d2i_PrivateKey -> d2i_evp_pkey unconditionally does wolfSSL_EVP_PKEY_free(*out); *out = NULL; (wolfcrypt/src/evp_pk.c:1588-1591) before building the replacement. If the subsequent decode fails, d2i_evp_pkey returns NULL, and wolfSSL_PEM_read_bio_PrivateKey sets its local pkey to NULL and skips the *key = pkey write because err is set. The caller's variable is left pointing at freed memory, and the caller's own EVP_PKEY_free() is a double free. The mechanism is pre-existing, but this PR is the first to route ML-DSA keys through wolfSSL_d2i_PrivateKey from the PEM readers, and its new test_wolfSSL_PEM_PrivateKey_mldsa explicitly asserts the 'out-parameter reuse form (documented OpenSSL semantics)' while only covering the success path.
Suggestion:
| type = WOLFSSL_FATAL_ERROR; | |
| pkey = NULL; | |
| /* d2i freed the object the caller passed in; do not leave a | |
| * dangling pointer behind. */ | |
| if (key != NULL) { | |
| *key = NULL; | |
| } | |
| err = 1; |
Recommendation: Write *key = NULL on the failure path (or make d2i_evp_pkey not free *out until it has a replacement), and extend test_wolfSSL_PEM_PrivateKey_mldsa with a negative case: read a valid ML-DSA key into pkey, then a corrupt PEM into the same &pkey, and assert the caller is not left with a freed pointer.
There was a problem hiding this comment.
Do not free caller's EVP_PKEY until d2i has a replacement
3 participants
Description
wolfSSL_PEM_read_bio_PrivateKey
wolfSSL_PEM_read_PrivateKey
wolfSSL_d2i_PrivateKey, wolfSSL_d2i_PublicKey
wolfSSL_X509_sign
wolfSSL_X509_REQ_sign
wolfSSL_X509_set_pubkey
Fixes zd#22151
Testing
Added test_wolfSSL_PEM_PrivateKey_mldsa
Added ML-DSA cases in test_wolfSSL_X509_set_pubkey
Checklist