Closed Bug 126769 Opened 23 years ago Closed 22 years ago

sessionID wrap-around and overflow issues

Categories

(NSS :: Libraries, defect, P3)

3.3.2
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wtc, Assigned: rrelyea)

Details

Attachments

(1 file, 3 obsolete files)

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.
Changed the QA contact to Bishakha.
QA Contact: sonja.mirtitsch → bishakhabanerjee
target 3.3.3 should we ever have such a release. (the 3.4 and beyond version of this bug is already fixed).
Target Milestone: --- → 3.3.3
Marking P3 since we already have fixed NSS 3.4.x versions.
Priority: -- → P3
Attached patch patch for 3.3 branch (obsolete) — Splinter Review
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 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.
Attached patch rev 2 (obsolete) — Splinter Review
1) those are the only possible values of slot ID 2) good catch, fixed in this patch 3) removed comment in this patch
Attachment #108041 - Attachment is obsolete: true
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.
Attached patch rev 3 (obsolete) — Splinter Review
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).
Attachment #108081 - Attachment is obsolete: true
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.
Attachment #108097 - Flags: review-
Attached patch rev 4Splinter Review
you're right, I put it in the wrong place.
Attachment #108097 - Attachment is obsolete: true
Comment on attachment 108102 [details] [diff] [review] rev 4 r=wtc. Bob, could you superreview this patch?
Attachment #108102 - Flags: superreview?(relyea)
Attachment #108102 - Flags: review+
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
Attachment #108102 - Flags: superreview?(relyea) → superreview+
checked in on 3.3 branch.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: