Coverity bugs in nss/lib/pki

RESOLVED FIXED in 3.12.7

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
10 years ago
8 years ago

People

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

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

1.38 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
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"
(Reporter)

Comment 1

10 years ago
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"
Assignee: nobody → nelson
Priority: -- → P1
(Reporter)

Updated

9 years ago
Target Milestone: 3.12.3 → 3.12.5
(Reporter)

Updated

8 years ago
Assignee: nelson → nobody
Target Milestone: 3.12.5 → ---
(Assignee)

Comment 2

8 years ago
Created attachment 433786 [details] [diff] [review]
Patch V 1

Hi Nelson,

  Please review the patch.

Thanks,
Shailendra
Attachment #433786 - Flags: review?(nelson)
(Reporter)

Updated

8 years ago
Attachment #433786 - Flags: review?(nelson) → review-
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.
(Assignee)

Comment 4

8 years ago
Created attachment 433876 [details] [diff] [review]
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
Attachment #433786 - Attachment is obsolete: true
Attachment #433876 - Flags: review?(nelson)
(Assignee)

Comment 5

8 years ago
Created attachment 433877 [details] [diff] [review]
Patch Version 3

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

Thanks and Regards,
Shailendra
Attachment #433876 - Attachment is obsolete: true
Attachment #433877 - Flags: review?(nelson)
Attachment #433876 - Flags: review?(nelson)
(Assignee)

Comment 6

8 years ago
Created attachment 433878 [details] [diff] [review]
Patch Version 4

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

Thanks and Regards,
Shailendra
Attachment #433877 - Attachment is obsolete: true
Attachment #433878 - Flags: review?(nelson)
Attachment #433877 - Flags: review?(nelson)
Comment on attachment 433878 [details] [diff] [review]
Patch Version 4

r=nelson.
Attachment #433878 - Flags: review?(nelson) → review+
(Reporter)

Updated

8 years ago
Assignee: nobody → shailen.n.jain
Priority: P1 → P2
Target Milestone: --- → 3.12.7
Checking in lib/pki/pkibase.c; new revision: 1.32; previous revision: 1.31

Thanks for the contribution!
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.