Last Comment Bug 875601 - SECMOD_CloseUserDB / SECMOD_OpenUserDB fails to reset the token delay, leading to spurious failures
: SECMOD_CloseUserDB / SECMOD_OpenUserDB fails to reset the token delay, leadin...
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.14.2
: All All
: P2 normal (vote)
: 3.15.1
Assigned To: Ryan Sleevi
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-23 18:38 PDT by Ryan Sleevi
Modified: 2013-06-10 11:59 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Flush the slot delay and cached info after removing a slot. (983 bytes, patch)
2013-05-23 18:42 PDT, Ryan Sleevi
wtc: review+
rrelyea: feedback+
Details | Diff | Review
As committed (975 bytes, patch)
2013-06-10 11:58 PDT, Ryan Sleevi
ryan.sleevi: checked‑in+
Details | Diff | Review

Description Ryan Sleevi 2013-05-23 18:38:28 PDT
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.
Comment 1 Ryan Sleevi 2013-05-23 18:42:55 PDT
Created attachment 753578 [details] [diff] [review]
Flush the slot delay and cached info after removing a slot.
Comment 2 Ryan Sleevi 2013-05-23 18:49:13 PDT
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.
Comment 3 Ryan Sleevi 2013-05-24 13:59:34 PDT
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.
Comment 4 Wan-Teh Chang 2013-05-29 18:44:48 PDT
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.
Comment 5 Robert Relyea 2013-06-07 16:05:02 PDT
Comment on attachment 753578 [details] [diff] [review]
Flush the slot delay and cached info after removing a slot.

r+ rrelyea

bob
Comment 6 Ryan Sleevi 2013-06-10 11:58:37 PDT
Created attachment 760517 [details] [diff] [review]
As committed

Note You need to log in before you can comment on or make changes to this bug.