Closed Bug 875601 Opened 8 years ago Closed 8 years ago

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

Categories

(NSS :: Libraries, defect, P2)

3.14.2
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.15.1

People

(Reporter: ryan.sleevi, Assigned: ryan.sleevi)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attachment #753578 - Flags: review?(rrelyea)
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 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 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 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+
Attached patch As committedSplinter Review
Attachment #753578 - Attachment is obsolete: true
Attachment #760517 - Flags: checked-in+
https://hg.mozilla.org/projects/nss/rev/0114a0ae7ebe
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.