1613632 - Stuck on invalidated PKCS#11 session handles

Status: UNCONFIRMED

(NSS :: Libraries, defect, P3)

Description

[frankmorgner]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:72.0) Gecko/20100101 Firefox/72.0

Steps to reproduce:

In NSS there is a problem with handling CKR_SESSION_HANDLE_INVALID as result of some PKCS#11 operation. When NSS opens a session and asks for the password, then CKR_SESSION_HANDLE_INVALID as result of C_Login correctly sets the internal state to CK_INVALID_SESSION. However, the opened (and invalidated) session is still used for further operation with the PKCS#11 module. This results in (a potentially infinite) number of password prompts although a token has long been removed.

Steps to reproduce:

1. Configure PKCS#11 module in Firefox
2. Insert token (it's shown in "Security Devices" as "not logged in")
3. Go to a Website that requests a client certificate and wait for the password prompt for the token
4. Remove the token
5. Enter a password or abort it to close the password prompt
6. Go to a Website that requests a client certificate and wait for the password prompt for the token
7. Go to step 5. and repeat.

This bug reproduces with Firefox 72, OpenSC and a Yubikey, for example. After Step 4, the slot for the token still exists and the token is correctly shown as "not present" in the list of "security devices". However, the open (but invalid) session is never closed and PK11_Authenticate constantly tries to use the session to verify a password.

Note, that in Step 4, the PKCS#11 module is forced to invalidate all existing PKCS#11 sessions to a slot:

(quote PKCS#11 v2.30 "Session events":)
When the device is removed, all sessions of all applications are automatically logged out. Furthermore, all sessions any applications have with the device are closed (this latter behavior was not present in Version 1.0 of Cryptoki)"an application cannot have a session with a token that is not present. Realistically, Cryptoki may not be constantly monitoring whether or not the token is present, and so the token's absence could conceivably not be noticed until a Cryptoki function is executed. If the token is re-inserted into the slot before that, Cryptoki might never know that it was missing.
(quote end)

Hence, all further operation with existing sessions to a removed token result in CKR_SESSION_HANDLE_INVALID

Actual results:

Depending on the number of open TLS session, the user cannot get rid of constantly popping password prompts and is forced to close Firefox.

Expected results:

If the PKCS#11 module returns CKR_SESSION_HANDLE_INVALID, NSS needs to close the session and re-open a new one.

Comment 1

[J.C. Jones [:jcj] (he/they)]

Not to totally pass the buck here, but is it possible that you're affected by this OpenSC bug? https://github.com/OpenSC/OpenSC/issues/1822

Comment 2

[frankmorgner]

Nope, I'm running the latest development version of OpenSC, which contains a fix to the bug that you've referenced.

If you wish, I could try to generate a log of the PKCS#11 calls.

Comment 3

[frankmorgner]

Attachment: PKCS#11 Log

Comment 4

[frankmorgner]

I have attached a log file.

As you can see, the session 0x7efd1a5f1d60 is reused for C_GetSessionInfo (call 64) although the session has been closed with C_CloseSession (call 60). C_GetSessionInfo is called repeatedly thereafter whenever I want to access that website (even though the token has been removed or re-inserted)

Comment 5

[deengert]

Attachment: annotated-reader-removed-nspr_log_file.txt

Comment 6

[deengert]

The problem is not really with CKR_SESSION_HANDLE_INVALID or CK_INVALID_HANDLE.

The underlying problem appears to be the comment in:
https://github.com/mozilla/gecko-dev/blob/master/security/nss/lib/pk11wrap/pk11util.c#L996
says: "PKCS 11 v2.20 allows for modules to add new slots, but never remove them."

https://www.cryptsoft.com/pkcs11doc/STANDARD/pkcs-11v2-20.pdf
page 107 says: "If an application calls C_GetSlotList with a non-NULL pSlotList,
and then the user adds or removes a hardware device, the changed slot list will only be
visible and effective if C_GetSlotList is called again with NULL."

That is what SECMOD_UpdateSlotList is doing, requesting an updated slot list to see if slots have been added. But it does not handled removed slots or the special case when the slot list count == 0.

With Yubikey (and any other token with built in reader) removing the token removes the reader too. (OpenSC does not remove a slot when just a card is removed from a reader) When the reader is removed, the slot is also removed, and C_GetSlotList will return a new slot list without that reader, as pkcs-11v2-20 says it can. So the problem only shows if the reader and token are removed together.

See the annotated-reader-removed-nspr_log_file.txt for step by step failure analysis. This was done using FireFox 72.0.2 on Ubuntu-18.04 running under VirtualBox with Windows 10 as host. One reader with a PIV card was used to access an OpenSSL s_server with client authentication which works. If just the card is removed, I can cancel the PIN prompt or the selection of a certificate. If the reader is removed with the card still in it, (simulating a Yubikey)

The problem really starts with https://github.com/mozilla/gecko-dev/blob/master/security/nss/lib/pk11wrap/pk11util.c#L1032
There should be no assumption that just because the number of slots has not changed, there are no changes.

It is also complicated if a PKCS11 module reassigns CK_SLOT_IDs between calls to C_GetSlotList to get a new slot list. One would expect that unchanged slots would return the same CK_SLOT_ID each time. As an OpenSC developer, I am looking at this problem. With only one reader this is not a problem with OpenSC today.

P11kit or other PKCS11 modules may have similar issues.

Comment 7

[deengert]

https://github.com/OpenSC/OpenSC/issues/1822#issuecomment-544461489(In reply to J.C. Jones [:jcj] (he/him) from comment #1)

Not to totally pass the buck here, but is it possible that you're affected by this OpenSC bug? https://github.com/OpenSC/OpenSC/issues/1822

Probably not, because the https://github.com/OpenSC/OpenSC/pull/1875 fixed that problem, or at least move the problem further along, to where the SECMOD_UpdateSlotList does not handled removed slots or the special case when the slot list count == 0.

Comment 8

[frankmorgner]

I modified OpenSC to never remove any slot, just to say that the token disappeared from the slot (removing CKF_TOKEN_PRESENT from the slot's flags). Unfortunately, that doesn't work around the original problem. The problem still persists by using the using the steps for reproducing in the original post.

So we actually have two problems here:

1. NSS fails tracking a slot's state when a card disappears
2. NSS can't cope with a shrinking number of slots as described above

Here's a patch for OpenSC to reproduce the original problem with a static slot list.

diff --git a/src/pkcs11/pkcs11-global.c b/src/pkcs11/pkcs11-global.c
index 71e13831..f5239f11 100644
--- a/src/pkcs11/pkcs11-global.c
+++ b/src/pkcs11/pkcs11-global.c
@@ -488,21 +488,6 @@ CK_RV C_GetSlotList(CK_BBOOL       tokenPresent,  /* only slots with token prese
                prev_reader = slot->reader;
        }
 
-       /* Slot list can only change in v2.20 */
-       if (pSlotList == NULL_PTR) {
-               /* slot->id is derived from its location in the list virtual_slots.
-                * When the slot list changes, so does slot->id, so we reindex the
-                * slots here the same way it is done in `create_slot()`
-                *
-                * TODO use a persistent CK_SLOT_ID, e.g. by using something like
-                * `slot->id = sc_crc32(slot, sizeof *slot);` (this example, however,
-                * is currently not thread safe).  */
-               for (i=0; i<list_size(&virtual_slots); i++) {
-                       slot = (sc_pkcs11_slot_t *) list_get_at(&virtual_slots, i);
-                       slot->id = (CK_SLOT_ID) list_locate(&virtual_slots, slot);
-               }
-       }
-
        if (pSlotList == NULL_PTR) {
                sc_log(context, "was only a size inquiry (%lu)\n", numMatches);
                *pulCount = numMatches;
diff --git a/src/pkcs11/slot.c b/src/pkcs11/slot.c
index 423457b4..d4aca5ba 100644
--- a/src/pkcs11/slot.c
+++ b/src/pkcs11/slot.c
@@ -133,18 +133,11 @@ CK_RV create_slot(sc_reader_t *reader)
 void empty_slot(struct sc_pkcs11_slot *slot)
 {
        if (slot) {
-               if (slot->flags & SC_PKCS11_SLOT_FLAG_SEEN) {
                        /* Keep the slot visible to the application. The slot's state has
                         * already been reset by `slot_token_removed()`, lists have been
                         * emptied. We replace the reader with a virtual hotplug slot. */
                        slot->reader = NULL;
                        init_slot_info(&slot->slot_info, NULL);
-               } else {
-                       list_destroy(&slot->objects);
-                       list_destroy(&slot->logins);
-                       list_delete(&virtual_slots, slot);
-                       free(slot);
-               }
        }
 }

Comment 9

[J.C. Jones [:jcj] (he/they)]

Daiki, is this something your team would have time to address in the near future? We're in the "review a patch, but probably not time right now to write one" stage on this one, though it's clearly a problem that needs addressing it.

Comment 10

[Robert Relyea]

Right NSS currently requires the 2.20 semantics. One way to handle this is to never remove a reader, even if it is removed from the system. pcsc-lite can handle this. You can look at coolkey to see how to maintain a reader in the same slot even if it has been removed. NSS can handle new slots being added, but not slots being removed.

Comment 11

[deengert]

Have a look at OpenSC PR: https://github.com/OpenSC/OpenSC/pull/1961 from 2 days ago to never remove a slot/reader because PC/SC can handle this by never removing a slot from the list.

But I respectfully disagree with the NSS comments that PKCS11 v2.20 semantics do not allow the list of slots to shrink.

PKCS11-v2.20 says: "Furthermore, the set of slots accessible through a Cryptoki library is checked at the time that C_GetSlotList, for list length prediction (NULL pSlotList argument) is called. If an application calls C_GetSlotList with a non-NULL pSlotList, and then the user adds or removes a hardware device, the changed slot list will only be visible and effective if C_GetSlotList is called again with NULL."

As I read this, it says the slots returned can change between calls to C_GetSlotList(NULL,...) and specifically says: "user ADDS or REMOVES a hardware device". the meaning of "hardware device" is not clear, it could be a reader or a smart card. In the case of a Yubico its both.

PKCS11 v2.11 says: "Furthermore, the set of slots accessible through a Cryptoki library is fixed at the time that C_Initialize is called."

The difference between v2.11 and v2.20 appears to be saying the slot list can changed and can grow or shrink.

Comment 12

[frankmorgner]

First of all, I'd like concur Doug: PKCS#11 2.20 explicitly supports a removal of a slot. Please read the standard again.

Second, in PKCS#11 2.11 (https://www.cryptsoft.com/pkcs11doc/STANDARD/pkcs-11v2-11r1.pdf) it's simply undefined whether slots can be removed. The standard only talks about adding a slot, which doesn't exclude a removal (which is something v2.20 made explicit later).

Third, if NSS assumes compatibility to some version of PKCS#11 (say v2.11, which is the last one your interpretation may be considered valid), NSS should check which version of PKCS#11 the module is compatible with. However, currently, NSS doesn't look at CK_INFO's cryptokiVersion.major/minor in any way to check the compatibility.

Fourth, assuming NSS's interpretation is correct (which it isn't), my patch above changes OpenSC's slot list to not shrink. Still, the original bug persists and NSS/Firefox is stuck on an invalid session handle!

Stop deflecting and please start actually looking at the problem.

Comment 13

[Robert Relyea]

I'm not saying you are wrong in your implementation. I am pointing out what NSS semantic has been for decades*, and how other modules have handle the issue. Handling slot removal is not trivial and will not likely happen anytime soon in NSS. Given that, I was meerly suggesting how you can work around the NSS restriction if you want. You can take that suggestion or ignore it.

I'm happy to keep this around as a long term goal for NSS, but the fact is it's not going to get much attention soon because inside NSS slots are term data structures. Having a slot change slotID or slot name will definitely cause issues.

All that being said, handling the simple case where a slot appears and then disappears may be doable (it might be that we can just mark the token 'removed' in NSS, which is what coolkey did with removed readers). You really do need to make sure you don't change the slot name on the slot ID. Since there is no way to detect removal or insertion of a different reader other than the change in the slot count, NSS will have to continue to use that method. If you have a suggestion on how that change could be signalled (even if it includes creating a new object type or new function), please propose it to the PKCS #11 OASIS working group at [e-mail].

*When NSS initially created the code, only additions were allowed to the slotlist. I believe the spec changed, but NSS's implementation (and thus requirement) did not.

Comment 14

[deengert]

I agree that a long term goal for NSS is to handle removed and added slots. I was pointing out that the comments in the NSS code say
v2.20 which is misleading.

Things have changed in the last few years and more and more tokens with built in readers are being used. This increases the problem.
It looks like in OpenSC-0.15.0 we started to delete the readers and remove the slots when a reader was removed. https://github.com/OpenSC/OpenSC/commits/9f318b829f

https://github.com/OpenSC/OpenSC/pull/1961 removes the code that deleted the slot . And keeps the slot around so if PC/SC sees the reader reinserted, the slot does not change and a token is marked as having been inserted.

I would like you and Frank to give it a try. I have tried it with 4 devices, a Yubikey, Smartcard-hsm both have built in readers, and two real readers with smart cards and removing and inserting in different combinations. Tested using Firefox 73.0 on Ubuntu-18.4 running under VirtualBox with a Windows 10 host. If a reader with a card or one of the tokens is remove and inserted into the same USB, port and the device is attached to the virtual host, it is assigned to the same slot as before. If into a different USB it will show up as a new slot with new slot Id.
The default for OpenSC is 4 readers, but that is a config option and can be set higher.

Comment 15

[frankmorgner]

Robert, your answer baffled me in many ways. Mozilla, home of modern software development, focusing on user needs, savior of the free internet, fails to fix a simple bug? And you justify this by the fact that this bug is present for ages? You think you can postpone this because your software design is too complex? That's sad an disappointing.

No, keeping the number of slots static does not solve the problem. You may get around the user being prompted for a PIN (if you abort the dialog often enough!), but Firefox still shows a non-existent token to the user, we're already getting support requests about Firefox wanting to access cached certificates although the token is removed https://github.com/OpenSC/OpenSC/issues/1908. The user experience is just terrible.

With your kind of attitude, it's no wonder that everyone's hope centers around FIDO2 and the completely new implementations for token support in browsers... (Note that the current implementations for FIDO2 are far from perfect and it looks like we're already repeating some of the mistakes from the past.)

Anyway, since you want to rest in peace, I'll no longer bother you.

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.