Closed Bug 550204 Opened 11 years ago Closed 11 years ago

crmf_decode_process_popoprivkey passes unset privKeyDer.type or unset privKeyDer to SECITEM_CopyItem

Categories

(NSS :: Libraries, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
3.12.7

People

(Reporter: timeless, Assigned: timeless)

References

()

Details

(Keywords: coverity)

Attachments

(1 file)

118  	crmf_decode_process_popoprivkey(CRMFCertReqMsg *inCertReqMsg)
126  	    SECItem         *derPOP, privKeyDer;

135  	    switch (popoPrivKey->messageChoice) {
136  	    case crmfThisMessage:
137  	    case crmfDHMAC:
138  	        privKeyDer.data = &derPOP->data[5];
139  		privKeyDer.len  = derPOP->len - 5;
140  		break;
141  	    case crmfSubsequentMessage:
142  	        privKeyDer.data = &derPOP->data[4];
143  		privKeyDer.len  = derPOP->len - 4;
144  		break;
145  	    default:
146  	        rv = SECFailure;

this rv is stomped on in 149

147  	    }

privKeyDer.type is *never* set

149  	    rv = SECITEM_CopyItem(inCertReqMsg->poolp, 
150  				  &popoPrivKey->message.subsequentMessage,
151  				  &privKeyDer);

238  	SECITEM_CopyItem(PRArenaPool *arena, SECItem *to, const SECItem *from)
239  	{
240  	    to->type = from->type;
Summary: crmf_decode_process_popoprivkey plays unset privKeyDer.type or unset privKeyDer when calling SECITEM_CopyItem → crmf_decode_process_popoprivkey passes unset privKeyDer.type or unset privKeyDer to SECITEM_CopyItem
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #430541 - Flags: review?(nelson)
Are we sure crmf_decode_process_popoprivkey isn't dead code?
Comment on attachment 430541 [details] [diff] [review]
proposal (checked in)

This patch is fine, as far as it goes, but I suspect it will merely
move the UMR from occurring inside SECITEM_CopyItem to occurring at
these new lines of code.  IOW, I'm saying that I suspect you'll 
find that derPOP->type is also uninitialized.  Hope I'm wrong, but...

>+        privKeyDer.type = derPOP->type;
Attachment #430541 - Flags: review?(nelson) → review+
Checking in crmfdec.c; new revision: 1.5; previous revision: 1.4

I committed this patch. 
Let's wait on closing it until we know what coverity thinks.
Target Milestone: --- → 3.12.7
Attachment #430541 - Attachment description: proposal → proposal (checked in)
Timeless, does Coverity now give this file a clean bill of health?
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Hardware: x86 → All
Resolution: --- → FIXED
run info: xulrunner/20100724
file: security/nss/lib/crmf/crmfdec.c
outstanding defects: 0
functions: 13
loc: 297
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.