Do not remove sessions in C_GetSessionInfo if card is logged out #2740
Conversation
This sounds very similar to PIV and which requires the pin (same one as login) to be entered before using the signature key. (The card enforces this - the crypt apdu must be preceded by the pin verify apdu.) You might be able to use the feature:
|
|
Currently we are only doing presence detection in C_WaitForSlotEvent, C_Initalize, C_WaitForSlotEvent, C_GetSlotInfo (via |
The GetSessionInfo is usually the first call the application does. Either straight away before login, after login or after some idle period, to verify the session is still usable before trying to do some operation on the module. The original problem and problem solved here is the long-running login session, used by various client applications, including OpenSSH's ssh-agent, but probably also Firefox/NSS (but these have another tread monitoring the card with
In case of PIV, I think the signature PIN is always the same as the login pin, therefore the PIV presents all the objects in one slot and enforces the "always authenticate" behavior (and checks the login status through a discovery object). The IDPrime aligns more with the pkcs15 definition, where different keys are accessible with different PINs, which in PKCS#11 maps different PINs to different slots, surfacing this problem (after one signature operation, the login status is lost which is causing invalidation of all sessions, which is obviously wrong). The summary is that this is mostly attempt to revisit changes made in 4bd8cda which were incomplete and were looking into the login statues instead of to the card/reader status. The fixups 8ab39bd and c2e00e9 were trying to fix it for various other use cases, but they still kept looking into the login status instead of checking the real trigger for resetting all sessions -- reader status. |
src/libopensc/reader-pcsc.c
Outdated
| if (priv->reader_state.dwEventState & SCARD_STATE_PRESENT && !(reader->flags & SC_READER_CARD_PRESENT)) | ||
| reader->flags |= SC_READER_CARD_CHANGED; |
There was a problem hiding this comment.
I think this part deserves a comment in the code why it is needed to be done this way, probably updating the above comment.
Also I am not sure if this can have any side effects. As far as I remember, the timeout from SCardGetStatusChange() returns when there was no change made to the reader since the last call. So this condition roughly to the case when the card is now present, but we believe previously it was not present (which sounds like the SCardGetStatusChange() was called in between the reader was removed and inserted, but I would have to try myself.
There was a problem hiding this comment.
Good point, I will update the comment.
Regarding the SCardGetStatusChange(), when no change happens, the SCARD_E_TIMEOUT is returned. Then, when reader is removed, next call returns SCARD_E_UNKNOWN_READER and SC_READER_CARD_PRESENT is removed from flags (but no SC_READER_CARD_CHANGED is set though). When reader is then inserted back again, the return code is again SCARD_E_TIMEOUT.
There was a problem hiding this comment.
The comment above (and the change to refresh_attributes) is somewhat confusing. If a reader is removed, then it should be treated as removed. If it is inserted again, it should be treated like a new reader, because all card state is lost and additionally you can't be sure whether this is really the very same card that's inserted.
For me this seems like SCARD_E_UNKNOWN_READER is not treated correctly. This change may help (note that the first change actually fixes a check to SC_READER_REMOVED as well):
diff --git a/src/libopensc/reader-pcsc.c b/src/libopensc/reader-pcsc.c
index 04d5ac8f..8c2f98c9 100644
--- a/src/libopensc/reader-pcsc.c
+++ b/src/libopensc/reader-pcsc.c
@@ -364,7 +364,7 @@ static int refresh_attributes(sc_reader_t *reader)
if (reader->ctx->flags & SC_CTX_FLAG_TERMINATE)
return SC_ERROR_NOT_ALLOWED;
- if (priv->reader_state.szReader == NULL || reader->ctx->flags & SC_READER_REMOVED) {
+ if (priv->reader_state.szReader == NULL || reader->flags & SC_READER_REMOVED) {
priv->reader_state.szReader = reader->name;
priv->reader_state.dwCurrentState = SCARD_STATE_UNAWARE;
priv->reader_state.dwEventState = SCARD_STATE_UNAWARE;
@@ -394,6 +394,8 @@ static int refresh_attributes(sc_reader_t *reader)
#endif
|| rv == (LONG)SCARD_E_SERVICE_STOPPED) {
reader->flags &= ~(SC_READER_CARD_PRESENT);
+ reader->flags |= SC_READER_REMOVED;
+ priv->gpriv->removed_reader = reader;
SC_FUNC_RETURN(reader->ctx, SC_LOG_DEBUG_VERBOSE, SC_SUCCESS);
}
|
Sorry for a long post. Looking into this issue again, I think there might be one more issue that I did not notice before. When applications check for the slot/card presence, they usually use a sequence of
This basically goes through the card detection needlessly twice. It can also be racy as if the slots change in between, we might get unexpected I tried again to reproduce the issue with #1822 (comment) with this branch, but it fails spectacularly. Most probably related to 7b782d5. After rebasing on current master it prompts for the pin correctly, but after reinsertion, I am asked for the PIN twice:
I do not see the invalidation of all the sessions as expected right after the reinsertion. Debug logs from this attempt:
From another thread it looks like the NSS checks the validity of the key by calling this notices the state change, but still present. Then we skip the newly introduced condition in pkcs11-session.c and go to checking PIN info, while we discovered the card was changed and we should have killed the existing sessions instead of trying resurrect them somehow. The pin info kind of worked The NSS calls Then we go to open session & Sign attempt where init works, but later we find out we are not logged in: Up until now, there was no APDU call that could detect the login state was reset in the yubikey (might be limitation of my old yubikey, the PIV driver), but the change state from the pcsc is ignored, which is the main cause, I think. During the second attempt, it looks like it behaves as expected (when I waited 5-10 seconds before reinserting the token), which means that this is still racy -- depending on if we will get the From the second attempt, I see the following list of events:
So this looks like the current version still needs some love.
|
|
Test server:
|
src/libopensc/reader-pcsc.c
Outdated
| if (priv->reader_state.dwEventState & SCARD_STATE_PRESENT && !(reader->flags & SC_READER_CARD_PRESENT)) | ||
| reader->flags |= SC_READER_CARD_CHANGED; |
There was a problem hiding this comment.
The comment above (and the change to refresh_attributes) is somewhat confusing. If a reader is removed, then it should be treated as removed. If it is inserted again, it should be treated like a new reader, because all card state is lost and additionally you can't be sure whether this is really the very same card that's inserted.
For me this seems like SCARD_E_UNKNOWN_READER is not treated correctly. This change may help (note that the first change actually fixes a check to SC_READER_REMOVED as well):
diff --git a/src/libopensc/reader-pcsc.c b/src/libopensc/reader-pcsc.c
index 04d5ac8f..8c2f98c9 100644
--- a/src/libopensc/reader-pcsc.c
+++ b/src/libopensc/reader-pcsc.c
@@ -364,7 +364,7 @@ static int refresh_attributes(sc_reader_t *reader)
if (reader->ctx->flags & SC_CTX_FLAG_TERMINATE)
return SC_ERROR_NOT_ALLOWED;
- if (priv->reader_state.szReader == NULL || reader->ctx->flags & SC_READER_REMOVED) {
+ if (priv->reader_state.szReader == NULL || reader->flags & SC_READER_REMOVED) {
priv->reader_state.szReader = reader->name;
priv->reader_state.dwCurrentState = SCARD_STATE_UNAWARE;
priv->reader_state.dwEventState = SCARD_STATE_UNAWARE;
@@ -394,6 +394,8 @@ static int refresh_attributes(sc_reader_t *reader)
#endif
|| rv == (LONG)SCARD_E_SERVICE_STOPPED) {
reader->flags &= ~(SC_READER_CARD_PRESENT);
+ reader->flags |= SC_READER_REMOVED;
+ priv->gpriv->removed_reader = reader;
SC_FUNC_RETURN(reader->ctx, SC_LOG_DEBUG_VERBOSE, SC_SUCCESS);
}
|
I still remember that I needed several iterations of #1923, primarily due to the different layers of state keeping (and swallowing). I think, if
No, I don't think we need to overengineer this. As you've pointed out, the worst thing that could happen is In any case, PC/SC changes need to be tested on macOS and Windows as well. |
By using sc_detect_card_presence() function and observing status flags.
Do not delete all sessions, when the card was logged out during operation. Instead, check whether the card was changed and by logging out only set correct flag.
|
I tried the changes introduced in #2740 (comment) regarding the |
frankmorgner
left a comment
frankmorgner
left a comment
There was a problem hiding this comment.
I think the change on the PC/SC level looks simple enough to assume that it will also work on macOS and Windows without in-depth-testing.
src/pkcs11/framework-pkcs15.c
Outdated
| out: | ||
| return 0; |
There was a problem hiding this comment.
The goto/label here is not even saving any repeated code as it contains only one return. I would probably propose to simplify this to call return 0 directly in case of errors.
Short doc text explaining at least what is the meaning of the returned values would also help future readers.
|
It looks like I managed to get the Now I waited couple of seconds before the reinsertion, but pulled out and removed between two blinks of the yubikey (which is probably initiated by the periodic check by NSS). The OpenSC debug log looks like previously -- there is no sign from the pcsc that there was a change on of the reader. So I decided to have a better look if this is something we could detect from the pcsc-spy logs. The But from what I did not notice in the OpenSC logs, there is one call where After this, opensc reconnects automatically: but still considers the card/reader good and does not invalidate the state, which is something I am not completely sure it is correct. This is handled in So my question would be if the following block should handle also invalidation of the whole card structures or if this code path is likely to be hit also in some other use cases: Lines 478 to 484 in 493a3d4 @LudovicRousseau can you have a look? Should |
|
@Jakuje Yes, This is because the same PC/SC reader name is reused. For example I have this reader name 'Gemalto PC Twin Reader 00 00'. If I remove and reinsert the reader I will get the same reader name. So the second call to Also if the reader is removed before |
Thanks for checking!
Is it a feature or a bug? Given that the reader changed even though it has the same name, i think the changed flag should be raised. If there is a technical/specification reason why it should not, I think this corner case should be documented. If one has two readers/tokens with same reader name, the new reader/token might be something completely different. But I still have a feeling that we should probably invalidate the reader when we get |
|
@Jakuje I checked on Windows and I have the same behavior than with pcsc-lite. The same PC/SC name is reused if I remove and re-insert a reader. The SCARD_READERSTATE structure used by SCardGetStatusChange() has a
I used this sample code: from smartcard.scard import *
from smartcard.pcsc.PCSCExceptions import *
import sys
hresult, hcontext = SCardEstablishContext(SCARD_SCOPE_USER)
if hresult != SCARD_S_SUCCESS:
raise EstablishContextException(hresult)
hresult, readers = SCardListReaders(hcontext, [])
if hresult != SCARD_S_SUCCESS:
raise ListReadersException(hresult)
print('PC/SC Readers:', readers)
readerstates = {}
for reader in readers:
readerstates[reader] = (reader, SCARD_STATE_UNAWARE)
hresult, newstates = SCardGetStatusChange(hcontext, 0,
list(readerstates.values()))
print("SCardGetStatusChange:", SCardGetErrorMessage(hresult))
for readerState in newstates:
readername, eventstate, atr = readerState
print("reader: {}\n state: {}\n ATR: {}".format(readername,
hex(eventstate), atr))
readerstates[readername] = (readername, eventstate)
print("Remove reader and press enter")
sys.stdin.read(1)
hresult, newstates = SCardGetStatusChange(hcontext, 10,
list(readerstates.values()))
print("SCardGetStatusChange:", SCardGetErrorMessage(hresult))
for readerState in newstates:
readername, eventstate, atr = readerState
print("reader: {}\n state: {}\n ATR: {}".format(readername,
hex(eventstate), atr))
readerstates[readername] = (readername, eventstate)
hresult = SCardReleaseContext(hcontext)
print("SCardReleaseContext:", SCardGetErrorMessage(hresult))scenario:
I get: Note that the field if I do not remove the reader I get: the state changes from 0x40022 to 0x40020. So the program can know if a card has been removed (or not) between 2 The results of my Python sample code are a bit different under Windows (otherwise it is not funny). See also
|
|
Any idea why in my pcsc spy trace (excerpt in the previous comment #2740 (comment)) nor opensc traces above do have all the high bits zeroed? I have When I run your python script, I am also getting the high bytes empty: Regardless if I use my yubikey or gemalto reader |
|
The I run the script again and I do not see the high bytes increase any more :-( In fact it was a bug in pcsc-lite I just fixed in LudovicRousseau/PCSC@92080dc. The event counter was not reset when a reader is inserted. Oups! I now get, with a reader removal/reinsertion: The event counter is reset to 0 |
|
oh, card event. Missed that. That explains it. So this wont be any use for tokens without removable cards. |
|
In any case, if the application has a connection to the card/token (with In your case you have: OpenSC should then consider the card has been removed. I agree the error message is not very informative. I note that, on Windows, the |
This is a difficult question. The easiest thing one could do to signal this problem beyond the return code of pcsc_lock would be to set SC_READER_REMOVED. This would be removed in the next run of pcsc_detect_readers or pcsc_wait_for_event (but not in plain refresh_attributes/pcsc_detect_card_presence. At least in PKCS#11, SC_READER_REMOVED would result in a full reinitialization of the reader/card. In minidriver this can be ignored, because the PCSC events are handled by the OS. In CTK this can be ignored as well, I think, because the OS handles reader events as well (and initializes or tears down the token driver i.e. OpenSCToken). Although this should work, it creates a logical gap since we are reconnecting to and actively using the reader/card even though we are marking it as removed. From a logical point of view, I would choose to avoid automatic reconnection on the lower level (i.e. in sc_lock or pcsc_lock), but this kind of behavior has been used for decades and it touches every interaction with the card, so I am be a little unsettled about "fixing" this now.. |
|
My proposal would be to merge the current code and move the outstanding issue into separate issue/PR if we will get to fix that. I think the current version catches the most common cases and if we would like to catch also the last one for short-lived removals, it should be a stretch goal. What do you think? |
|
Ok, thank you! |
5 participants
In
C_GetSessionInfo, the existing session should be removed when the card or reader was removed/has changed and not only by logging out of the card.When checking the functionality of #2666, it was found that IDPrime 940 works with two PINs - default PIN and signature PIN. For signature PIN, it holds that we must log into the card before every operation with a private key, and the card then logs out after every such operation. The
pkcs11-tooltest revealed that this behaviour might cause problems when after such a sensitive operationC_GetSessionInfo()is called since it checks whether the card is in the logged-in state, and if not, it removes all existing sessions.Removing of sessions in
C_GetSessionInfowas introduced by #1875.Changes were tested with IDPrime 940 (rebased with changes from #2666) and Yubikey 5.
Checklist