Closed
Bug 469588
Opened 16 years ago
Closed 15 years ago
Coverity errors reported for softoken
Categories
(NSS :: Libraries, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
3.12.4
People
(Reporter: nelson, Assigned: nelson)
Details
(Keywords: coverity, Whiteboard: FIPS Thaw)
Attachments
(2 files)
5.12 KB,
patch
|
rrelyea
:
review+
|
Details | Diff | Splinter Review |
484 bytes,
patch
|
nelson
:
review+
|
Details | Diff | Splinter Review |
Coverity tests NSS as part of Firefox. Of course, it is testing the snapshot used in Firefox, not the NSS trunk. Coverity run 278 reports lots of errors in softoken. Some of these may already be fixed on the trunk. ============================================================= CID 1402 - use after free, double free, in NSC_DestroyObject. Near the end of NSC_DestroyObject, we see: sftk_DeleteObject(session,object); sftk_FreeSession(session); status = sftk_FreeObject(object); Coverity claims that sftk_DeleteObject frees "object" by passing it to sftk_FreeObject(object), but then two lines later NSC_DestroyObject passes it to sftk_FreeObject again. =========================================================== CID 1399 - use of uninitialized variable crv in sftk_FindTokenAttribute If the attempt to allocate variable myattribute fails (gets NULL) at myattribute = (SFTKAttribute*)PORT_Alloc(sizeof(SFTKAttribute)); Then at label loser, variable crv is used, uninitialized. There are more CIDs. I will add them in subsequent comments to this bug.
Assignee | ||
Comment 1•16 years ago
|
||
CID 1359 - leak in sftkdb_AddSecmodDB in sftkdb_AddSecmodDB, inside the while (*module) loop, after value = sftk_argFetchValue has stored the address of an allocated block in "value", if block = sftkdb_DupnCat(block, module, keyEnd-module+1); stores NULL in block, then the code goes to loser and leaks value.
Priority: -- → P2
Assignee | ||
Comment 2•16 years ago
|
||
CID 1370 - null check AFTER pointer has been dereferenced In file nss/lib/softoken/sdb.c function s_open, the code dereferences pointer variable certdb, then checks it for NULL. CID 1371 - null check AFTER pointer has been dereferenced In file nss/lib/softoken/sdb.c function s_open, the code dereferences pointer variable keydb, then checks it for NULL.
Assignee | ||
Comment 3•16 years ago
|
||
CID 1358 - memory leak In file nss/lib/softoken/sftkmod.c, In function sftkdb_DeleteSecmodDB, The allocated block whose address is stored in variable "name" at name = sftk_argGetParamValue("name",args); is not freed in all paths. There exists at least one path in which it is leaked. Likewise, in the same function, the block allocated by lib = sftk_argGetParamValue("library",args); is leaked in at least one path. ======================================================================== SID 1260 - dead code in sftkdb_DeleteSecmodDB In function sftkdb_DeleteSecmodDB, at label loser, coverity claims that the condition 613 if (fd2 != NULL) { can never be true, and thus the following code 614 fclose(fd2); is dead code.
Assignee | ||
Comment 4•16 years ago
|
||
SID 1385 - null check after use In file nss/lib/softoken/sftkpwd.c function sftkdb_FreeUpdatePasswordKey the code dereferences the pointer argument named handle, and then immediately after that, checks it for NULL. oy!
Assignee | ||
Comment 5•16 years ago
|
||
SID 1261 - dead code In file nss/lib/softoken/sftkdb.c, function sftkdb_write, at label loser, inside of the block that begins 901 if (inTransaction) { Coverity claims that CRV cannot be zero, and so the following lines are dead code: 906 PORT_Assert(crv != CKR_OK); 907 if (crv == CKR_OK) crv = CKR_GENERAL_ERROR; ======================================================================== SID 1308 - NULL pointer dereference (crash) In function sftkdb_updateObjectTemplate, at case CKO_SECRET_KEY, we see 1811 attr = sftkdb_getAttributeFromTemplate(CKA_ID,ptemplate,*plen); 1812 crv = sftkdb_incrementCKAID(arena, attr); attr can be NULL, and will be dereferenced. ======================================================================= SID 1360 - leak In function sftkdb_SetAttributeValue, if the attempt to allocate "arena" fails (arena gets NULL), then we return without freeing allocated ntemplate.
Assignee | ||
Comment 6•16 years ago
|
||
Because of the memory leaks and potential crashes, making this P1
Priority: P2 → P1
Assignee | ||
Comment 7•15 years ago
|
||
This REALLY should have been addressed for the FIPS release! P1, 4 months old. :(
Whiteboard: FIPS
Assignee | ||
Updated•15 years ago
|
Target Milestone: 3.12.3 → 3.12.4
Assignee | ||
Updated•15 years ago
|
Whiteboard: FIPS → FIPS SUN_WANTS
Assignee | ||
Comment 8•15 years ago
|
||
Bob, please review thoroughly. I am particularly unsure about the changes to softoken/sftkmod.c.
Attachment #381452 -
Flags: review?(rrelyea)
Assignee | ||
Comment 9•15 years ago
|
||
Oh, by the way, after reviewing the code, I am not at all sure that the alleged "bug" described in comment 0 as CID 1402 - use after free, double free, in NSC_DestroyObject. is really a bug. Bob, I'd appreciate your analysis of that.
Assignee | ||
Updated•15 years ago
|
Assignee: rrelyea → nelson
Status: NEW → ASSIGNED
Updated•15 years ago
|
Attachment #381452 -
Flags: review?(rrelyea) → review+
Comment 10•15 years ago
|
||
Comment on attachment 381452 [details] [diff] [review] untested patch v1 r+ rrelyea. The patches as supplied are correct. There are 2 issues that are described in this bug which do not have patches: CID 1402 (which nelson has indicated). SID 1260 - dead code SID 2161 - dead code. I have not reviewed the source code to those related issues yet. Minor comments on the bug.: Patch to sftkdb_SetAttributeValue(): the Abort call could be handled by calling abort inside the return code check before the goto loser. That would remove the need for the 'inTransaction' variable. On the other hand, the code in the patch as it now stands is safer in view of future 'improvements' (in the case where there may be multiple commands which could fail). No change to patch required. in sftk_updateObjectTemplate(). The rc code in that case should be CKR_ATTRIBUTE_TYPE_INVALID (spelling my need adjustment). This is minor since the code only cares about success for failure. The failure case would be the case where the token returned to us an improperly defined key (keys could have NULL id's, but all keys should have a CKA_ID). Such a mistake is theoretically possible, though unlikely (I think in the case of this code, it would require a corrupted libdbmnss2.so. In any case the code is a good defence, though I would accept a change in the CKR value to match the correct error code
Comment 11•15 years ago
|
||
NOTE: none of these appears to be an issue for the validation. I would be OK for inclusing in the FIPS respin. bob
Version: 3.12 → 3.12.4
Comment 12•15 years ago
|
||
(In reply to comment #9) > Oh, by the way, after reviewing the code, I am not at all sure that the > alleged "bug" described in comment 0 as > CID 1402 - use after free, double free, in NSC_DestroyObject. > is really a bug. > Bob, I'd appreciate your analysis of that. sftk_FreeObject(object) function can really be called twice: one time in sftk_DeleteObject when dealing with a token with a valid session http://mxr.mozilla.org/security/source/security/nss/lib/softoken/pkcs11u.c#1224 and the second time: after exiting from sftk_DeleteObject function http://mxr.mozilla.org/security/source/security/nss/lib/softoken/pkcs11c.c#303 But object is protected by the ref counter: the memory will be freed only once. To my surprise FreeObject does not protect from an attempt to free an object more times than it reference was acquired(refCount is not checked against null). I'm wondering if it was done intentionally?
Comment 13•15 years ago
|
||
Yes, this is expected behavior. The sftk_FreeObject in sftk_DeleteObject frees the reference to the object that is held by the list (this is what keeps the object from being deleted during it's normal lifetime). The sftk_FreeObject call at the endo of NSC_DestroyObject frees the reference acquired with sftk_ObjectFromHandle(hObject,session). It's only after the second free that destroy will be called. sftk_FreeObject does not check refCount <= 0 namely because it would expect the whole object it's dealing with to be toast at this point (because it was freed). If we had a bug here, then we would see server crashes when ever the server finished handling a peak load. The sftk_FreeObject would cause sftk_DestroyObject to be called, if the server is freeing up several objects and our object freelist is beyond the maximum, the destroyed object would have it's locks and data destroyed. The second sftk_FreeObject would crash at this point. Looking at the other cases. Because sftk_FreeObject guarrentees that Destroy is only called once & because the Object goes on the freelist (so it remains mostly valid through the call) and because freed objects are placed on free lists (to reuse the object and lock for performance reasons), as long as you never have more than 800 objects on that list, the code would mask multiple frees. On the other hand, you can see from inspection that there cannot be a double free going on here. 1. The reference count on the object on entry must be one or more (otherwise the object would have been freed). 2. sftk_ObjectFromHandle(hObject,session) acquires at least one reference (I walked the stack and verified this). 3. Now the reference count is at least 2, for the object to be deleted we need at least 2 frees. 4. We have at most 2 more frees, so we know the object can, at most be deleted after the second free. bob
Updated•15 years ago
|
Whiteboard: FIPS SUN_WANTS → FIPS Thaw
Comment 14•15 years ago
|
||
I've now looked at the dead code issues. Both are actual dead code. The first is in a loser block. For code maintainence issues I think it should stay. Future modifications to the function may add a new user label and keeping the code there is unlikely to cause problem, but likely to prevent future problems in modified versions. The second block is known 'dead code'. First, there is a assert. Second, there is a comment which says that it's trivial to show that crv can't be CKR_OK. Clearly we've made an evaluation that keeping the code in helps clarity. I think that covers all the issues and we can probably then close this bug. bob
Assignee | ||
Comment 15•15 years ago
|
||
Checking in pkcs11u.c; new revision: 1.82; previous revision: 1.81 Checking in sdb.c; new revision: 1.16; previous revision: 1.15 Checking in sftkdb.c; new revision: 1.23; previous revision: 1.22 Checking in sftkmod.c; new revision: 1.7; previous revision: 1.6 Checking in sftkpwd.c; new revision: 1.10; previous revision: 1.9
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 16•15 years ago
|
||
This bug most probably caused crashes of certutil in one test that I found just accidentally: --------------------------------------------------------------- | test importing an old cert to a conflicting nickname --------------------------------------------------------------- ./dbtests.sh: line 113: 30087 Segmentation fault ${BINDIR}/certutil -A -n alice -t ,, -i ${R_BOBDIR}/Bob.cert -d ${CONFLICT_DIR} Core stack: 0 PORT_FreeArena_Util + 43 (secport.c:290) 1 sftkdb_SetAttributeValue + 479 (sftkdb.c:1396) 2 sftk_forceTokenAttribute + 162 (pkcs11u.c:550) 3 sftk_forceAttribute + 224 (pkcs11u.c:575) 4 NSC_SetAttributeValue + 787 (pkcs11.c:4110) 5 nssCKObject_SetAttributes + 84 (ckhelper.c:264) 6 nssToken_ImportCertificate + 1685 (devtoken.c:572) 7 PK11_ImportCert + 749 (pk11cert.c:911) 8 AddCert + 317 (certutil.c:180) 9 certutil_main + 10551 (certutil.c:2846) 10 main + 32 (certutil.c:2981) 11 start + 52 First I found it on nssmac3 Tinderbox machine, later when I checked current nightly builds I found out that it's crashing also on other platforms. This bug is NOT reported as failure in Tinderboxes, because output goes to dbtest.log file that is not checked for failures. I have already prepared patch fixing this (bug 496305), currently waiting for review. I also plan to improve Tinderboxes to be able to detect similar issues even is tests would not be marked as failed (negative tests).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 17•15 years ago
|
||
Comment 18•15 years ago
|
||
I believe what is happening is when we fail the critical check, we used to just 'blow out' (potentially leaking data/ntemplate). Now we go to our error handling, but in this case we now may have not yet allocated the arena (in the old code the arena was always allocated if we go to loser). This check only frees the arena if it had been allocated. I'll ask for a review once I verify this fixed the problem. bob
Comment 19•15 years ago
|
||
Comment on attachment 382995 [details] [diff] [review] Only free arena if we created it in the first place. tests completed. This fixes the crash. bob
Attachment #382995 -
Flags: review?(nelson)
Assignee | ||
Updated•15 years ago
|
Attachment #382995 -
Flags: review?(nelson) → review+
Assignee | ||
Comment 20•15 years ago
|
||
Comment on attachment 382995 [details] [diff] [review] Only free arena if we created it in the first place. r=nelson
Comment 21•15 years ago
|
||
Checking in sftkdb.c; /cvsroot/mozilla/security/nss/lib/softoken/sftkdb.c,v <-- sftkdb.c new revision: 1.24; previous revision: 1.23 done CVS comment is wrong, the comment should read r=nelson:(
Assignee | ||
Comment 22•15 years ago
|
||
So, I think this is resolved/fixed again.
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•