fix: log out token on C_CloseAllSessions#204
Open
MarkAtwood wants to merge 2 commits into
Open
Conversation
WP11_Slot_CloseSessions closed every session for the slot but never reset the token login state, so a session opened after C_CloseAllSessions still reported CKS_RW_USER_FUNCTIONS instead of CKS_RW_PUBLIC_SESSION. Per PKCS#11, closing all of a slot's sessions logs the application out of the token. Call WP11_Slot_Logout after the sessions are closed, mirroring the single-session WP11_Slot_CloseSession path (which already logs out when no in-use sessions remain). WP11_Slot_Logout takes the slot lock itself and is a no-op when the token is already public, so it is called after the walk. Add tests/close_all_sessions_logout_test.c: log in CKU_USER, call C_CloseAllSessions, reopen a session and assert it is CKS_RW_PUBLIC_SESSION. The test fails against the pre-fix library (reopened session reports state 3 instead of 2).
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aligns C_CloseAllSessions behavior with PKCS#11 semantics by ensuring that closing all sessions for a slot also resets the token login state back to public, and adds a regression test to prevent the issue from recurring.
Changes:
- Call
WP11_Slot_Logout(slot)at the end ofWP11_Slot_CloseSessionsto reset the token to the logged-out (public) state. - Add a new regression test that logs in, calls
C_CloseAllSessions, reopens a session, and verifies the session is public. - Wire the new test into the Automake test build in
tests/include.am.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/include.am | Registers the new regression test target in the test build/run lists. |
| tests/close_all_sessions_logout_test.c | New regression test that asserts C_CloseAllSessions returns the token/session to public state. |
| src/internal.c | Adds the missing logout call after bulk session close to match PKCS#11 logout semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+189
to
+191
| funcList->C_Logout(session); | ||
| funcList->C_CloseSession(session); | ||
| session = 0; |
Contributor
Author
There was a problem hiding this comment.
Fixed in 58124d1 — both calls now go through CHECK_RV and the test bails to cleanup if either fails.
Comment on lines
+208
to
+211
| rv = funcList->C_CloseAllSessions(slot); | ||
| CHECK_RV(rv, "C_CloseAllSessions", CKR_OK); | ||
| session = 0; | ||
|
|
Contributor
Author
There was a problem hiding this comment.
Fixed in 58124d1 — the handle is now only zeroed when C_CloseAllSessions returns CKR_OK, so the cleanup path closes it on failure.
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.
Problem
C_CloseAllSessionsdoes not log the application out of the token.WP11_Slot_CloseSessionscloses every session for the slot but never resets the token login state, so a session opened afterC_CloseAllSessionsstill reportsCKS_RW_USER_FUNCTIONSinstead ofCKS_RW_PUBLIC_SESSION.Per PKCS#11 (v2.40 / v3.0), closing all of a slot's sessions returns the token to the logged-out state. The single-session path
WP11_Slot_CloseSessionalready does this — it callsWP11_Slot_Logoutonce no in-use sessions remain — but the bulk path was missing the equivalent call.Fix
Call
WP11_Slot_Logout(slot)inWP11_Slot_CloseSessionsafter all sessions are closed, mirroring the single-session path.WP11_Slot_Logoutacquires the slot lock itself (so it runs after the walk's unlock) and is a no-op when the token is already public.Test
Adds
tests/close_all_sessions_logout_test.c: initialize a token, set a user PIN, log inCKU_USER, callC_CloseAllSessions, reopen a session and assertC_GetSessionInforeportsCKS_RW_PUBLIC_SESSION.make checksuite is green.RW_USER_FUNCTIONSinstead of 2RW_PUBLIC_SESSION), so it genuinely gates the regression.Verified by build + run on Ubuntu 24.04.