Closed Bug 353422 Opened 14 years ago Closed 14 years ago

Klocwork bugs in nss/lib/crmf

Categories

(NSS :: Libraries, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
3.11.4

People

(Reporter: nelson, Assigned: KaiE)

References

Details

(Keywords: coverity, klocwork)

Attachments

(2 files)

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?
(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.
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.
*** Bug 353432 has been marked as a duplicate of this bug. ***
Keywords: coverity
what Nelson said
Attachment #240156 - Flags: superreview?(rrelyea)
Attachment #240156 - Flags: review?(nelson)
Comment on attachment 240157 [details] [diff] [review]
Patch v1 - standard patch including whitespace adjustments

r=nelson
Attachment #240157 - Flags: review+
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.
Attachment #240156 - Flags: review?(nelson)
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
Attachment #240157 - Flags: superreview+
Attachment #240156 - Flags: superreview?(rrelyea)
taking for checkin, I guess we want this on 3.11 branch, too
Assignee: nobody → kengert
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 ?
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: 3.12 → 3.11.4
Priority: -- → P2
You need to log in before you can comment on or make changes to this bug.