210660 - New PK11Context's adoption of the PK11SymKey's session is not thread-safe

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Wan-Teh Chang]

This bug was introduced in NSS 3.6 by the fix for bug
145322.

In pk11skey.c, when we create a new PK11Context in
pk11_CreateNewContextInSlot, we have:

     context->key = symKey ? PK11_ReferenceSymKey(symKey) : NULL;
     context->slot = PK11_ReferenceSlot(slot);
     if (symKey && symKey->sessionOwner) {
        /* The symkey owns a session.  Adopt that session. */
        context->session = symKey->session;
        context->ownSession = symKey->sessionOwner;
        symKey->sessionOwner = PR_FALSE;
     } else {
        context->session = pk11_GetNewSession(slot, &context->ownSession);
     }

The testing and setting of symKey->sessionOwner is
not thread-safe.  If two threads execute this code
at the same time, both of them may adopt that session,
which is incorrect.

Similarly, in PK11_DestroyContext, we have:

    if (context->ownSession && context->key && /* context owns session & key */
        context->key->session == context->session && /* sharing session */
        !context->key->sessionOwner)              /* sanity check */
    {
        /* session still valid, let the key free it as necessary */
        rv = PK11_Finalize(context); /* end any ongoing activity */
        if (rv == SECSuccess) {
            context->key->sessionOwner = PR_TRUE;
        } /* else couldn't finalize the session, close it */
    }

The testing and setting of context->key->sessionOwner is
not thread-safe.

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch: back out Ian's change

The easiest way to make PK11SymKey thread-safe is to
back out Ian's change (in bug 145322) that caused a
new context to adopt a sym key's session.

With this patch applied, a sym key's sessionOwner
field will never change throughout its lifetime,
except when PK11_SetWrapKey is called.	I added
comments to warn that PK11_SetWrapKey is not
thread-safe and verified that we are only calling
PK11_SetWrapKey when only one thread has a reference
to the sym key.

Unfortunately this patch will undo some of Ian's
saving of sessions, but the code must be correct.
I haven't been able to come up with a better patch
that preserves Ian's saving of sessions.  One idea
I have requires adding a new function: a function
that creates a sym key for single-threaded use.
Then a new context can adopt the session of a
single-threaded session safely.

Comment 2

[Wan-Teh Chang]

Comment on attachment 126489 [details] [diff] [review]
Proposed patch: back out Ian's change

Code reviewers please verify the following points:

1. Use LXR to verify that all the direct access to
   the sessionOwner field of PK11SymKey is in
   pk11skey.c.	Then you only need to look at
   pk11skey.c.

2. With the patch applied, we are only setting
   sessionOwner in two functions:
   - pk11_getKeyFromList
   - PK11_SetWrapKey
   In the other functions we are only reading
   sessionOwner.

3. Both of those functions are only called when
only one thread has a reference to the sym key.
(Use LXR to verify this.)

With the above, we can conclude that sessionOwner
doesn't change throughout the lifetime of a sym
key (with the exception of PK11_SetWrapKey).

Comment 3

[Jamie Nicolson]

Comment on attachment 126489 [details] [diff] [review]
Proposed patch: back out Ian's change

Looks good to me.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 126489 [details] [diff] [review]
Proposed patch: back out Ian's change

The assertions in comment 2 above seem correct.

Comment 5

[Ian McGreer]

I think the reason this worked with SSL is that the SSL code itself provides a
synchronization of context destruction and closure of the symkey's session.  I
probably made this mistake by only considering the implications on SSL.

This will take a hit on performance, as it was the most interesting part of the
session reuse changes.  I'll try to think of something else, and open an RFE if
I do.

Comment 6

[Wan-Teh Chang]

Patch checked into the NSS tip (3.9), NSS_3_8_BRANCH (3.8.2),
and NSS_3_7_BRANCH (3.7.8).