The default bug view has changed. See this FAQ.

Klocwork bugs in nss/lib/crmf

RESOLVED FIXED in 3.11.4

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: kaie)

Tracking

({coverity, klocwork})

trunk
3.11.4
coverity, klocwork

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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?
(Assignee)

Comment 1

11 years ago
(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.
Keywords: klocwork
(Reporter)

Comment 2

11 years ago
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.
(Reporter)

Comment 3

11 years ago
*** Bug 353432 has been marked as a duplicate of this bug. ***
(Reporter)

Updated

11 years ago
Keywords: coverity
(Assignee)

Comment 4

11 years ago
Created attachment 240156 [details] [diff] [review]
Patch v1 - cvs diff -w (ignores whitespace)

what Nelson said
Attachment #240156 - Flags: superreview?(rrelyea)
Attachment #240156 - Flags: review?(nelson)
(Assignee)

Comment 5

11 years ago
Created attachment 240157 [details] [diff] [review]
Patch v1 - standard patch including whitespace adjustments
(Reporter)

Comment 6

11 years ago
Comment on attachment 240157 [details] [diff] [review]
Patch v1 - standard patch including whitespace adjustments

r=nelson
Attachment #240157 - Flags: review+
(Reporter)

Comment 7

11 years ago
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 8

11 years ago
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+

Updated

11 years ago
Attachment #240156 - Flags: superreview?(rrelyea)
(Assignee)

Comment 9

11 years ago
taking for checkin, I guess we want this on 3.11 branch, too
Assignee: nobody → kengert
(Assignee)

Comment 10

11 years ago
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
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: 3.12 → 3.11.4
(Reporter)

Updated

11 years ago
Priority: -- → P2
You need to log in before you can comment on or make changes to this bug.