Several hardening fixes#201
Merged
aidangarske merged 7 commits intoJun 24, 2026
Merged
Conversation
LinuxJedi
commented
Jun 22, 2026
LinuxJedi
commented
Member
- Cap HKDF expand output length in C_DeriveKey - F-4806
- Guard against length underflow in wp11_Object_Decode_SymmKey - F-4263
- Clamp key zeroize length in WP11_Object_Free
- Bound secret key length in WP11_Object_SetSecretKey - F-5157
- Drop active object reference on C_DestroyObject
- Validate mechanism and length bounds before key use in C_VerifyRecover - F-3411, F-5152, F-3838
- Use a local word32 for RSA verify-recover output length - F-1611
LinuxJedi
force-pushed
the
crypto-length-and-session-handling
branch
from
9b2054e to
d4552c1
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR focuses on hardening wolfPKCS11 against length/bounds issues and stale object references, reducing the risk of underflows, oversized allocations/zeroization, and use-after-free during active operations.
Changes:
- Add length/bounds checks and clamping to prevent underflow/overflow and out-of-bounds zeroization/copies (symmetric key decode, secret key set, object free).
- Cap HKDF expand output length to the RFC 5869 maximum to avoid large allocations prior to per-digest enforcement.
- Prevent active-operation use-after-free by clearing cached “current object” references on object destruction; tighten mechanism/type/length validation in
C_VerifyRecover.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
wolfpkcs11/internal.h |
Adds internal API for clearing active object references across sessions. |
src/internal.c |
Implements additional bounds checks, clamps zeroization lengths, bounds secret key lengths, and adds slot-wide active-object clearing helper. |
src/crypto.c |
Uses the new active-object clearing during C_DestroyObject, strengthens C_VerifyRecover validation, and caps HKDF requested output length. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WP11_Rsa_Verify_Recover passed the caller's CK_ULONG_PTR outLen to wc_RsaDirect cast as (word32*). On LP64 platforms CK_ULONG is 8 bytes while wc_RsaDirect writes only a 4-byte word32 through the pointer, so the upper bytes of *outLen retained their caller-supplied value. The inflated length then drove the front-padding scan and XMEMMOVE. Use a local word32 for the wc_RsaDirect output length and assign it back to *outLen on success, matching the pattern used by the other RSA functions in this file. F-1611
C_VerifyRecover read the active session object and called WP11_Rsa_KeyLen(obj) before checking the mechanism or that a verify-recover operation had been initialized. Because the active object pointer is set by any *Init function, a non-RSA object left active (for example an HMAC generic-secret key) was dereferenced as an RSA key. Move the mechanism switch and operation-initialized checks ahead of WP11_Rsa_KeyLen, and add an explicit CKK_RSA key-type check. Also add CK_ULONG_FITS_WORD32 guards on ulSignatureLen and *pulDataLen to reject values that would not fit a word32, and set *pulDataLen to the required size when returning CKR_BUFFER_TOO_SMALL as required by PKCS#11. F-3411, F-5152, F-3838
LinuxJedi
force-pushed
the
crypto-length-and-session-handling
branch
2 times, most recently
from
38127c8 to
7dc2a4b
Compare
aidangarske
reviewed
Jun 22, 2026
aidangarske
left a comment
aidangarske
left a comment
Member
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-security + bugsOverall recommendation: REQUEST_CHANGES
Findings: 2 total — 2 posted, 0 skipped
2 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [High] [review+bugs] Decoder rejects zero-length symmetric keys that the encoder still writes —
src/internal.c:5251 - [Medium] [review-security+bugs] Active-object clearing can orphan RSA-OAEP labels —
src/internal.c:8937-8942
Review generated by Skoll
An operation init function (for example C_SignInit) caches the key object in session->curr as a raw pointer. C_DestroyObject freed the object without clearing that pointer, so completing an asymmetric operation (for example C_Sign with an RSA key) read freed memory. Add WP11_Slot_ClearActiveObject and call it from C_DestroyObject before WP11_Object_Free. For asymmetric key types, which are dereferenced while the operation runs, it scans the slot's sessions and clears session->curr and the operation state for any session whose active object is the one being destroyed. Token objects can be active in more than one session, so all sessions on the slot are scanned. Symmetric operations (AES, HMAC/generic-secret, HKDF) copy the key into the operation context during their *Init and complete without referring to the object again, so a caller may legitimately destroy the key before finishing a multi-part operation. Those operations are left untouched so they can still complete.
WP11_Object_SetSecretKey stored the caller-supplied CKA_VALUE_LEN into key->len after narrowing CK_ULONG to word32 and only checked the result against WP11_MAX_SYM_KEY_SZ afterwards. A value above the cap left the oversized length recorded even though BUFFER_E was returned, and a value outside word32 range (for example 0x100000000) narrowed to a small value that passed the check, so the call returned CKR_OK with mismatched length and key bytes. Bound the requested length against WP11_MAX_SYM_KEY_SZ before narrowing to word32, apply the same bound to the CKA_VALUE length, and reset key->len to zero on any error path so teardown does not zeroize or copy past the fixed key->data buffer. F-5157
WP11_Object_Free passed object->data.symmKey->len (and the DH equivalent) directly to wc_ForceZero. Both key buffers are fixed-size arrays, so a length recorded larger than the buffer would zeroize past its end. Clamp the zeroize length to the size of the backing buffer for both the symmetric-key and DH branches as a backstop.
The length guard computed object->keyDataLen - AES_BLOCK_SIZE in signed arithmetic and compared it to WP11_MAX_SYM_KEY_SZ. A keyDataLen below AES_BLOCK_SIZE produced a negative value that passed the comparison and was then passed as an unsigned length to wp11_DecryptData and used to size a copy into the fixed symmKey->data buffer. Reject keyDataLen below AES_BLOCK_SIZE first, then bound the payload length as unsigned. A keyDataLen of exactly AES_BLOCK_SIZE is the valid encoding of a zero-length secret (the encoder writes symmKey->len + AES_BLOCK_SIZE), so it must continue to load. F-4263
The CKM_HKDF_DERIVE/CKM_HKDF_DATA expand branch only rejected a requested output length above word32 range, so a length up to nearly 4 GB was allocated and zeroized before wc_HKDF_Expand rejected it per RFC 5869. Cap the requested length to the RFC 5869 maximum (255 * HashLen) before allocation; this also keeps the value within word32 range. F-4806
LinuxJedi
force-pushed
the
crypto-length-and-session-handling
branch
from
7dc2a4b to
f45137d
Compare
Member
Author
|
@aidangarske items flagged should be fixed now. |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #201
Scan targets checked: wolfpkcs11-bugs, wolfpkcs11-src
No new issues found in the changed files. ✅
aidangarske
approved these changes
Jun 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.