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

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Ryan Sleevi]

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]

Attachment: Flush the slot delay and cached info after removing a slot.

Comment 2

[Ryan Sleevi]

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]

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]

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]

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]

Attachment: As committed

Comment 7

[Ryan Sleevi]

https://hg.mozilla.org/projects/nss/rev/0114a0ae7ebe