Repository navigation
Refuse to use token keys whose value is not decoded - #280
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
After an RSA-OAEP failure clears session initialization, key-destruction cleanup skips the still-allocated label, causing a memory leak when a subsequent non-OAEP initialization overwrites the parameters.
1 open issue
What changed in this PR
This PR prevents wolfPKCS11 from using token keys whose values remain encrypted or have been cleared after logout. It builds on #279’s concatenation mechanisms and strengthens key-state handling across cryptographic operations.
Changes:
- Reject undecoded keys in operations, derivation, wrapping, copying, and protected attribute access.
- Improve logout cleanup, operation-state ownership, and stored-object loading.
- Add regression coverage for logged-out keys, persisted attributes, operation cleanup, and concatenation.
| File | Description |
|---|---|
wolfpkcs11/pkcs11.h |
Adds concatenation mechanism constants and parameter types. |
wolfpkcs11/internal.h |
Declares key-state and concatenation helpers; adjusts ML-KEM key capacity. |
tests/pkcs11v3test.c |
Tests ML-DSA and ML-KEM key access after logout. |
tests/pkcs11test.c |
Adds key-state, cleanup, concatenation, and derivation regressions. |
tests/pkcs11str.c |
Tests stored keys and attributes before login. |
src/slot.c |
Advertises concatenation mechanisms and their capabilities. |
src/internal.c |
Implements key-state checks, loading fixes, cleanup, and concatenation helpers. |
src/crypto.c |
Enforces decoded-key checks and implements concatenation derivation. |
Review effort: Balanced
Frauschi
force-pushed
the
undecoded-key-use
branch
from
November 2, 2026 16:32
cabdc76 to
33a85b0
Compare
Member
Author
|
@wolfSSL-Fenrir-bot review balanced |
Member
Author
|
@claude review |
wolfSSL-Fenrir-bot
previously requested changes
Nov 2, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #280
Scan targets checked: wolfpkcs11-src, wolfpkcs11-bugs
Coverage: 5 of 8 in-scope changed file(s) opened by the reviewer; not opened: src/slot.c, wolfpkcs11/internal.h, wolfpkcs11/pkcs11.h
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
Review tier: Balanced
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Implement the PKCS#11 miscellaneous concatenation derive mechanisms (v3.1/v3.2 section 6.43.3 to 6.43.5). They let a hybrid key agreement such as ETSI TS 103 744 build PSK || Z_ECDH || Z_MLKEM as a sensitive, non-extractable secret and feed it to CKM_HKDF_DERIVE without exporting the parts. A CKA_VALUE_LEN shorter than the concatenation keeps the leading bytes, matching NSS and SoftHSM, and the data may then be longer than the largest key. The shorter length may not end inside the bytes of a sensitive or non-extractable input key: a caller could otherwise derive keys one byte longer at a time, use each to sign or encrypt, and find every key byte in 256 tries. Dropping trailing data, or all of a key, stays allowed. Without CKA_VALUE_LEN the key takes the summed length, as in NSS softoken; NSS asks for a CKK_HKDF key this way when it combines the secrets of its hybrid TLS groups. An AES key has no single well-defined length, so it needs CKA_VALUE_LEN and otherwise gets CKR_TEMPLATE_INCOMPLETE. CKA_KEY_TYPE stays required, as for the other derive mechanisms. For CKM_CONCATENATE_BASE_AND_KEY the second key must be a secret key with CKA_DERIVE set, the derived key is sensitive if either input is and non-extractable if either input is, and CKA_ALWAYS_SENSITIVE / CKA_NEVER_EXTRACTABLE need both inputs to have them. A CKM_CONCATENATE_DATA_AND_BASE result truncated to the caller's data holds no key bytes, so it gets neither flag. CKA_WRAP_WITH_TRUSTED is set when either input has it, so a copy cannot be wrapped with an untrusted key. A token key whose value is not decoded, before login or after logout, gives CKR_USER_NOT_LOGGED_IN instead of being concatenated as empty or zeroed bytes. An AES key wrapped by the STM32U5 DHUK gives CKR_KEY_FUNCTION_NOT_PERMITTED, as its stored value is not the key. The concatenation output contains the input key bytes, so these three mechanisms inherit CKA_SENSITIVE and CKA_EXTRACTABLE even in builds with WOLFPKCS11_LEGACY_DERIVE_NO_INHERIT (enabled for NSS); otherwise concatenating a sensitive key with empty data would give a readable copy. Under WP11_NSS_PERMISSIVE_KEY_DEFAULTS the second key skips the CKA_DERIVE check, as the base key does. Related derive changes: - Drop WP11_DERIVE_SECRET_KEY. Concatenation needs no crypto, so the secret-key derive path is always built. - For the other mechanisms, an AES key derived without CKA_VALUE_LEN takes the whole derived length. When that is not 16, 24 or 32 bytes, return CKR_TEMPLATE_INCOMPLETE before creating the object instead of failing in the key setter with CKR_FUNCTION_FAILED. A template that has CKA_VALUE_LEN keeps the error object creation returns for it. - Use a WP11_MAX_SYM_KEY_SZ of 256 bytes in ML-KEM builds without DH. The ECC-only value of 66 bytes could not hold a 96-byte PSK || ECDH || ML-KEM input. Fixes wolfSSL#264.
Encrypted token objects (RSA, EC, DH, ML-DSA and ML-KEM private keys, AES, generic secret and HKDF keys) are encoded at logout: the key value in memory is zeroed and only the encrypted copy is kept. Objects loaded from the store stay undecoded until the user logs in. A token key with CKA_PRIVATE false is still visible in a public session in both states, and every operation except concatenation used it as is. HMAC sign and verify, AES encrypt and decrypt, ECDSA sign, C_WrapKey, HKDF and C_DigestKey all returned CKR_OK while working with an all-zero key. Check the key state before using a key: - CheckOpSupported, so every operation init and derive base key. The check runs after the usage flag check, so a key without the flag still gives CKR_KEY_FUNCTION_NOT_PERMITTED. - The key being wrapped in C_WrapKey. - The HKDF salt key, when it is a secret key. - C_DigestKey, under the object lock. The digest operation is aborted on failure. - The source object of C_CopyObject. The copy kept the encoded key data but could never decode it. An undecoded key gives CKR_USER_NOT_LOGGED_IN when no user is logged in, and CKR_FUNCTION_FAILED when the user is logged in, as the stored key then failed to decrypt. The concatenate mechanisms now share the same state check. C_GetAttributeValue returned the zeroed CKA_VALUE of a non-sensitive, extractable key with CKR_OK. While a key is encoded or undecoded, each attribute held in its encrypted record now gives CKR_ATTRIBUTE_SENSITIVE: CKA_VALUE and the check value of a secret key, CKA_VALUE of a DH or EC private key, the modulus, modulus bits and public exponent of an RSA private key, the EC parameters and EC point of an EC private key, and the parameter set of an ML-DSA or ML-KEM private key. Some of these survived logout, depending on what the wolfCrypt free functions keep, but none exist for a key that was loaded and never decoded, so a key would answer differently before the first login than after a logout. Reading the EC parameters of such a key dereferenced a NULL curve. CKA_VALUE_LEN, which follows from the stored record length, and the DH domain parameters, which are stored in the clear, stay readable. The key length is now set when a secret key is loaded, as it read 0 until the first login. Objects that are not encrypted, such as public keys, were also decoded only at login. Before the first login a public key had no key data, and reading CKA_EC_POINT of an EC public key dereferenced a NULL curve. These objects are now decoded when the token is loaded, and login decodes them again only when that failed, so a damaged record still fails the login. An operation started before C_Logout kept running afterwards with the scrubbed key. AES-GCM and TLS MAC used the zero key and returned CKR_OK. Logout now drops the session references to a key it scrubbed, in the same way C_DestroyObject does for a destroyed key. Asymmetric operations, AES-GCM, AES-CCM, AES-ECB, TLS MAC and the NSS TLS PRF read the key while they run, so their reference is dropped. The owning session then releases the operation state on its next call and returns CKR_OPERATION_NOT_INITIALIZED, or starts the new operation it was asked for. AES-CBC, HMAC and the other operations that copy the key at init keep running with the right key, as after a destroy, since wolfCrypt's PKCS#11 layer destroys the key before it finishes a multi-part HMAC. A digest started after an operation that used the key keeps running. An init that runs at the same time as C_Logout can still pick up the key before it is scrubbed. Closing that needs a state check where each key is read and is left for a follow-up. Destroy used to free another session's RSA-OAEP label and reset its operation state from the destroying thread. The owning thread could then free the label a second time at the end of a decrypt, and hash contexts were dropped without being freed. Only the owning session changes its operation state now. A failed RSA-OAEP encrypt or decrypt left its label allocated, and it leaked once the session moved on to another mechanism. The label is now freed whatever the result. test_wrap_key_wrap_with_trusted created its CKA_TRUSTED wrapping key as a token object in an SO session. Such a key is encrypted under the user token key, which an SO login never sets, so it never decodes for the user and the test wrapped with a zero key. The test now wraps with a public session object, which is not encrypted, and checks that the SO token key fails with CKR_FUNCTION_FAILED rather than being used. SO created token keys need a separate fix.
Frauschi
force-pushed
the
undecoded-key-use
branch
from
November 2, 2026 17:42
33a85b0 to
fe70490
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #280
Scan targets checked: wolfpkcs11-src, wolfpkcs11-bugs
Coverage: 2 of 2 in-scope changed file(s) opened by the reviewer
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
wolfSSL-Fenrir-bot
dismissed
their stale review
November 2, 2026 17:54
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
LinuxJedi
approved these changes
Nov 3, 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.
A token key with CKA_PRIVATE false stays usable in a public session after C_Logout and before the first login, while its value is zeroed or encrypted. HMAC, AES, ECDSA, C_WrapKey, HKDF and C_DigestKey used it anyway and returned CKR_OK with an all-zero key. Stacked on #279.
Changes
Testing
Follow-ups