Fix ML-DSA MakePublicKey derivation, verify guards, and ASN derivation testing

[stenslae]

Description

This update introduces automatic derivation of the ML-DSA public key from the private key during parsing if the public key data is missing.

wolfSSL/wolfssh#1120

Testing

Added test coverage.

Checklist

• added tests
• updated/added doxygen
• updated appropriate READMEs
• Updated manual and documentation

[github-actions]

retest this please

[github-actions]

MemBrowse Memory Report

gcc-arm-cortex-m0plus

• FLASH: .text +24 B (+0.0%, 64,231 B / 262,144 B, total: 25% used)

gcc-arm-cortex-m3

• FLASH: .text +20 B (+0.0%, 122,559 B / 262,144 B, total: 47% used)

gcc-arm-cortex-m4-baremetal

• FLASH: .text +64 B (+0.1%, 66,891 B / 262,144 B, total: 26% used)

gcc-arm-cortex-m4-min-ecc

• FLASH: .text +64 B (+0.1%, 61,805 B / 262,144 B, total: 24% used)

gcc-arm-cortex-m4-pq

• FLASH: .text +448 B (+0.2%, 280,448 B / 1,048,576 B, total: 27% used)

gcc-arm-cortex-m4-sp-math

• FLASH: .text +64 B (+0.1%, 61,805 B / 262,144 B, total: 24% used)

gcc-arm-cortex-m4-tls12

• FLASH: .text +64 B (+0.1%, 123,355 B / 262,144 B, total: 47% used)

gcc-arm-cortex-m7-pq

• FLASH: .text +448 B (+0.2%, 281,024 B / 1,048,576 B, total: 27% used)

linuxkm-pie

• Data: __patchable_function_entries +8 B (+0.0%, 25,968 B)

linuxkm-standard

• Data: __patchable_function_entries +16 B (+0.0%, 48,632 B)
  No memory changes detected for:
• gcc-arm-cortex-m4
• gcc-arm-cortex-m4-crypto-only
• gcc-arm-cortex-m4-dtls13
• gcc-arm-cortex-m4-openssl-compat
• gcc-arm-cortex-m4-pkcs7
• gcc-arm-cortex-m4-rsa-only
• gcc-arm-cortex-m4-tls13
• gcc-arm-cortex-m7
• gcc-arm-cortex-m7-tls13
• stm32-sim-stm32h753

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #10985

Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

[wolfSSL-Fenrir-bot]

🟠 [Medium] Memory leak in wc_MlDsaKey_MakePublicKey when WC_MLDSA_CACHE_PRIV_VECTORS is defined · Resource leaks on error paths

The cleanup block that calls XFREE(s1, ...) is gated #ifndef WC_MLDSA_CACHE_PRIV_VECTORS, but when that macro is defined without WC_MLDSA_FIXED_ARRAY, XMALLOC is still called into local s1 (didAlloc=1): in non-small-mem mode line 10399 adds aSz to allocSz; in small-mem mode lines 10403-10406 always include s1/s2/t sizes regardless of the macro. The allocation is never freed.

Fix: Add a matching if (didAlloc) { XFREE(s1, key->heap, DYNAMIC_TYPE_MLDSA); } inside a #ifdef WC_MLDSA_CACHE_PRIV_VECTORS block, or restructure so didAlloc always drives a single free regardless of the macro.

[Frauschi]

🐺 Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 6 total — 4 posted, 2 skipped

Posted findings

• [High] wc_MlDsaKey_MakePublicKey mishandles buffers in the small-mem path (aSet, h, s1) — wolfcrypt/src/wc_mldsa.c:10413-10665
• [Medium] wc_EccPrivateKeyDecode now unconditionally derives the public point — wolfcrypt/src/asn.c:32722-32745
• [Low] Dead/empty guard branch before t0Scratch allocation — wolfcrypt/src/wc_mldsa.c:10458-10467
• [Medium] Duplicated t = A o NTT(s1) + s2 kernel - extract shared helper with keygen — wolfcrypt/src/wc_mldsa.c:10479-10635

Skipped findings

• [Medium] No test coverage for small-mem / cache-macro configurations of MakePublicKey
• [Medium] ML-DSA secret vectors left unzeroed in wc_MlDsaKey_MakePublicKey under SMALL_MEM + CACHE_PRIV_VECTORS

Review generated by Skoll via Claude/Codex

[Frauschi]

🟠 [High] wc_MlDsaKey_MakePublicKey mishandles buffers in the small-mem path (aSet, h, s1)
🚫 BLOCK bug

The new function's WOLFSSL_MLDSA_MAKE_KEY_SMALL_MEM path diverges from the reference keygen (mldsa_make_key_from_seed, which uses a cleanly separated small-mem implementation that always allocates locally and never touches the caches). Three concrete defects appear when small-mem is combined with the cache / fixed-array knobs (no #error prevents these combinations):

1. key->aSet = 1 set without populating key->a. The #ifdef WC_MLDSA_CACHE_MATRIX_A / key->aSet = 1; block (line 10637-10641) sits outside the #ifndef/#else WOLFSSL_MLDSA_MAKE_KEY_SMALL_MEM split, so it runs for both paths. In the small-mem path a is reassigned to a local single-poly streaming buffer (line 10448), so key->a is never filled — and it was XMALLOC'd un-zeroed at line 10352 (unlike the XMEMSET at line 12099). Reference keygen small-mem (lines 8194-8198) deliberately does not set aSet. Because sign/verify skip re-expanding A when aSet is already 1 (lines 8474, 9733, 11817), a later wc_MlDsaKey_Sign would use the uninitialized matrix and produce a silently invalid signature.

2. NULL h scratch under WC_MLDSA_FIXED_ARRAY. The FIXED_ARRAY branch (10414-10421) sets s1/s2/t/a from the key struct but never assigns h; h is only initialized in the non-fixed #else small-mem branch (line 10448). It is then passed NULL to mldsa_rej_ntt_poly_ex(&key->shake, aseed, a, h).

3. Leaked + un-zeroized secret with WC_MLDSA_CACHE_PRIV_VECTORS. The small-mem allocSz (line 10403) is not reduced for cached private vectors, so s1 = XMALLOC(allocSz) still runs (didAlloc=1), overwriting the cached pointer. The ForceZero+XFREE cleanup lives under #ifndef WC_MLDSA_CACHE_PRIV_VECTORS (10645-10660), so that allocation is neither zeroized (it holds the decoded secret vector s1) nor freed. Reference keygen small-mem frees unconditionally (lines 8206-8209).

Suggestion: Move the key->aSet = 1; assignment inside the non-small-mem branch only (mirror keygen, which leaves aSet unset in small-mem). Initialize/allocate h (and free s1/ForceZero) unconditionally on the small-mem path regardless of FIXED_ARRAY / CACHE_PRIV_VECTORS, or reuse keygen's cleanly-separated small-mem implementation. Alternatively add #error guards documenting that small-mem is incompatible with these cache macros.

[Frauschi]

🟡 [Medium] wc_EccPrivateKeyDecode now unconditionally derives the public point
💡 SUGGEST api

The decode of a SEC1 private-only key now performs a scalar point-multiplication on every call, changing key->type from ECC_PRIVATEKEY_ONLY to ECC_PRIVATEKEY and adding a per-decode cost. This is the stated intent of the PR and errors are swallowed (best-effort, so no functional regression), but it is a behavioral/performance change to a widely-called public API (the identical change is duplicated in asn_orig.c:7707-7731). Any caller that asserted the key remained private-only after decode, or perf-sensitive bulk-decode paths, are affected. Under ECC_TIMING_RESISTANT it uses key->rng, which wc_ecc_import_private_key_ex does not set — so on timing-resistant builds the derivation silently runs unblinded / may no-op, which is acceptable as best-effort but worth documenting.

Suggestion: Document the behavior change in the ChangeLog/API docs; consider whether the key->rng reference on timing-resistant builds is meaningful given import does not populate it.

[Frauschi]

🔵 [Low] Dead/empty guard branch before t0Scratch allocation
🔧 NIT style

The construct if ((ret == 0) && (t0Scratch != NULL || t0ScratchSz == 0)) { /* empty */ } else if (ret == 0) { alloc } is confusing. At this point t0Scratch is always NULL and t0ScratchSz is always non-zero (k * 416), so the first branch is dead code and the comment ("shouldn't happen, but guard") describes an impossible state.

Suggestion:

Suggested change

	if ((ret == 0) && (t0Scratch != NULL || t0ScratchSz == 0)) {
	if (ret == 0) {
	t0Scratch = (byte*)XMALLOC(t0ScratchSz, key->heap, DYNAMIC_TYPE_MLDSA);
	if (t0Scratch == NULL) {
	ret = MEMORY_E;
	}
	}

[Frauschi]

🟡 [Medium] Duplicated t = A o NTT(s1) + s2 kernel - extract shared helper with keygen
💡 SUGGEST maintainability

wc_MlDsaKey_MakePublicKey re-implements the same t = A o NTT(s1) + s2 -> decompose-into-t0/t1 math that already exists in the reference keygen mldsa_make_key_from_seed. It is a fresh copy, not a shared helper:

1. Standard path (wc_mldsa.c:10486-10494) duplicates keygen wc_mldsa.c:7927-7940 - identical apart from the destination variable name (t0 vs t0Scratch).
2. Small-mem streaming multiply (wc_mldsa.c:10502-10631, ~130 lines) duplicates keygen wc_mldsa.c:8075-8189 byte-for-byte: the whole 8x-unrolled inner multiply across all four POLY64 x SMALL permutations, plus mldsa_invntt_full / mldsa_add / mldsa_make_pos. A grep confirms this streaming multiply ((sword64)a[e] * s1t[e]) exists in exactly these two functions and nowhere else.

Only the setup and teardown legitimately differ: keygen sources s1/s2 from mldsa_expand_s(seed), then encodes them into the private key and hashes tr; MakePublicKey instead decodes s1/s2 from the already-stored private key (mldsa_vec_decode_eta_bits) and only sets pubKeySet. The compute kernel between those two ends is identical.

This duplication is the root cause of the BLOCK finding (Finding 1) and the secret-leak finding (Finding 4): keygen's small-mem path allocates locally, never sets aSet, and always ForceZero+frees, whereas the copy drifted while bolting the cache/fixed-array knobs onto the same body. Extracting the kernel once means those defects are fixed - and audited - in a single place.

Proposed shared helper (declaration only - body is a mechanical lift of the keygen kernel):

/* Compute t = A o NTT(s1) + s2 and encode it into t0/t1.
 * Shared kernel for private->public derivation. Caller supplies the already
 * resolved scratch buffers (malloc'd, cached, or fixed-array) so the helper is
 * agnostic to the WC_MLDSA_CACHE_* / WC_MLDSA_FIXED_ARRAY layout; pass NULL for
 * h / t64 in configs where they are unused.
 *
 * key    [in]   ML-DSA key (provides shake and params).
 * rho    [in]   Public seed used to expand (standard) or stream (small-mem) A.
 * s1     [in]   Secret vector s1 in small form; NTT'd in place by the helper.
 * s2     [in]   Secret vector s2.
 * t      [out]  Scratch for the k-polynomial result vector t.
 * a      [in]   Matrix A: full matrix (standard) or one poly (small-mem).
 * h      [in]   Rejection-sampling scratch (small-mem only, else NULL).
 * t64    [in]   64-bit accumulator (WOLFSSL_MLDSA_SMALL_MEM_POLY64 only, else NULL).
 * t0Dst  [out]  Encoded t0 destination.
 * t1Dst  [out]  Encoded t1 destination.
 * returns 0 on success, negative on error.
 */
static int mldsa_encode_t0_t1_from_s(wc_MlDsaKey* key, const byte* rho,
    sword32* s1, sword32* s2, sword32* t, sword32* a, byte* h,
    sword64* t64, byte* t0Dst, byte* t1Dst);

Note the prototype is config-independent: h and t64 are always valid types, so both callers can share one signature and pass NULL for the buffers a given build does not use.

Suggestion: Extract the shared compute kernel (NTT of s1, A o NTT(s1) + s2 for both the standard and small-mem paths, make_pos, and mldsa_vec_encode_t0_t1) into the internal helper declared above. Keep allocation policy in each caller since it genuinely differs, and have the helper take the resolved buffer pointers so it stays independent of the cache/fixed-array layout.