OpenSSL compat with ML-DSA - #10962
Conversation
|
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
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
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
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
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
| return WOLFSSL_FAILURE; | ||
| } | ||
|
|
||
| derSz = MLDSA_MAX_PUB_KEY_SIZE + MAX_ALGO_SZ + MAX_SEQ_SZ * 2; |
There was a problem hiding this comment.
🔵 [Low] ML-DSA SPKI buffer size hand-computed instead of using MLDSA_MAX_PUB_KEY_DER_SIZE
💡 SUGGEST convention
The new WC_EVP_PKEY_DILITHIUM case sizes the SPKI output buffer as MLDSA_MAX_PUB_KEY_SIZE + MAX_ALGO_SZ + MAX_SEQ_SZ * 2, re-deriving the DER framing overhead by hand. wolfssl/wolfcrypt/wc_mldsa.h already exports exactly this constant: MLDSA_MAX_PUB_KEY_DER_SIZE (2614 for ML-DSA-87, versus 2622 for the hand-rolled expression), and it tracks the compiled-in level set the same way. The current expression is large enough today, but it duplicates knowledge of the encoding in a second place and would silently become undersized if MAX_SEQ_SZ/MAX_ALGO_SZ ever shrank or the SPKI encoding grew. Note the same header is already reachable here (asn.h includes wc_mldsa.h under WOLFSSL_HAVE_MLDSA), and the new #define WC_MAX_X509_GEN_MLDSA a few thousand lines below repeats the same MAX_ALGO_SZ + MAX_SEQ_SZ * 2 idiom.
Suggestion:
| derSz = MLDSA_MAX_PUB_KEY_SIZE + MAX_ALGO_SZ + MAX_SEQ_SZ * 2; | |
| derSz = MLDSA_MAX_PUB_KEY_DER_SIZE; | |
| p = (byte*)XMALLOC(derSz, cert->heap, DYNAMIC_TYPE_PUBLIC_KEY); |
Recommendation: Use MLDSA_MAX_PUB_KEY_DER_SIZE here, and consider using it in WC_MAX_X509_GEN_MLDSA too so the SPKI overhead is defined in exactly one place.
There was a problem hiding this comment.
Use MLDSA_MAX_PUB_KEY_DER_SIZE for SPKI buffers
| #endif | ||
|
|
||
| (void)req; | ||
| WOLFSSL_ENTER("wolfSSL_X509_resign_cert"); |
There was a problem hiding this comment.
🔵 [Low] Signature algorithm and signing key level derived from two independent sources without a cross-check
💡 SUGGEST bug
sigType comes from wolfSSL_sigTypeFromPKEY(), which switches on the cached pkey->mldsaOID. A few lines later type/key come from actually decoding pkey->pkey.ptr and calling mldsa_get_oid_sum() on the decoded key. Nothing checks that the two agree. If they ever diverge — e.g. mldsaOID left over from a previous decode on a reused EVP_PKEY where the OID store was skipped (see the d2i_make_pkey finding) — the emitted certificate would carry an ML-DSA-87 signatureAlgorithm over an ML-DSA-44 signature, i.e. a silently unverifiable certificate rather than an error. The decode already computes oidSum, so the assertion is free.
Suggestion:
| WOLFSSL_ENTER("wolfSSL_X509_resign_cert"); | |
| /* sigType was derived from the cached pkey->mldsaOID; make sure | |
| * the key we are actually signing with matches it. */ | |
| if (oidSum != WOLFSSL_ATOMIC_LOAD(pkey->mldsaOID)) { | |
| WOLFSSL_MSG("ML-DSA key level does not match signature OID"); | |
| wc_MlDsaKey_Free(mldsa); | |
| XFREE(mldsa, x509->heap, DYNAMIC_TYPE_MLDSA); | |
| return WOLFSSL_FATAL_ERROR; | |
| } |
Recommendation: Add an equality check between the decoded oidSum and pkey->mldsaOID (or derive sigType from oidSum directly) so a level mismatch fails loudly instead of producing a malformed certificate.
There was a problem hiding this comment.
Cross-check cached mldsaOID against decoded signing key
| @@ -247,10 +377,12 @@ static int d2iTryEccKey(WOLFSSL_EVP_PKEY** out, const unsigned char* mem, | |||
| * @return 1 on success. | |||
There was a problem hiding this comment.
⚪ [Info] Doxygen @param inserted between @return lines
🔧 NIT style
In both helpers the new @param [in] prePopulated block was inserted in the middle of the @return list, so the generated docs list a parameter after two return values and before a third. The equivalent addition to d2iTryMlDsaKey (line 1125) is correctly placed after the other @param entries, so the three helpers are now inconsistent with each other.
Suggestion:
| * @return 1 on success. | |
| * @param [in] priv 1 means private key, 0 means public key. | |
| * @param [in] prePopulated 1 means *out already holds the input bytes | |
| * so the d2i_make_pkey allocate/copy is skipped. | |
| * @return 1 on success. | |
| * @return 0 when input was recognized as this key type but object | |
| * creation/import failed. | |
| * @return WOLFSSL_FATAL_ERROR when input is not this key type. |
Recommendation: Move the @param prePopulated block above the @return lines in both helpers, matching d2iTryMlDsaKey.
| /* able to override max size until dynamic buffer created */ | ||
| #define WC_MAX_X509_GEN 4096 | ||
| #endif | ||
| #ifdef WOLFSSL_HAVE_MLDSA |
There was a problem hiding this comment.
⚪ [Info] X509_GEN_BUF_SZ macro evaluates its pkey argument twice
🔧 NIT style
X509_GEN_BUF_SZ(x509, pkey) expands pkey twice ((pkey) != NULL && (pkey)->type == ...) and dereferences (x509)->pubKey.length from a macro. Both current call sites pass plain locals so there is no side-effect hazard today, but the pattern is easy to break later, and the two-branch #ifdef duplication of the same expression is avoidable. A static int helper next to wolfssl_x509_make_der() would express the same thing with argument evaluation guarantees and a single body.
Suggestion:
| #ifdef WOLFSSL_HAVE_MLDSA | |
| static int x509_gen_buf_sz(const WOLFSSL_X509* x509, | |
| const WOLFSSL_EVP_PKEY* pkey) | |
| { | |
| int base = WC_MAX_X509_GEN; | |
| #ifdef WOLFSSL_HAVE_MLDSA | |
| if ((pkey != NULL) && (pkey->type == WC_EVP_PKEY_DILITHIUM)) { | |
| base = WC_MAX_X509_GEN_MLDSA; | |
| } | |
| #else | |
| (void)pkey; | |
| #endif | |
| return base + (int)x509->pubKey.length; | |
| } |
Recommendation: Replace the function-like macro with a small static helper; keeps one body for both configurations and removes the double evaluation.
There was a problem hiding this comment.
Replace the macro with a small static helper
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