126769 - sessionID wrap-around and overflow issues

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Wan-Teh Chang]

This bug is a derivative of bug 125149, where we first noticed
the problems caused by session ID wrap-around and overflow.

In NSS 3.3.2, the session handles are constructed as follows
(mozilla/security/nss/lib/softoken/pkcs11.c, line 2911):
    sessionID = slot->sessionIDCount++;
    if (slotID == PRIVATE_KEY_SLOT_ID) {
        sessionID |= PK11_PRIVATE_KEY_FLAG;
    } else if (slotID == FIPS_SLOT_ID) {
        sessionID |= PK11_FIPS_FLAG;
    } else ...  /* code irrelevent to sessionID */

where PK11_PRIVATE_KEY_FLAG is 0x80000000L and PK11_FIPS_FLAG
is 0x40000000L.

So in NSS 3.3.2 session IDs will wrap around after 2^32 sessions
have been created.  There is no protection against generating
a session handle value of zero (which is invalid) and generating
a session handle that is still in use.

In addition, NSS 3.3.2 has the problem of session ID count
overflowing and corrupting the PK11_PRIVATE_KEY_FLAG and
PK11_FIPS_FLAG bits.  The PK11_PRIVATE_KEY_FLAG bit will
be corrupted after 2^31 sessions have been created and
the PK11_FIPS_FLAG bit will be corrupted after 2^30 sessions
have been created.

In summary, with NSS 3.3.2, subtle bugs *may* occur after
2^30 sessions have been created.

Comment 1

[Wan-Teh Chang]

Changed the QA contact to Bishakha.

Comment 2

[Robert Relyea]

target 3.3.3 should we ever have such a release. (the 3.4 and beyond version of
this bug is already fixed).

Comment 3

[Robert Relyea]

Marking P3 since we already have fixed NSS 3.4.x versions.

Comment 4

[Ian McGreer]

Attachment: patch for 3.3 branch

Bob, can you review this?  There were several differences between 3.4 and 3.3
in this area, I think I understand them.  Note the bitmask used to protect the
two high-order bits, and also that everything is done under slot->sessionLock
(there is no slot->slotLock).

Comment 5

[Wan-Teh Chang]

Comment on attachment 108041 [details] [diff] [review]
patch for 3.3 branch

>-    if (slotID == PRIVATE_KEY_SLOT_ID) {
>-	sessionID |= PK11_PRIVATE_KEY_FLAG;
>-    } else if (slotID == FIPS_SLOT_ID) {
>-	sessionID |= PK11_FIPS_FLAG;
>-    } else if (flags & CKF_RW_SESSION) {
>+    if (slotID == NETSCAPE_SLOT_ID && (flags & CKF_RW_SESSION)) {
> 	/* NETSCAPE_SLOT_ID is Read ONLY */
> 	session->info.flags &= ~CKF_RW_SESSION;
>     }

Are PRIVATE_KEY_SLOT_ID, FIPS_SLOT_ID, and NETSCAPE_SLOT_ID the
only possible values for slotID?

>+	do {
>+	    sessionID = (slot->sessionIDCount++ & 0x3fffffff);
>+	} while (sessionID == CK_INVALID_HANDLE);
>+	pk11queue_find(sameID, sessionID, slot->head, SESSION_HASH_SIZE);
>+	if (sameID == NULL) {
>+	    if (slotID == PRIVATE_KEY_SLOT_ID) {
>+		sessionID |= PK11_PRIVATE_KEY_FLAG;
>+	    } else if (slotID == FIPS_SLOT_ID) {
>+		sessionID |= PK11_FIPS_FLAG;
>+	    }

It seems that the PK11_PRIVATE_KEY_FLAG or PK11_FIPS_FLAG should
be OR'ed in *before* the pk11queue_find() call.

>+    int                 sessionIDConflict;  /* not protected by a lock */

This comment contradicts the code because your code protects
slot->sessionIDConflict with slot->sessionLock.

Comment 6

[Ian McGreer]

Attachment: rev 2

1) those are the only possible values of slot ID
2) good catch, fixed in this patch
3) removed comment in this patch

Comment 7

[Wan-Teh Chang]

Comment on attachment 108081 [details] [diff] [review]
rev 2

>-    slot->sessionIDCount = 1;
>+    slot->sessionIDCount = 0;

A nit: this change is not really necessary, right?

>+    if (slotID == NETSCAPE_SLOT_ID && (flags & CKF_RW_SESSION)) {
> 	/* NETSCAPE_SLOT_ID is Read ONLY */
> 	session->info.flags &= ~CKF_RW_SESSION;
>     }

In the old code, this was done while holding slot->sessionLock.
In the new code, this is done without holding slot->sessionLock.
Is it safe to do that?	If you are not sure, I suggest that you
move this code after the PK11_USE_THREADS(PZ_Lock(slot->sessionLock);)
line.

>+	do {
>+	    sessionID = (slot->sessionIDCount++ & 0x3fffffff);
>+	} while (sessionID == CK_INVALID_HANDLE);

For maintainability, I suggest that you define a macro for
0x3fffffff.  It should be defined in pkcs11i.h, next to the
definitions of PK11_PRIVATE_KEY_FLAG and PK11_FIPS_FLAG.

>     int			sessionIDCount;
>+    int                 sessionIDConflict;
>     int			sessionCount;
>     int			rwSessionCount;
>     int			tokenIDCount;

Is the new line indented correctly?  It's hard to tell from
the patch.

Comment 8

[Ian McGreer]

Attachment: rev 3

1) that part of the patch applied from the 3.4 patch cleanly.  I don't know if
it is necessary, but since it is in 3.4 I took it.

2) okay, moved it under the lock since I don't really know for certain that it
can be outside of it

3) made the macro

4) the other members have tabs between the type declaration and member name.  I
used spaces (exactly so that this kind of thing doesn't happen).

Comment 9

[Wan-Teh Chang]

Comment on attachment 108097 [details] [diff] [review]
rev 3

>@@ -320,6 +321,9 @@
> #define NETSCAPE_SLOT_ID 1
> #define PRIVATE_KEY_SLOT_ID 2
> #define FIPS_SLOT_ID 3
>+
>+/* session IDs mask the slot ID above in the high-order bits */
>+#define MAX_SESSION_ID 0x3fffffff

Sorry to beat this patch to death, but I want this macro to
be defined next to PK11_PRIVATE_KEY_FLAG (0x80000000L) and
PK11_FIPS_FLAG (0x40000000L), not XXX_SLOT_ID's.

Comment 10

[Ian McGreer]

Attachment: rev 4

you're right, I put it in the wrong place.

Comment 11

[Wan-Teh Chang]

Comment on attachment 108102 [details] [diff] [review]
rev 4

r=wtc.

Bob, could you superreview this patch?

Comment 12

[Robert Relyea]

Comment on attachment 108102 [details] [diff] [review]
rev 4

One note: sessionID of '0' is reserved, which is why sessionIDCount started at
'1'. The current code will work because it has a loop checking for
CK_INVALID_HANDLE and disallow sessionID of '0', so it's all correct.

bob

Comment 13

[Ian McGreer]

checked in on 3.3 branch.