Fix resource leaks and secure buffer erasing

[LinuxJedi]

wc_ecc_init_ex called twice and wrong session being finalised, along with added ForceZero usage.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #172

Scan targets checked: wolfpkcs11-bugs, wolfpkcs11-src
Findings: 3

Medium (2)

Removal of wc_ecc_init_ex before wc_EccPrivateKeyDecode may leave ecc_key uninitialized

File: src/internal.c:3964-3966
Function: wp11_Object_Decode_EccKey
Category: API contract violations

The PR removes the wc_ecc_init_ex(key, NULL, object->devId) call that previously occurred between decryption and wc_EccPrivateKeyDecode. In the wolfSSL API, wc_EccPrivateKeyDecode expects an already-initialized ecc_key structure — it does not internally call wc_ecc_init. If no other initialization of key occurs earlier in this function, decoding into an uninitialized ecc_key is undefined behavior and will likely corrupt memory or fail. Additionally, the devId was being propagated via wc_ecc_init_ex; without this call, the key may lose its device ID association, which matters for hardware crypto offload.

}
-        if (ret == 0) {
-            ret = wc_ecc_init_ex(key, NULL, object->devId);
-        }
         if (ret == 0) {
             /* Decode ECC private key. */
             ret = wc_EccPrivateKeyDecode(der, &idx, key, len);

Recommendation: Verify that key is initialized (via wc_ecc_init or wc_ecc_init_ex) earlier in wp11_Object_Decode_EccKey before this decode call. If the removal was intentional to fix a double-init, ensure the earlier init also passes object->devId. If there is no earlier init, restore the removed wc_ecc_init_ex call.

---

Removal of wc_ecc_init_ex may lose device ID association for ECC keys

File: src/internal.c:3964-3966
Function: wp11_Object_Decode_EccKey
Category: PKCS#11 API contract violations

The PR removes the wc_ecc_init_ex(key, NULL, object->devId) call that previously executed before wc_EccPrivateKeyDecode. This call initialized the ECC key structure with the object's devId, which directs cryptographic operations to a specific hardware device (e.g., a TPM or HSM). If wc_EccPrivateKeyDecode does not internally call wc_ecc_init_ex with the correct devId (it typically calls wc_ecc_init which defaults devId to INVALID_DEVID), the decoded key will lose its hardware device association. This could cause private key operations to fall back to software, potentially exposing key material in process memory that should remain on hardware, or cause operations to fail entirely.

/* Removed code: */
        if (ret == 0) {
            ret = wc_ecc_init_ex(key, NULL, object->devId);
        }
        if (ret == 0) {
            /* Decode ECC private key. */
            ret = wc_EccPrivateKeyDecode(der, &idx, key, len);

Recommendation: Verify that wc_EccPrivateKeyDecode internally initializes the key with the appropriate devId. If it does not, restore the wc_ecc_init_ex call or ensure devId is set on the key after decoding, e.g., by calling wc_ecc_set_devId(key, object->devId) after the decode succeeds.

---

Low (1)

Session finalization loop may use-after-free if wp11_Session_Final frees session memory

File: src/internal.c:6424
Function: WP11_Slot_CloseSessions
Category: Resource leaks

The PR correctly fixes a bug where the loop was always passing slot->session (the list head) to wp11_Session_Final instead of curr. However, the corrected loop for (curr = slot->session; curr != NULL; curr = curr->next) wp11_Session_Final(curr); reads curr->next after curr has been finalized. If wp11_Session_Final frees the session's memory or modifies the linked list structure (e.g., unlinking curr), then the subsequent curr->next dereference would access freed memory. The fix is a clear improvement over the original bug (which repeatedly finalized only the first session, leaking all others), but the iteration pattern should be verified.

for (curr = slot->session; curr != NULL; curr = curr->next)
        wp11_Session_Final(curr);

Recommendation: Verify that wp11_Session_Final does not free the session struct or modify the linked list. If it does, use a safe iteration pattern that captures next before calling finalize: for (curr = slot->session; curr != NULL; curr = next) { next = curr->next; wp11_Session_Final(curr); }.

---

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

[Copilot]

Pull request overview

This PR fixes a couple of concrete resource-handling and security hygiene issues in the wolfPKCS11 implementation: it removes a redundant ECC initialization that could leak/overwrite state, corrects session finalization logic when closing all sessions on a slot, and replaces non-guaranteed memory wiping with a secure zeroing routine.

Changes:

• Remove a duplicate wc_ecc_init_ex() call during ECC key decode to avoid re-initializing an already-initialized key structure.
• Fix WP11_Slot_CloseSessions() to finalize each session in the list (instead of repeatedly finalizing only slot->session).
• Use wc_ForceZero() to securely erase sensitive buffers before freeing (and erase TPM-wrapping temporary private key material).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/internal.c	Fixes ECC double-init, corrects session finalization loop, and securely wipes sensitive key material/buffers.
src/crypto.c	Switches sensitive-buffer clearing from XMEMSET to wc_ForceZero before freeing.

---

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

[LinuxJedi]

Fenrir's review are false positives due to it not having more codebase context for the scan.