353422 - Klocwork bugs in nss/lib/crmf

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

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:]]

(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)]

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)]

*** Bug 353432 has been marked as a duplicate of this bug. ***

Comment 4

[Kai Engert [:KaiE:]]

Attachment: Patch v1 - cvs diff -w (ignores whitespace)

what Nelson said

Comment 5

[Kai Engert [:KaiE:]]

Attachment: Patch v1 - standard patch including whitespace adjustments

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

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

r=nelson

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

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]

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:]]

taking for checkin, I guess we want this on 3.11 branch, too

Comment 10

[Kai Engert [:KaiE:]]

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 ?