469623 - Coverity bugs in nss/lib/pki

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

Coverity is checking Firefox's snapshot of NSS 3.12, which is now pretty old.
These bugs may already be fixed, or may be different.  

CID 1041 - null ptr dereference

File nss/lib/pki/pkibase.c, function nssCertificateArray_FindBestCertificate

515  		bestdc = nssCertificate_GetDecoding(bestCert);
516  		/* time */
517  		if (bestdc->isValidAtTime(bestdc, time)) {

Event dereference: Dereferencing NULL value "bestdc"

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

CID 1306 - null ptr dereference

File nss/lib/pki/pkibase.c, function NSSTime_SetPRTime

1252 	    rvTime = (timeOpt) ? timeOpt : nss_ZNEW(NULL, NSSTime);
1253 	    rvTime->prTime = prTime;

Event dereference: Dereferencing NULL value "rvTime"

Comment 2

[Shailen]

Attachment: Patch V 1

Hi Nelson,

  Please review the patch.

Thanks,
Shailendra

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 433786 [details] [diff] [review]
Patch V 1

Sorry, This patch makes two changes and there are problems with 
both of them.

> 	bestdc = nssCertificate_GetDecoding(bestCert);
>+	if (!bestdc) continue;
> 	/* time */

This code occurs in a loop that goes through a list of certs, 
trying to find the "best" one based on comparing the decoded 
values.  If, for some reason, one cannot be decoded, and the 
other can, then the one that cannot be decoded cannot be the 
"best" cert.  So, if bestdc is null, meaning that the cert we
think is "best" until now cannot be decoded, then we must not
go on continuing to think it is the best.  We must switch to 
the other one, e.g. 

	    nssCertificate_Destroy(bestCert);
	    bestCert = nssCertificate_AddRef(c);
            continue;

> {
>     NSSTime *rvTime;
>     rvTime = (timeOpt) ? timeOpt : nss_ZNEW(NULL, NSSTime);
>-    rvTime->prTime = prTime;
>+    if (!rvTime) {
>+        rvTime->prTime = prTime;
>+    }
>     return rvTime;
> }

Uh, no, definitely not.  Look at that again.

Comment 4

[Shailen]

Attachment: Patch Version 2

>This code occurs in a loop that goes through a list of certs,
>trying to find the "best" one based on comparing the decoded
>values.  If, for some reason, one cannot be decoded, and the
>other can, then the one that cannot be decoded cannot be the
>"best" cert.  So, if bestdc is null, meaning that the cert we
>think is "best" until now cannot be decoded, then we must not
>go on continuing to think it is the best.  We must switch to
>the other one, e.g.

>           nssCertificate_Destroy(bestCert);
>           bestCert = nssCertificate_AddRef(c);
>           continue;

Thanks for providing the detailed explanation. I made the the changes you suggested.

> Uh, no, definitely not.  Look at that again.

Sorry. I must have closed my eyes while making this change :( I have corrected this in the attached patch file. Can you please review and let me know if I had missed anything.

Thanks,
Shailendra

Comment 5

[Shailen]

Attachment: Patch Version 3

Few space/tab issues in patch version 2 and rectified in the patch version 3.

Thanks and Regards,
Shailendra

Comment 6

[Shailen]

Attachment: Patch Version 4

One more space/tab issue found in Patch Version 3 and rectified the same in version 4.

Thanks and Regards,
Shailendra

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 433878 [details] [diff] [review]
Patch Version 4

r=nelson.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Checking in lib/pki/pkibase.c; new revision: 1.32; previous revision: 1.31

Thanks for the contribution!