Last Comment Bug 353422 - Klocwork bugs in nss/lib/crmf
: Klocwork bugs in nss/lib/crmf
Status: RESOLVED FIXED
: coverity, klocwork
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: trunk
: All All
: P2 normal (vote)
: 3.11.4
Assigned To: Kai Engert (:kaie)
:
Mentors:
: 353432 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-09-19 17:03 PDT by Nelson Bolyard (seldom reads bugmail)
Modified: 2006-10-04 11:31 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 - cvs diff -w (ignores whitespace) (1.10 KB, patch)
2006-09-26 09:19 PDT, Kai Engert (:kaie)
no flags Details | Diff | Splinter Review
Patch v1 - standard patch including whitespace adjustments (1.12 KB, patch)
2006-09-26 09:20 PDT, Kai Engert (:kaie)
nelson: review+
rrelyea: superreview+
Details | Diff | Splinter Review

Description Nelson Bolyard (seldom reads bugmail) 2006-09-19 17:03:14 PDT
Klocwork ID 76812
File     nss/lib/crmf/crmfdec.c
function crmf_decode_process_popoprivkey

Numerous errors in this function.
The default case (at line 145) for the switch (at line 135) sets rv
but does not return.  Instead it immediately falls through to a line that 
ignores rv and assigns a new value to rv.  
Consequently, 'privKeyDer.len' might be used uninitialized at line 161.

Kai, does this function get used from PSM?
Comment 1 Kai Engert (:kaie) 2006-09-19 17:11:37 PDT
(In reply to comment #0)
> 
> Kai, does this function get used from PSM?

I could not find any code in PSM which calls any of the functions whose name starts with crmf_decode_process

The only functions named differently, which might be indirectly calling the above function, are CRMF_CreateCertReqMsgFromDER and CRMF_CreateCertReqMessagesFromDER. They are not called by PSM either.
Comment 2 Nelson Bolyard (seldom reads bugmail) 2006-09-23 20:01:27 PDT
ID:       89595
Function: CMMF_DestroyCertifiedKeyPair
Location: nss/lib/crmf/respcmn.c : 271

Pointer 'inCertKeyPair' checked for NULL at line 268 will be dereferenced 
at line 271.

267	    PORT_Assert(inCertKeyPair != NULL); 
268	    if (inCertKeyPair != NULL) { 
269	        cmmf_DestroyCertOrEncCert(&inCertKeyPair->certOrEncCert, 
270	    } 
271	    if (inCertKeyPair->privateKey) { 
272	        crmf_destroy_encrypted_value(inCertKeyPair->privateKey, 
273	    } 

Clearly the if block beginning at line 268 should not end at line 270,
but should enclose the rest of the function.
Comment 3 Nelson Bolyard (seldom reads bugmail) 2006-09-23 22:10:26 PDT
*** Bug 353432 has been marked as a duplicate of this bug. ***
Comment 4 Kai Engert (:kaie) 2006-09-26 09:19:33 PDT
Created attachment 240156 [details] [diff] [review]
Patch v1 - cvs diff -w (ignores whitespace)

what Nelson said
Comment 5 Kai Engert (:kaie) 2006-09-26 09:20:00 PDT
Created attachment 240157 [details] [diff] [review]
Patch v1 - standard patch including whitespace adjustments
Comment 6 Nelson Bolyard (seldom reads bugmail) 2006-09-26 10:08:31 PDT
Comment on attachment 240157 [details] [diff] [review]
Patch v1 - standard patch including whitespace adjustments

r=nelson
Comment 7 Nelson Bolyard (seldom reads bugmail) 2006-09-26 10:09:24 PDT
Comment on attachment 240156 [details] [diff] [review]
Patch v1 - cvs diff -w (ignores whitespace)

I gave r+ to the other patch, since it is the one that would actually get applied.
Comment 8 Robert Relyea 2006-09-26 11:36:22 PDT
Comment on attachment 240157 [details] [diff] [review]
Patch v1 - standard patch including whitespace adjustments

Yes, apply this patch.
Obviously we don't call this Destroy function with NULL pointers a lot;).

bob
Comment 9 Kai Engert (:kaie) 2006-09-26 11:48:32 PDT
taking for checkin, I guess we want this on 3.11 branch, too
Comment 10 Kai Engert (:kaie) 2006-09-26 11:59:32 PDT
fixed on trunk and 3.11 branch

Checking in respcmn.c;
/cvsroot/mozilla/security/nss/lib/crmf/respcmn.c,v  <--  respcmn.c
new revision: 1.12; previous revision: 1.11

Checking in respcmn.c;
/cvsroot/mozilla/security/nss/lib/crmf/respcmn.c,v  <--  respcmn.c
new revision: 1.10.28.2; previous revision: 1.10.28.1

I guess I should set the target milestone to 3.11.4 ?

Note You need to log in before you can comment on or make changes to this bug.