Misc wolfcrypt fixes

[philljj]

Description

Fix some wolfcrypt issues and add tests.

Fixes F-3981, 4497, 5380, 5383.

Testing

./configure --enable-dsa --enable-keygen && make && ./tests/unit.test

[Copilot]

Pull request overview

This PR addresses several wolfCrypt correctness and safety issues, and adds API-level regression tests to lock in the intended error behavior.

Changes:

• Zeroize PKCS#12 PBKDF temporary buffer prior to free to avoid leaving sensitive material in memory.
• Make wc_HmacUpdate() return an error for unsupported/uninitialized macType (e.g., update-before-setkey), and add regression tests for this.
• Harden key export/population paths: prevent exporting an ungenerated DSA key and avoid leaving a dangling pointer on ECC EVP_PKEY population failure.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
wolfcrypt/src/pwdbased.c	Clears PBKDF working buffer before freeing.
wolfcrypt/src/hmac.c	Ensures invalid/unsupported macType in wc_HmacUpdate() returns BAD_FUNC_ARG.
wolfcrypt/src/evp.c	Avoids dangling pkey->pkey.ptr on ECC public-key DER generation failure.
wolfcrypt/src/dsa.c	Rejects raw export when DSA key material is not present.
tests/api/test_hmac.c	Adds update-before-setkey negative tests across multiple hash types.
tests/api/test_dsa.c	Adds export-before-keygen negative test for DSA raw export.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

MemBrowse Memory Report

gcc-arm-cortex-m4

• FLASH: .text +128 B (+0.1%, 198,830 B / 262,144 B, total: 76% used)

gcc-arm-cortex-m4-crypto-only

• FLASH: .text +64 B (+0.0%, 173,614 B / 262,144 B, total: 66% used)

gcc-arm-cortex-m4-dtls13

• FLASH: .text +128 B (+0.1%, 179,672 B / 1,048,576 B, total: 17% used)

gcc-arm-cortex-m4-openssl-compat

• FLASH: .text +512 B (+0.1%, 766,764 B / 1,048,576 B, total: 73% used)

gcc-arm-cortex-m4-pkcs7

• FLASH: .text +320 B (+0.2%, 211,313 B / 262,144 B, total: 81% used)

gcc-arm-cortex-m4-pq

• FLASH: .text +384 B (+0.1%, 277,528 B / 1,048,576 B, total: 26% used)

gcc-arm-cortex-m4-rsa-only

• FLASH: .text +320 B (+0.1%, 322,936 B / 1,048,576 B, total: 31% used)

gcc-arm-cortex-m7

• FLASH: .text +128 B (+0.1%, 198,830 B / 262,144 B, total: 76% used)

gcc-arm-cortex-m7-pq

• FLASH: .text +320 B (+0.1%, 278,104 B / 1,048,576 B, total: 27% used)

gcc-arm-cortex-m7-tls13

• FLASH: .text +64 B (+0.0%, 234,528 B / 262,144 B, total: 89% used)

linuxkm-pie

• Data: __patchable_function_entries +56 B (+0.2%, 24,264 B)

linuxkm-standard

• Data: __patchable_function_entries +40 B (+0.1%, 45,960 B)

stm32-sim-stm32h753

• FLASH: .text +64 B (+0.0%, 181,556 B / 2,097,152 B, total: 9% used)
  No memory changes detected for:
• gcc-arm-cortex-m0plus
• gcc-arm-cortex-m3
• gcc-arm-cortex-m4-baremetal
• gcc-arm-cortex-m4-min-ecc
• gcc-arm-cortex-m4-sp-math
• gcc-arm-cortex-m4-tls12
• gcc-arm-cortex-m4-tls13

[philljj]

Retest this please.

(unclear, looks like tests passed, then hung).

[dgarske]

Skoll Code Review

Scan type: reviewOverall recommendation: COMMENT
Findings: 6 total — 6 posted, 0 skipped
6 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [Medium] PKCS12 PBKDF zeroization fix is incomplete: B buffer still holds derived key material — wolfcrypt/src/pwdbased.c:780-784
• [Medium] New update-before-setkey test expectation will not hold on WOLFSSL_KCAPI_HMAC builds — tests/api/test_hmac.c:308-312 (and 354, 400, 446, 492)
• [Medium] wc_DsaExportKeyRaw now rejects public-only keys entirely - confirm intended — wolfcrypt/src/dsa.c:712-715
• [Low] wc_HmacUpdate mutates innerHashKeyed before failing on unknown macType — wolfcrypt/src/hmac.c:1050-1139
• [Low] New DSA negative test passes uninitialized xOutSz/yOutSz — tests/api/test_dsa.c:558-562
• [Low] pkey-pkey_sz left stale after pkey.ptr is reset to NULL on DER failure — wolfcrypt/src/evp.c:9847-9855

Review generated by Skoll

[dgarske]

🟠 [Medium] PKCS12 PBKDF zeroization fix is incomplete: B buffer still holds derived key material

The PR adds ForceZero(buffer, totalLen) to scrub D || S || P (password-derived data) before free, matching the WC_PKCS12_PBKDF_USING_MP_API implementation branch (line 608). However the fix is incomplete: B holds A_i, the actual derived key blocks, and the final output block is still present in B when the function exits. In non-WOLFSSL_SMALL_STACK builds B is a stack array (ALIGN8 byte B[WC_MAX_BLOCK_SIZE]) and WC_FREE_VAR_EX expands to WC_DO_NOTHING, so B is never cleared; in WOLFSSL_SMALL_STACK builds B is heap-allocated and XFREEd without zeroing. The MP-API branch of this same function explicitly does ForceZero(B, WC_MAX_BLOCK_SIZE) (line 605), so this branch is now inconsistent with its sibling and leaves the highest-value secret (the derived key itself) unscrubbed while scrubbing the lower-value input material.

Fix: Add ForceZero(B, WC_MAX_BLOCK_SIZE); before WC_FREE_VAR_EX(B, ...) so the derived key block is scrubbed in both the stack and small-stack configurations, matching the MP-API implementation branch.

[dgarske]

🟠 [Medium] New update-before-setkey test expectation will not hold on WOLFSSL_KCAPI_HMAC builds

The new ExpectIntEQ(wc_HmacUpdate(...), BAD_FUNC_ARG) checks are guarded only for HAVE_SELFTEST and pre-7.0 HAVE_FIPS. But WOLFSSL_KCAPI_HMAC builds provide their own wc_HmacUpdate (wolfcrypt/src/port/kcapi/kcapi_hmac.c:184) that does not contain the new default: ret = BAD_FUNC_ARG logic. With macType == WC_HASH_TYPE_NONE (no SetKey yet), that implementation falls into its default: case and calls kcapi_md_update(hmac->handle, ...) with hmac->handle == NULL, returning a kcapi/errno-style error code (not BAD_FUNC_ARG), so the new assertions would fail on that port. The same class of concern applies to async-HW HMAC paths (HAVE_CAVIUM) that dispatch before the macType switch. This only matters if those configurations run tests/unit.test, but the guard is incomplete as written.

Fix: Either extend the guard with !defined(WOLFSSL_KCAPI_HMAC) (and any other port that overrides wc_HmacUpdate), or mirror the unknown-macType BAD_FUNC_ARG check in the KCAPI port so the API contract is uniform across implementations.

[dgarske]

🟠 [Medium] wc_DsaExportKeyRaw now rejects public-only keys entirely - confirm intended

The new check if (mp_iszero(&dsa->x) || mp_iszero(&dsa->y)) return BAD_FUNC_ARG; is correct for the params-only case (a valid generated key can never have x == 0 or y == 0, since x is in [1, q-1] and y = g^x mod p), and it properly fixes the export-before-MakeKey case the new test covers. However, because the check requires BOTH x and y to be non-zero and runs before the LENGTH_ONLY_E size-query path, it also changes behavior for a public-only DsaKey (e.g. populated via wc_DsaPublicKeyDecode, where x stays zero): previously such a key would export y successfully with *xSz = 0, and the NULL-buffer size query returned LENGTH_ONLY_E; now both return BAD_FUNC_ARG. There are no in-tree callers besides the test, so blast radius is limited to external users, and the function is documented as a key-pair export, so this is likely intended - but it is a silent behavioral break for any external caller using it on public-only keys.

Fix: Confirm the public-only-key behavior change is intended. If it is, consider noting it in the function's doc comment ("requires a complete key pair; returns BAD_FUNC_ARG if x or y is not set") so the contract is explicit.

[philljj]

wc_DsaExportKeyRaw() already requires both x and y (priv and pub) pointer args

    if (x == NULL || y == NULL)
        return BAD_FUNC_ARG;

and it checks both xSz, ySz against xLen, yLen. So would be strange to use this with public only key.

I can move the check later and make it && like

    if (mp_iszero(&dsa->x) && mp_iszero(&dsa->y)) {                             
        return BAD_FUNC_ARG;                                                    
    }

so it would only err if key totally 0.

[dgarske]

🔵 [Low] wc_HmacUpdate mutates innerHashKeyed before failing on unknown macType

The new default: ret = BAD_FUNC_ARG; catches the unkeyed/unknown-macType case, but only after earlier code has already mutated state: with macType == WC_HASH_TYPE_NONE, HmacKeyHashUpdate() / HmacKeyCopyHash() silently return 0 (their switches have a bare default: break;), so hmac->innerHashKeyed = WC_HMAC_INNER_HASH_KEYED_SW is set on a context that has no key before the error is returned. This is harmless in practice today (wc_HmacSetKey_ex resets innerHashKeyed = 0 at line 561, and repeat Update/Final calls still hit the BAD_FUNC_ARG defaults), but a failing call that leaves state modified is a wart, and the helpers silently succeeding on an unknown type is what forced the error detection to live this late. Note the paired removal of ret = 0; /* reset error code */ in the crypto-cb fall-through is correct: that assignment (added in 58fe278 for a scan-build dead-store warning) becomes a dead store now that every switch branch, including default, assigns ret, and all intermediate paths (STM32 block, innerHashKeyed block, async returns) assign or return independently. The new default also matches wc_HmacFinal's existing default: ret = BAD_FUNC_ARG; convention.

Fix: Optional cleanup: validate hmac->macType at the top of wc_HmacUpdate (before the innerHashKeyed re-key block) so a failing call leaves the context untouched. Current behavior is functionally correct, so this is take-it-or-leave-it.

[dgarske]

🔵 [Low] New DSA negative test passes uninitialized xOutSz/yOutSz

xOutSz and yOutSz are declared at line 550 but only initialized at lines 566-567, after the new export-before-MakeKey call. The new call therefore passes pointers to uninitialized sizes. It works today only because the new zero-key check in wc_DsaExportKeyRaw returns before *xSz is read - the test silently depends on the internal ordering of checks. If the implementation ever reorders validation (e.g. buffer-size checks first), the function would compare against indeterminate values, making the test flaky and tripping MSan-style analyzers.

Fix: Move the xOutSz = sizeof(xOut); yOutSz = sizeof(yOut); initialization above the new negative-test call.

[dgarske]

🔵 [Low] pkey-pkey_sz left stale after pkey.ptr is reset to NULL on DER failure

The added pkey->pkey.ptr = NULL; correctly fixes the dangling pointer: previously, when wc_EccPublicKeyToDer() failed after the XREALLOC'd buffer had already been assigned to pkey->pkey.ptr, the buffer was freed but the pointer kept pointing at freed memory, leading to a double free in wolfSSL_EVP_PKEY_free() (or use-after-free on any later access). Good fix. One loose end: pkey->pkey_sz still holds the old (now meaningless) size while pkey.ptr is NULL, leaving the two fields inconsistent on the failure path. Most consumers null-check pkey.ptr first, but resetting pkey_sz = 0 alongside would make the failure state fully consistent. (The XFREE(derBuf, NULL, ...) heap-hint mismatch with the pkey->heap used for allocation is pre-existing on that line and out of scope, though it could be fixed in the same touch.)

Fix: Also reset pkey->pkey_sz = 0 on this failure path so the size field cannot be paired with a NULL pointer. Optionally use pkey->heap in the adjacent XFREE while touching this block. Note this fix has no accompanying regression test, which is acceptable given the failure requires DER-encode fault injection.

[dgarske]

See skoll feedback -> #10647 (review)