Closed Bug 469588 Opened 11 years ago Closed 11 years ago

Coverity errors reported for softoken

Categories

(NSS :: Libraries, defect, P1)

3.12.4
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.12.4

People

(Reporter: nelson, Assigned: nelson)

Details

(Keywords: coverity, Whiteboard: FIPS Thaw)

Attachments

(2 files)

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.
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
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.
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.
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!
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.
Because of the memory leaks and potential crashes, making this P1
Priority: P2 → P1
Keywords: coverity
This REALLY should have been addressed for the FIPS release!
P1, 4 months old. :(
Whiteboard: FIPS
Target Milestone: 3.12.3 → 3.12.4
Whiteboard: FIPS → FIPS SUN_WANTS
Bob, please review thoroughly.  
I am particularly unsure about the changes to softoken/sftkmod.c.
Attachment #381452 - Flags: review?(rrelyea)
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: rrelyea → nelson
Status: NEW → ASSIGNED
Attachment #381452 - Flags: review?(rrelyea) → review+
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
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
(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?
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
Whiteboard: FIPS SUN_WANTS → FIPS Thaw
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
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: 11 years ago
Resolution: --- → FIXED
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 → ---
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 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)
Attachment #382995 - Flags: review?(nelson) → review+
Comment on attachment 382995 [details] [diff] [review]
Only free arena if we created it in the first place.

r=nelson
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:(
So, I think this is resolved/fixed again.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.