Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The ECDSA test helper and assertions lack PKCS#11 ECC guards, causing return-value and session-count assertions to fail when NO_PKCS11_ECC is defined but software ECC signing remains enabled.
1 open issue
What changed in this PR
This PR updates wolfSSL’s PKCS#11 integration to preserve shared logins across session closes and store token-owned copies of user PINs.
Changes:
- Accepts existing logins, avoids session logout, and closes sessions when login fails.
- Copies and securely clears PINs, with a new
Open_exoption to discard them after login. - Adds API documentation and tests for login sharing, PIN ownership, and session lifecycle.
| File | Description |
|---|---|
wolfssl/wolfcrypt/wc_pkcs11.h |
Adds PIN ownership state and declares Open_ex. |
wolfssl/wolfcrypt/pkcs11.h |
Defines the already-logged-in return code. |
wolfcrypt/src/wc_pkcs11.c |
Implements shared-login handling, PIN ownership, and session cleanup. |
tests/api/test_pkcs11.h |
Declares and registers the new tests. |
tests/api/test_pkcs11.c |
Tests login sharing, PIN copying, and optional PIN disposal. |
doc/dox_comments/header_files/pkcs11.h |
Documents PIN ownership and session behavior. |
Review effort: Balanced
|
PKCS#11 keeps the login state per application, not per session. When a second Pkcs11Token for the same token logs in, C_Login returns CKR_USER_ALREADY_LOGGED_IN, which wolfSSL turned into WC_HW_E. NXP's SMW module does this for every token after the first. Treat CKR_USER_ALREADY_LOGGED_IN as logged in. A session that had logged in also called C_Logout before C_CloseSession. C_Logout ends the login of every session of the application, so closing a per-operation session of one token logged out the open session of any other token, and its private key operations then failed. PKCS#11 returns the application to public sessions when its last session on the token closes, and wc_Pkcs11Token_Final closes all of them, so wolfSSL no longer calls C_Logout.
wc_Pkcs11Token_Init and wc_Pkcs11Token_InitName take the PIN as a const buffer but stored the caller's pointer, and wc_Pkcs11Token_Final then zeroed that buffer. A PIN in read-only memory, such as a string literal, crashed on finalize, and a caller's buffer had to stay valid for the token's lifetime. The token now allocates a copy of the PIN from the device heap, kept in a heap field appended to Pkcs11Token, and wc_Pkcs11Token_Final zeroes and frees that copy. An empty PIN gets a one-byte buffer, so a token initialized with a PIN always holds a non-NULL copy. A negative PIN length is rejected with BAD_FUNC_ARG before the token is set up. A pinOwned bit marks the token's own copy, so a PIN that the application assigned to the token itself is only forgotten, not freed. wc_Pkcs11Token_Final frees it only for a token that was set up, so finalizing a token whose initialization failed leaves its fields alone.
The token keeps its copy of the user PIN for its whole lifetime. PKCS#11 keeps the login state per application for as long as one of its sessions is open, and every operation runs on the session that wc_Pkcs11Token_Open opened. Once that session is logged in, an application may not want the PIN kept in memory. wc_Pkcs11Token_Open_ex takes a discardPin flag. With it set, the PIN copy is zeroized and freed once the call has opened the session and logged it in, whether its own C_Login succeeded or reported CKR_USER_ALREADY_LOGGED_IN because another token of the application logged in first. Either way the login lasts while a session of the application is open, since nothing calls C_Logout any more. A pinDropped bit in Pkcs11Token then makes another open return BAD_STATE_E, rather than a session that is not logged in, until the token is finalized and initialized again with the PIN. Operations that open their own session, as after wc_Pkcs11Token_Close, still do so but skip C_Login. The application stays logged in while any of its sessions is open, as wolfPKCS11's per-call session interop test relies on, and with none open their private key operations fail with WC_HW_E. wc_Pkcs11Token_Final clears the bit along with the PIN. A session that was already open, possibly one the application set up itself, keeps the PIN, as wolfSSL did not log it in. wc_Pkcs11Token_Open keeps the PIN, so a token can still be closed and opened again, and operations after wc_Pkcs11Token_Close log in on their own session as before. wc_Pkcs11Token_Open also stored the session handle when opening or logging in failed. A failed C_Login left the session open, and a retry reused it without logging in. The session is now closed when its login fails and the handle is stored only on success. Calling the function again while a session is open does nothing, and the session keeps its read/write mode.
8c69fe6 to
abdc5f5
Compare
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| static void Pkcs11Token_ClearPin(Pkcs11Token* token) | ||
| { | ||
| if (token->userPin != NULL && token->pinOwned) { | ||
| ForceZero(token->userPin, (word32)token->userPinSz); | ||
| XFREE(token->userPin, token->heap, DYNAMIC_TYPE_TMP_BUFFER); | ||
| } |
There was a problem hiding this comment.
🔴 Applications that init a token with a PIN and later point userPin at their own buffer now get that buffer freed by Final, while the copy leaks. Pkcs11Token_ClearPin frees token->userPin whenever pinOwned is set. The flag stays set after Init even if the public userPin field is reassigned. Fix: free only the buffer the token allocated, e.g. keep the owned copy in its own field and free that, never whatever userPin points to. A pointer the application assigns would then only be forgotten, as the PR's NoLogin test expects.
Why this was flagged
This assumes an application calls wc_Pkcs11Token_Init or wc_Pkcs11Token_InitName with a PIN and later assigns token.userPin/userPinSz itself, for example after changing the PIN. userPin is a public field in wolfssl/wolfcrypt/wc_pkcs11.h, and no in-repo caller rules this out. Pkcs11Token_SetPin (wc_pkcs11.c:985-993) sets pinOwned = 1, and nothing clears it when the field is reassigned. wc_Pkcs11Token_Final then calls Pkcs11Token_ClearPin (wc_pkcs11.c:1118-1120), which runs ForceZero and XFREE on the application's pointer. A string literal or static buffer crashes. A heap buffer is double-freed when the application frees it later. The token's own PIN copy leaks without being zeroized. On the base branch Final only ran ForceZero on whatever userPin pointed to and freed nothing. The PR's 'PIN the application set itself' test (test_pkcs11.c:1566-1573) covers only Init_NoLogin, where pinOwned is 0.
Verification: Pkcs11Token_SetPin (wc_pkcs11.c:985-993) sets token->pinOwned = 1. No code clears pinOwned when the public field is reassigned. Pkcs11Token_ClearPin (wc_pkcs11.c:1118-1120) then frees the application's buffer from wc_Pkcs11Token_Final, and the token's copy leaks. On the base, Final never freed it. tests/api/test_pkcs11.c:1567-1574 covers only the Init_NoLogin path.
| if (userPin != NULL && userPinSz < 0) | ||
| ret = BAD_FUNC_ARG; | ||
| else | ||
| ret = Pkcs11Token_Init(token, dev, slotId, tokenName, tokenNameSz); | ||
| if (ret == 0 && userPin != NULL) | ||
| ret = Pkcs11Token_SetPin(token, userPin, userPinSz); |
There was a problem hiding this comment.
🟡 (optional) Applications that call Init again on a token that still holds a PIN copy now leak the old copy without zeroizing it. One example is retrying after a wrong PIN without calling Final. Pkcs11Token_Init resets userPin to NULL before Pkcs11Token_SetPin allocates a new copy, so the previous owned buffer is lost. Fix: document that wc_Pkcs11Token_Final must come before any re-init, since Init cannot tell an initialized token from stack garbage. Otherwise give callers a way to change the PIN that clears the old copy first.
Why this was flagged
This assumes an external application re-initializes a token with wc_Pkcs11Token_Init or wc_Pkcs11Token_InitName without calling wc_Pkcs11Token_Final first. Retrying login with a corrected PIN after WC_HW_E is one case. The only in-repo callers are tests, so the outside population cannot be ruled out. Pkcs11Token_Init overwrites token->userPin = NULL_PTR and pinOwned = 0 (wc_pkcs11.c:957-961). Pkcs11Token_SetPin then XMALLOCs a fresh copy (wc_pkcs11.c:985), so the earlier copy is never freed or wiped. Each retry leaves a plaintext PIN in leaked heap memory. On the base branch re-init only replaced a pointer to the caller's own buffer, so nothing was allocated and nothing leaked. No safeguard applies: Init has no way to free the previous copy, and the docs (doc/dox_comments/header_files/pkcs11.h:14-16) say only that Final frees the copy.
Verification: Pkcs11Token_Init (wolfcrypt/src/wc_pkcs11.c:957-963) writes token->userPin = NULL_PTR; without looking at the old values. Pkcs11Token_SetPin then does token->userPin = (CK_UTF8CHAR_PTR)XMALLOC(...), so the buffer allocated by the earlier Init is never freed, and its PIN bytes are never wiped. On the base, Init only stored the caller's pointer, so re-initializing without Final leaked nothing.
| if (token != NULL && token->func != NULL) { | ||
| token->func->C_CloseAllSessions(token->slotId); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: Finalizing one token still kills every other token's session on the same slot, so the PR's shared-login goal breaks on Final. wc_Pkcs11Token_Final calls C_CloseAllSessions(token->slotId) (wc_pkcs11.c:1137). That closes all of the application's sessions on the slot, including another token's open handle, and drops the shared login. Fix: in Final, close only this token's own session (as wc_Pkcs11Token_Close does via Pkcs11CloseSession) rather than every session on the slot, so other tokens keep their sessions and login.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
Two tokens A and B are initialised on the same slot. B calls wc_Pkcs11Token_Open and holds a logged-in session in B.handle. A then calls wc_Pkcs11Token_Final. At wc_pkcs11.c:1137 that calls token->func->C_CloseAllSessions(token->slotId), which by the PKCS#11 spec closes every session the application has on that slot. B.handle is left pointing at a closed session. B's next operation reuses it through Pkcs11OpenSession (token->handle != NULL_PTR) and gets CKR_SESSION_HANDLE_INVALID, which surfaces as WC_HW_E. Closing the last session also logs the application out. The base branch behaves the same way. But this PR removes C_Logout from Pkcs11CloseSession and accepts CKR_USER_ALREADY_LOGGED_IN specifically so that tokens sharing a slot do not disrupt each other, and Final is the remaining path that still does. The diff edits Final itself but keeps this call.
Verification: Triggers when two Pkcs11Token objects on the same slot are in use and one is finalized while the other still holds an open session. wc_Pkcs11Token_Final (wc_pkcs11.c:1134-1141) still calls C_CloseAllSessions at line 1137. Under PKCS#11, C_CloseAllSessions closes every session the application has on that slot. The base already fails this way, so this is pre-existing, not introduced.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #12111
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-src, wolfssl-bugs
Not reviewed (no scan target covers them): doc/dox_comments/header_files/pkcs11.h, tests/api/test_pkcs11.h
Coverage: 3 of 4 in-scope changed file(s) opened by the reviewer; not opened: wolfssl/wolfcrypt/pkcs11.h
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: Max
Description
This PR lets several
Pkcs11Tokens of one application use the same slot without logging each other out, and stops the token from keeping a pointer to the caller's PIN.C_Logout, andCKR_USER_ALREADY_LOGGED_INcounts as a successful login, so closing one token's session leaves another's login in place.wc_Pkcs11Token_Initandwc_Pkcs11Token_InitNamecopy the PIN into token-owned heap memory, whichwc_Pkcs11Token_Finalzeroizes and frees; a PIN in read-only memory no longer crashes on finalize.wc_Pkcs11Token_Open_ex(token, readWrite, discardPin)can discard the PIN once its session is logged in.wc_Pkcs11Token_Openkeeps the PIN, so close and reopen work as before.C_Loginfails is closed, and its handle is not kept.Behavior changes
wc_Pkcs11Token_Closeand per-operation sessions no longer log out.wc_Pkcs11Token_Finalno longer zeroizes the caller's PIN buffer.Pkcs11Tokengainsheap,pinOwnedandpinDropped.Testing
test_wc_Pkcs11_LoginShared: shared login, failed login closing its session, operations after close with and without the PIN.test_wc_Pkcs11_TokenPinCopy: PIN copy, empty and negative lengths, a PIN the application assigned itself.test_wc_Pkcs11_TokenOpenDropsPin:OpenversusOpen_exwithdiscardPin, reopen, failed login, already open session.Merging
Adds tests next to #11876 in
tests/api/test_pkcs11.c; whichever merges second needs a small rebase there.