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 #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.
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
🐺 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
b6324ad to
b72a8ed
Compare
106f551 to
fa80057
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10985
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
fa80057 to
e292a3c
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10985
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
e292a3c to
ceb0a7d
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
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.
ceb0a7d to
3d9668e
Compare
3d9668e to
2527a37
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
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.
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: REQUEST_CHANGES
Findings: 18 total — 8 posted, 10 skipped
Posted findings
- [High] Three undisclosed sword32-overflow fixes in ML-DSA small-mem arithmetic ship with no executing test —
wolfcrypt/src/wc_mldsa.c:8773 - [Medium] Under WOLF_CRYPTO_CB_FIND the devId gate in EccDerivePubBestEffort is still ineffective, so every SEC1 private-key decode fires an unsolicited EccMakePub callback —
wolfcrypt/src/asn.c:33196 - [Medium] Two sign-only ECC decode sites (pkcs7.c, ocsp.c) were not converted to derivePub=0 —
wolfcrypt/src/pkcs7.c:2150 - [Low] WC_MLDSA_HAVE_CHECK_KEY's second arm is unreachable and the condition is duplicated in two spellings —
wolfcrypt/src/wc_mldsa.c:262 - [Low] Doxygen for PublicKeyToDer/KeyToDer states the derivation is unconditional, but the code gates it on output != NULL —
doc/dox_comments/header_files/wc_mldsa.h:909 - [Low] placeholder byte relies on an undocumented internal contract of SetAsymKeyDer —
wolfcrypt/src/wc_mldsa.c:14463 - [Low] New key->ku scrub in ecc_make_pub_ex() is skipped on async WC_PENDING_E and never re-run —
wolfcrypt/src/ecc.c:5778-5791 - [Info] Redundant guard terms in tests/suites.c and the new ML-DSA test guards —
tests/suites.c:1330
Skipped findings
- [High] [BOT-DUPLICATE] Async derive failure leaves the key ECC_PRIVATEKEY with no public point, and the derivation runs unblinded
- [Medium] ML-DSA export functions now mutate a caller-owned key from inside wc_MakeCert/wc_SignCert
- [Medium] [BOT-DUPLICATE] mldsa_oom_derive_fail_level asserts an allocation count that is zero under WC_MLDSA_CACHE_PRIV_VECTORS
- [Medium] Coverage gaps around the new derivation opt-outs and error paths
- [Low] [BOT-DUPLICATE] Test comment claims a temporary RNG is used for blinding
- [Low] [BOT-DUPLICATE] ECC OOM sub-test can still pass vacuously, and carries a dead guard condition
- [Low] 23 added lines exceed the 80-column limit
- [Low] [BOT-DUPLICATE] internal.c justification comment is wrong (also static ECDH, and WOLFSSL_CHECK_SIG_FAULTS)
- [Info] PUBLIC_KEY_E replaces BAD_FUNC_ARG on four public verify entry points - needs a ChangeLog note
- [Info] MakePublicKey encode scratch holding encoded t0 is not registered with wc_MemZero_Add
Review generated by Skoll via Claude/Codex
| #ifdef WOLFSSL_MLDSA_SMALL | ||
| /* Reduce before invntt to avoid sword32 overflow, as in | ||
| * mldsa_calc_t_std()'s vec_red() call. */ | ||
| mldsa_poly_red(tt); |
There was a problem hiding this comment.
🟠 [High] Three undisclosed sword32-overflow fixes in ML-DSA small-mem arithmetic ship with no executing test
🚫 BLOCK test
Beyond the stated scope of the PR, the diff adds three new mldsa_poly_red() calls under WOLFSSL_MLDSA_SMALL, immediately before mldsa_invntt_full(): in mldsa_calc_t_small_mem (wc_mldsa.c:8773), in mldsa_sign_with_seed_mu (wc_mldsa.c:9926), and in the small-mem mldsa_verify_with_mu (wc_mldsa.c:11020). Each mirrors an existing mldsa_vec_red() in the corresponding standard path (wc_mldsa.c:8630, 9763, 10771) that the small-mem paths were missing. These are real latent correctness fixes: without the reduce, accumulated products can overflow sword32 before the inverse NTT, producing wrong keys and wrong signatures in WOLFSSL_MLDSA_SMALL + small-mem builds. The supporting guard widening at wc_mldsa.c:6298 and wc_mldsa.c:8289 (dropping !defined(WOLFSSL_MLDSA_VERIFY_SMALL_MEM)) exists solely to make mldsa_red/mldsa_poly_red compile for the new verify call site, confirming the additions are deliberate. Neither the PR description nor any of the four commit messages mentions them, no test fails without them, and no CI job would notice: --enable-mldsa=yes,small appears in exactly one entry (noasm-mldsa-align0, .github/workflows/wolfCrypt-Wconversion.yml:62) which sets no *_SMALL_MEM macro, and every ML-DSA small-mem entry (smallstack-noasm-small-mem, smallstack-intelasm-precalc, precalc-a-no-int128, verify-only-m32) is "check": false with --disable-crypttests, which .github/scripts/parallel-make-check.py:285 documents as skipping the make-check phase entirely. So WOLFSSL_MLDSA_SMALL combined with any small-mem variant is never built, and the small-mem paths that are built are never executed - this is exactly the code the PR rewrites wholesale into mldsa_calc_t_small_mem and then adds a second consumer for in wc_MlDsaKey_MakePublicKey.
Suggestion: Split the three mldsa_poly_red() additions into their own commit describing the overflow they fix, and add a CI entry that both builds and runs the combination:
{"name": "mldsa-small-small-mem", "minutes": 1,
"configure": ["--enable-cryptonly", "--enable-all-crypto", "--enable-mldsa=yes,small",
"CPPFLAGS=-DWOLFSSL_MLDSA_SIGN_SMALL_MEM -DWOLFSSL_MLDSA_VERIFY_SMALL_MEM -DWOLFSSL_MLDSA_MAKE_KEY_SMALL_MEM"],
"check": true},
Recommendation: Separate the arithmetic fixes from the refactor so they are individually reviewable, and add at least one CI configuration combining WOLFSSL_MLDSA_SMALL with the small-mem macros that actually runs make check (the KATs in test_mldsa.c would catch a wrong t/w). Without a running config, both these fixes and the entire mldsa_calc_t_small_mem rewrite land unexercised.
There was a problem hiding this comment.
Did not reproduce any overflows, labeling this as a defensive change that follows convention. Added suggested CI config
| if (key->type != ECC_PRIVATEKEY_ONLY) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 [Medium] Under WOLF_CRYPTO_CB_FIND the devId gate in EccDerivePubBestEffort is still ineffective, so every SEC1 private-key decode fires an unsolicited EccMakePub callback
💡 SUGGEST bug
Sites: wolfcrypt/src/asn.c:33196; wolfcrypt/src/asn.c:33197-33202
The helper skips derivation only when key->devId != INVALID_DEVID, which is exactly backwards for WOLF_CRYPTO_CB_FIND builds: there a find callback owns keys that carry INVALID_DEVID, so those keys fall straight through to wc_ecc_make_pub_ex, and ecc_make_pub_ex deliberately drops the same devId gate under FIND (ecc.c:5722-5726) to issue wc_CryptoCb_EccMakePub(key, pub). The in-tree comment justifying that FIND behaviour (ecc.c:5706-5720) reasons explicitly about the signing path - "Reaching the software signer means the device already declined to sign (the whole-sign offload wc_CryptoCb_EccSign ran first)" - and concludes a find callback would likewise decline. That precondition does not exist during SEC1 key parsing, which is a brand-new call context this PR introduces for that callback. A find callback that answers EccMakePub from a resident device key rather than the supplied key->k therefore yields the wrong public point on decode, silently and with a 0 return. Separately, every other device-first branch in ecc.c wraps the devId test in #ifndef WOLF_CRYPTO_CB_FIND (ecc.c:4775, 5721, 5916, 7035, 9150, 10622, 11232, 11822), so the helper also breaks the file's own convention. The identical omission is repeated in the new ML-DSA entry point at wolfcrypt/src/wc_mldsa.c:11453, whose comment explicitly claims key->k may be a device handle - precisely the FIND case. This is the third review round in which the WOLF_CRYPTO_CB_FIND half of this issue has been raised; only the WOLF_CRYPTO_CB_ONLY_ECC exclusion landed.
Also, at wolfcrypt/src/asn.c:33197-33202 (Low, bugs scan) — devId early-return in EccDerivePubBestEffort does not cover WOLF_CRYPTO_CB_FIND:
The helper skips derivation for device-tagged keys by testing key->devId != INVALID_DEVID. But ecc_make_pub_ex() drops exactly that gate when WOLF_CRYPTO_CB_FIND is defined (ecc.c:5721-5725): #ifndef WOLF_CRYPTO_CB_FIND / if ((err == MP_OKAY) && (key->devId != INVALID_DEVID)) / #else / if (err == MP_OKAY) / #endif. So in a WOLF_CRYPTO_CB_FIND build, the keys the helper deliberately lets through (devId == INVALID_DEVID) are precisely the ones that then reach wc_CryptoCb_EccMakePub() anyway. This PR therefore introduces a crypto-callback invocation on every private-only SEC1 key decode in such builds — a call that did not previously happen at decode time at all. The hazard is already documented in the existing comment at ecc.c:5705-5720 ("if the device ignores key->k ... it returns dG instead of the requested kG"); this change widens that exposure from the ECDSA sign path to ordinary key loading. This point was raised in an earlier review round and the exclusion was applied to the test guard in test_asn.c but not to the library guard.
Suggestion:
| #if defined(PLUTON_CRYPTO_ECC) || defined(WOLF_CRYPTO_CB) | |
| /* Don't derive host-side public points for device-backed keys. Under | |
| * WOLF_CRYPTO_CB_FIND any key may be device-backed - and ecc_make_pub_ex() | |
| * drops its own devId gate there - so skip the derivation entirely. */ | |
| #ifndef WOLF_CRYPTO_CB_FIND | |
| if (key->devId != INVALID_DEVID) | |
| #endif | |
| { | |
| return; | |
| } | |
| #endif |
Recommendation: Apply the #ifndef WOLF_CRYPTO_CB_FIND wrapper in both EccDerivePubBestEffort and wc_MlDsaKey_MakePublicKey. The early return in the helper is the only place this can be fixed - gating inside ecc_make_pub_ex would change the sign path too.
[wolfcrypt/src/asn.c:33197-33202] Skip the derivation outright under WOLF_CRYPTO_CB_FIND, since the devId gate cannot be relied on there:
#if defined(PLUTON_CRYPTO_ECC) || defined(WOLF_CRYPTO_CB)
/* Don't derive host-side public points for devId-tagged keys. Under
* WOLF_CRYPTO_CB_FIND, ecc_make_pub_ex() routes even INVALID_DEVID keys
* to a find callback, so skip the derivation entirely. */
#ifdef WOLF_CRYPTO_CB_FIND
return;
#else
if (key->devId != INVALID_DEVID) {
return;
}
#endif
#endif
Alternatively add !defined(WOLF_CRYPTO_CB_FIND) to the WOLFSSL_ECC_DERIVE_PUB_BEST_EFFORT guard at asn.c:33172-33179, mirroring the !defined(WOLF_CRYPTO_CB_ONLY_ECC) exclusion already present there.
| return ret; | ||
| } | ||
| /* get recip private key */ | ||
| else if (kari->direction == WC_PKCS7_DECODE) { |
There was a problem hiding this comment.
🟡 [Medium] Two sign-only ECC decode sites (pkcs7.c, ocsp.c) were not converted to derivePub=0
💡 SUGGEST convention
The PR systematically converted probe-only and sign-only decode sites to EccPrivateKeyDecodeEx(..., 0) (src/internal.c:33097, src/sniffer.c:2843, src/ssl_api_hs.c:1269, src/ssl_api_pk.c:2379, src/ssl_load.c:575, wolfcrypt/src/asn.c:9620, asn.c:10270, evp_pk.c:201, pkcs7.c:8676) but missed two meeting the same criteria. wc_PKCS7_ImportECC (wolfcrypt/src/pkcs7.c:2150) decodes a key used only for signing, and the cost is doubled: the immediately-following wc_ecc_check_key() now sees ECC_PRIVATEKEY instead of ECC_PRIVATEKEY_ONLY and runs the full public-key validation (_ecc_validate_public_key, ecc.c:11395), adding a second scalar multiply for the order check on every wc_PKCS7_EncodeSignedData. src/ocsp.c:2455 decodes ca->key.ecc, consumed only by OcspResponseEncode at src/ocsp.c:2766 for signing. Note the PKCS#7 site does gain a genuine fix from the derivation (its wc_ecc_check_key previously failed with ECC_INF_E on a private-only key), so if it is left deriving that should be stated as the reason rather than left as an oversight.
Suggestion:
| else if (kari->direction == WC_PKCS7_DECODE) { | |
| /* Skip the best-effort public point derivation done on decode: | |
| * this key is only used for signing, which needs the private | |
| * scalar alone. */ | |
| ret = EccPrivateKeyDecodeEx(pkcs7->privateKey, &idx, privKey, | |
| pkcs7->privateKeySz, 0); |
Recommendation: Convert src/ocsp.c:2455 to EccPrivateKeyDecodeEx(..., 0), and either convert wolfcrypt/src/pkcs7.c:2150 or add a comment stating the derivation is kept deliberately so wc_ecc_check_key succeeds on private-only keys.
| * as an opt-in fault check under WC_MLDSA_FAULT_HARDEN. Small-mem re-derives | ||
| * streaming instead, so it never pulls CheckKey (and full matrix A) in. | ||
| * One macro each so the guards can't drift. */ | ||
| #if defined(WOLFSSL_MLDSA_CHECK_KEY) || \ |
There was a problem hiding this comment.
🔵 [Low] WC_MLDSA_HAVE_CHECK_KEY's second arm is unreachable and the condition is duplicated in two spellings
💡 SUGGEST convention
Sites: wolfcrypt/src/wc_mldsa.c:262; wolfcrypt/src/wc_mldsa.c:258-268
WC_MLDSA_HAVE_CHECK_KEY is WOLFSSL_MLDSA_CHECK_KEY || (!NO_CHECK_KEY && WC_MLDSA_HAVE_MAKE_PUBLIC_KEY && WC_MLDSA_FAULT_HARDEN && !MAKE_KEY_SMALL_MEM). The second arm requires !WOLFSSL_MLDSA_NO_MAKE_KEY via WC_MLDSA_HAVE_MAKE_PUBLIC_KEY, and wolfssl/wolfcrypt/dilithium.h:317-323 derives WOLFSSL_MLDSA_PUBLIC_KEY and WOLFSSL_MLDSA_PRIVATE_KEY from exactly that, then auto-defines WOLFSSL_MLDSA_CHECK_KEY at dilithium.h:325-329 whenever both are set and WOLFSSL_MLDSA_NO_CHECK_KEY is not. The second arm is therefore strictly subsumed by the first, and WC_MLDSA_HAVE_CHECK_KEY is always identical to WOLFSSL_MLDSA_CHECK_KEY. The same condition is hand-copied into wolfssl/wolfcrypt/wc_mldsa.h:806-810 in a different form (expanding WC_MLDSA_HAVE_MAKE_PUBLIC_KEY inline and dropping !WOLFSSL_MLDSA_VERIFY_ONLY); the two are equivalent today only because VERIFY_ONLY force-defines NO_MAKE_KEY at dilithium.h:310-312 - precisely the fragile coupling the new macro was meant to remove. The PR comment One macro each so the guards can't drift is not achieved here.
Also, at wolfcrypt/src/wc_mldsa.c:258-268 (Info, bugs scan) — WC_MLDSA_HAVE_CHECK_KEY second arm is unreachable; small-mem still compiles CheckKey contrary to its comment:
The new WC_MLDSA_HAVE_CHECK_KEY macro is introduced with the stated goal that "Small-mem re-derives streaming instead, so it never pulls CheckKey (and full matrix A) in." That goal is not achieved, because the second arm of the macro can never be independently true. wolfssl/wolfcrypt/dilithium.h:317-330 already auto-defines WOLFSSL_MLDSA_CHECK_KEY whenever PUBLIC_KEY && PRIVATE_KEY && !WOLFSSL_MLDSA_NO_CHECK_KEY, and !WOLFSSL_MLDSA_NO_MAKE_KEY (required by WC_MLDSA_HAVE_MAKE_PUBLIC_KEY) implies both. So arm B implies arm A, WC_MLDSA_HAVE_CHECK_KEY is identically equal to WOLFSSL_MLDSA_CHECK_KEY, and every guard substitution in this PR is a semantic no-op. A WOLFSSL_MLDSA_MAKE_KEY_SMALL_MEM build still compiles wc_MlDsaKey_CheckKey() along with its full matrix-A allocation (up to ~57 KB for ML-DSA-87), which is the exact footprint the macro was added to avoid. The same condition is hand-copied into wolfssl/wolfcrypt/wc_mldsa.h:806-812 (headers cannot see the .c-only macro), creating two copies that must be kept in sync for no benefit. No runtime misbehavior — the small-mem streaming re-derivation is still what executes — but the code-size promise in the comment is not delivered.
Suggestion:
| #if defined(WOLFSSL_MLDSA_CHECK_KEY) || \ | |
| #ifdef WOLFSSL_MLDSA_CHECK_KEY | |
| #define WC_MLDSA_HAVE_CHECK_KEY | |
| #endif | |
| /* and revert wc_mldsa.h:806-810 to the plain `#ifdef WOLFSSL_MLDSA_CHECK_KEY` | |
| * it had before. */ |
Recommendation: Drop the dead second arm and revert the wc_mldsa.h prototype guard, so there is one condition rather than three spellings of the same thing.
[wolfcrypt/src/wc_mldsa.c:258-268] Either drop the redundant macro and keep using WOLFSSL_MLDSA_CHECK_KEY directly (removing the duplicated condition from wc_mldsa.h:806-812), or — if the small-mem size saving is actually wanted — make the exclusion real by having dilithium.h stop auto-defining WOLFSSL_MLDSA_CHECK_KEY under WOLFSSL_MLDSA_MAKE_KEY_SMALL_MEM, so that:
#define WC_MLDSA_HAVE_CHECK_KEY
can genuinely be false in a small-mem build. Until one of those happens, correct the comment so it does not claim a code-size reduction that is not delivered.
There was a problem hiding this comment.
The first arm (WOLFSSL_MLDSA_CHECK_KEY) is for when the user/build explicitly wants CheckKey. If the first arm is true, CheckKey must be compiled, even if small-mem is on.
The second arm is strictly for when MakePublicKey wants to implicitly pull in CheckKey for its own fault-hardening purposes. By putting !defined(WOLFSSL_MLDSA_MAKE_KEY_SMALL_MEM) in the second arm, you ensure that MakePublicKey doesn't drag the massive CheckKey footprint into a small-mem build unless the rest of the build already explicitly demanded it.
|
|
||
| Pass NULL as output to query the required buffer size. | ||
|
|
||
| If only the private key is set, the public key is derived and cached |
There was a problem hiding this comment.
🔵 [Low] Doxygen for PublicKeyToDer/KeyToDer states the derivation is unconditional, but the code gates it on output != NULL
🔧 NIT style
The added blocks for wc_MlDsaKey_PublicKeyToDer (doc line 909) and wc_MlDsaKey_KeyToDer (doc line 941) say "If only the private key is set, the public key is derived and cached in key" and add MEMORY_E/PUBLIC_KEY_E returns, with no mention that the size query is exempt. The implementation deliberately skips the derive when output == NULL (wc_mldsa.c:14398 and wc_mldsa.c:14513), precisely so a size query stays cheap and non-mutating, and neither error can be returned in that case. The equivalent block for wc_MlDsaKey_ExportPubRaw (doc line 787) is accurate because that function has no size-query mode.
Suggestion: If only the private key is set and output is non-NULL, the public key is
derived and cached in key; don't share key across threads during such a
call. A size query (output == NULL) never derives and never mutates key.
Recommendation: Qualify both blocks with the output != NULL condition so the documented error set matches the size-query path.
| byte placeholder = 0; | ||
| const byte* pub = (key->p != NULL) ? key->p : &placeholder; | ||
| #else | ||
| const byte* pub = key->p; |
There was a problem hiding this comment.
🔵 [Low] placeholder byte relies on an undocumented internal contract of SetAsymKeyDer
🔧 NIT style
Both wc_MlDsaKey_PublicKeyToDer (wc_mldsa.c:14463) and wc_MlDsaKey_KeyToDer (wc_mldsa.c:14522) pass a one-byte stack object as pubKey alongside a pubKeyLen of up to 2592, relying on the callee not reading it. I verified this holds: SetAsymKeyDerPublic (wolfcrypt/src/asn.c:13389) only reaches its XMEMCPY(output, pubKey, pubKeyLen) inside if ((ret == 0) && (output != NULL)), and SetAsymKeyDer (wolfcrypt/src/asn.c:34327) only touches pubKey inside if (ret == 0 && output != NULL) in both the template and non-template branches. But neither function's header comment states that contract, so a future change reading pubKey during the sizing pass would become a 2591-byte stack over-read with no compiler or test signal.
Suggestion: Add to the doxygen of SetAsymKeyDer() and SetAsymKeyDerPublic() in wolfcrypt/src/asn.c:
- @note pubKey must be non-NULL but is only dereferenced when output is
-
non-NULL; callers performing a size query may pass a placeholder.
Recommendation: Document the "pubKey is not read when output == NULL" contract at both callees so the placeholder trick is protected by more than a comment at the call site.
| key->type = ECC_PRIVATEKEY; | ||
| } | ||
|
|
||
| #ifdef WOLFSSL_ECC_BLIND_K |
There was a problem hiding this comment.
🔵 [Low] New key->ku scrub in ecc_make_pub_ex() is skipped on async WC_PENDING_E and never re-run
🔧 NIT
This PR adds a scrub of key->ku - the unblinded private scalar that ecc_get_k() reconstructs from k ^ kb under WOLFSSL_ECC_BLIND_K - at the end of ecc_make_pub_ex(). The scrub is explicitly skipped when the operation returns WC_PENDING_E, and nothing re-runs it: the PR's own comment two blocks above states the async result lands directly in key->pubkey and "the operation is not re-issued", and mp_forcezero(key->ku) appears nowhere else in ecc.c except wc_ecc_free() (line 8592). So on the async path the unblinded scalar stays resident in the key struct for the whole key lifetime, which is exactly what WOLFSSL_ECC_BLIND_K exists to prevent. The exposure is slightly wider than the sp_int digits: ecc_make_pub_sw() at line 5560 calls wc_mp_to_bigint_sz(ecc_get_k(key), &ecc_get_k(key)->raw, keySz) before IntelQaEccPointMul(), copying the plain scalar into the ku->raw heap bigint for the hardware; sp_forcezero() clears a->raw via wc_bigint_zero() (sp_int.c:5395), so the skipped scrub also skips that copy. Reachable with WOLFSSL_ASYNC_CRYPT + WC_ASYNC_ENABLE_ECC_KEYGEN + HAVE_INTEL_QA + WOLFSSL_ECC_BLIND_K, and this PR newly routes every default wc_EccPrivateKeyDecode() through ecc_make_pub_ex(), so the path is hit once per key decode rather than only at keygen. Rated Low: exploitation requires local memory-forensics access, and other ECC operations already leave ku populated on their own (pre-existing), so the marginal exposure over the status quo is small - but the newly added mitigation is incomplete on a reachable path.
Recommendation: Scrub key->ku on the async-completion path as well, so the pending case is covered rather than permanently skipped. Either call mp_forcezero(key->ku) from the async resume/completion handler that harvests key->pubkey, or - since ecc_get_k() recomputes ku from k ^ kb on every call and the QAT operation reads ku->raw, not ku->dp - clear ku->dp immediately after wc_mp_to_bigint_sz() and clear ku->raw in the completion path. At minimum, document in the comment that the async path defers the scrub to wc_ecc_free() so the gap is a known, deliberate limitation.
There was a problem hiding this comment.
Added a comment because i dont have the intel QA hardware to test
| } | ||
| #endif | ||
| #if defined(WOLFSSL_HAVE_SLHDSA) && \ | ||
| !defined(WOLFSSL_MLDSA_VERIFY_ONLY) && defined(WOLFSSL_HAVE_MLDSA) && \ |
There was a problem hiding this comment.
⚪ [Info] Redundant guard terms in tests/suites.c and the new ML-DSA test guards
🔧 NIT style
The new suites.c guard rationale checks out against tests/test-tls13-slhdsa-shake.conf and tests/test-tls13-slhdsa-sha2.conf: both sides do use ML-DSA-44 entity certs, so sign and verify are both needed, and the SLH-DSA root is only ever verified. WOLFSSL_MLDSA_NO_SIGN and WOLFSSL_MLDSA_NO_VERIFY are real macros. However, now that !defined(WOLFSSL_MLDSA_NO_SIGN) is present, the pre-existing !defined(WOLFSSL_MLDSA_VERIFY_ONLY) term is redundant - dilithium.h:310-312 force-defines WOLFSSL_MLDSA_NO_SIGN whenever VERIFY_ONLY is set. The same pattern appears in the new tests: test_mldsa_private_key_export_derives_public_key (tests/api/test_mldsa.c:7934) nests an inner !defined(WOLFSSL_MLDSA_NO_ASN1) && defined(WOLFSSL_MLDSA_PUBLIC_KEY) that merely restates its own outer guard, and several new guards spell out defined(WOLFSSL_MLDSA_PRIVATE_KEY) alongside !defined(WOLFSSL_MLDSA_NO_MAKE_KEY), which implies it via dilithium.h:321-323.
Suggestion: Drop !defined(WOLFSSL_MLDSA_VERIFY_ONLY) && from both suites.c guards, and drop the redundant inner #if at tests/api/test_mldsa.c:7934.
Recommendation: Trim the redundant terms so each guard states its condition once; this is the same drift risk as the WC_MLDSA_HAVE_CHECK_KEY finding.
|
My new skoll findings are on top of the Fenrir ones (removed the duplicates). |
bc2c6f2 to
b9e69bd
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10985
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 6
6 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
b9e69bd to
a4e41b0
Compare
stenslae
changed the title
a4e41b0 to
c83ac1b
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10985
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-src, wolfssl-bugs
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
|
Reverted implicit derivation inside the exporters. wc_MlDsaKey_MakePublicKey() will stay as the explicit way to derive it, so caller's can decide. Implicit derivation would have costed a keygen and a risk of hitting a error in public key derivation, and could inject thread safety issues. A private-only key now needs wc_MlDsaKey_MakePublicKey() before any export, with failure otherwise. |
fca19a4 to
4ad552a
Compare
4 participants
Description
Added support from deriving public keys from ML-DSA private keys and makde public keys mandatory for ML-DSA signature verification. Added test coverage.
wolfSSL/wolfssh#1120
Testing
Added test coverage.
Checklist