SECMOD_CloseUserDB / SECMOD_OpenUserDB fails to reset the token delay, leading to spurious failures

RESOLVED FIXED in 3.15.1

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Ryan Sleevi, Assigned: Ryan Sleevi)

Tracking

3.14.2
3.15.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
This was noticed in Chromium as https://code.google.com/p/chromium/issues/detail?id=238654

We have a set of unittests that use the PKCS#12 functions to import a certificate. Each unittests independently uses SECMOD_OpenUserDB at the start of the test, and SECMOD_CloseUserDB at the end of the test, as a way of isolating unit test state by importing into a temporary database.

When multiple databases are opened then closed in rapid succession, it's possible for the PKCS#12 importing to fail. This happens in PK11_KeyGenWithTemplate attempting to generate a key into a token that is no longer present.

The root issue was identified as being nssSlot_IsTokenPresent() [ https://hg.mozilla.org/projects/nss/annotate/c499082b9a15/lib/dev/devslot.c#l119 ] returning true after the SECMOD DB has been closed, due to the slot being within the token delay period [lines 135-137, and https://hg.mozilla.org/projects/nss/annotate/c499082b9a15/lib/dev/devslot.c#l102 ]

This in turn is triggered only when the PK11SlotInfo structure is re-used between calls to SECMOD_OpenUserDB/SECMOD_CloseUserDB. Because SECMOD_UpdateSlotList assumes that modules can never be removed, it always reuses the PK11SlotInfo's.

As part of Bug 588269, I updated SECMOD_OpenUserDB to properly reset the token delay period, via calls to nssSlot_ResetDelay() and PK11_IsPresent(), but it seems it's also necessary to update SECMOD_CloseUserDB with similar changes, so that STAN-and-friends know not to attempt to generate keys in a slot that is known to no longer be present.
(Assignee)

Comment 1

4 years ago
Created attachment 753578 [details] [diff] [review]
Flush the slot delay and cached info after removing a slot.
Attachment #753578 - Flags: review?(rrelyea)
(Assignee)

Comment 2

4 years ago
In case the description wasn't clear: (One of) the Issues that Bug 588269 fixed was that a "recycled" slot was not immediately usable, because things like the token delay were not updated in a timely manner.

This is a similar, but different problem, in that functions like PK11_GetBestSlotForMechanism return a slot that's been removed, and thus fail.

While the PKCS#12 code could be made more robust - such as attempting to get all the slots suitable for that mechanism, and attempting them - this is an interim (and working) fix, that makes sure we mark the slot as "removed" immediately after we KNOW it's been unloaded by the SECMOD functions.
(Assignee)

Comment 3

4 years ago
Comment on attachment 753578 [details] [diff] [review]
Flush the slot delay and cached info after removing a slot.

Forgot that Bob is out for a bit. Wan-Teh, would you mind taking a look at this - since you had commented on and helped review the previous change.
Attachment #753578 - Flags: review?(wtc)
Attachment #753578 - Flags: review?(rrelyea)
Attachment #753578 - Flags: feedback?(rrelyea)

Comment 4

4 years ago
Comment on attachment 753578 [details] [diff] [review]
Flush the slot delay and cached info after removing a slot.

Review of attachment 753578 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. Bob knows this code much better than I do, so
please don't cancel the review request for Bob.

The bug's summary says "SECMOD_CloseUserDB / SECMOD_OpenUserDB",
but this patch only fixes SECMOD_CloseUserDB. I just
wanted to make sure SECMOD_OpenUserDB was not overlooked.

::: lib/pk11wrap/pk11util.c
@@ +1496,5 @@
>      rv = secmod_UserDBOp(slot, CKO_NETSCAPE_DELSLOT, sendSpec);
>      PR_smprintf_free(sendSpec);
> +    /* if we are in the delay period for the "isPresent" call, reset
> +     * the delay since we know things have probably changed... */
> +    if (slot && slot->nssToken && slot->nssToken->slot) {

Delete "slot &&". We already derefenced |slot| a few
lines above, so |slot| cannot be null here.
Attachment #753578 - Flags: review?(wtc) → review+

Comment 5

4 years ago
Comment on attachment 753578 [details] [diff] [review]
Flush the slot delay and cached info after removing a slot.

r+ rrelyea

bob
Attachment #753578 - Flags: feedback?(rrelyea) → feedback+
(Assignee)

Comment 6

4 years ago
Created attachment 760517 [details] [diff] [review]
As committed
Attachment #753578 - Attachment is obsolete: true
Attachment #760517 - Flags: checked-in+
(Assignee)

Comment 7

4 years ago
https://hg.mozilla.org/projects/nss/rev/0114a0ae7ebe
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.