Closed
Bug 126769
Opened 23 years ago
Closed 22 years ago
sessionID wrap-around and overflow issues
Categories
(NSS :: Libraries, defect, P3)
Tracking
(Not tracked)
RESOLVED
FIXED
3.3.3
People
(Reporter: wtc, Assigned: rrelyea)
Details
Attachments
(1 file, 3 obsolete files)
2.85 KB,
patch
|
wtc
:
review+
rrelyea
:
superreview+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•23 years ago
|
||
Changed the QA contact to Bishakha.
QA Contact: sonja.mirtitsch → bishakhabanerjee
Assignee | ||
Comment 2•23 years ago
|
||
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
Assignee | ||
Comment 3•23 years ago
|
||
Marking P3 since we already have fixed NSS 3.4.x versions.
Priority: -- → P3
Comment 4•22 years ago
|
||
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).
Reporter | ||
Comment 5•22 years ago
|
||
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•22 years ago
|
||
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
Reporter | ||
Comment 7•22 years ago
|
||
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•22 years ago
|
||
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
Reporter | ||
Comment 9•22 years ago
|
||
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-
Comment 10•22 years ago
|
||
you're right, I put it in the wrong place.
Attachment #108097 -
Attachment is obsolete: true
Reporter | ||
Comment 11•22 years ago
|
||
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+
Assignee | ||
Comment 12•22 years ago
|
||
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+
Comment 13•22 years ago
|
||
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.
Description
•