The default bug view has changed. See this FAQ.

Application crashes after having called CERT_PKIXVerifyCert

RESOLVED FIXED in 3.12

Status

NSS
Libraries
P1
critical
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: kaie, Assigned: Alexei Volkov)

Tracking

({crash})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PKIX NSS312B1)

Attachments

(1 attachment, 1 obsolete attachment)

782 bytes, patch
Alexei Volkov
: review+
Robert Relyea
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
When attempting to verify Paypal's cert for EV, I was calling PKIX_VerifyCert.

There seems to be a double-free-bug.

When PSM attempts to clean up all tracked, but not yet free NSS objects, we crash inside of CERT_DestroyCertificate. The pointer value looks like the already-freed-value.
(Reporter)

Updated

10 years ago
Keywords: crash
Alexei just fixed a double-free bug.  
Alexei, was it this one?
Severity: normal → critical
Priority: -- → P1
Whiteboard: PKIX NSS312B1
Target Milestone: --- → 3.12
Assignee: nobody → alexei.volkov.bugs
(Assignee)

Comment 2

10 years ago
Kai is testing with NSS 3.12 alpha 2 and so the described bug is not recent regression.
The fixed bug 403470 is a recent regression. The two bugs have nothing in common.  
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 403470
Kai, please retest with the current trunk, and reopen this bug if it does.
(Reporter)

Comment 5

10 years ago
I still get the crash after updating firefox, nspr and nss to the latest trunk.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(Reporter)

Comment 6

10 years ago
Sorry, I don't have a minimal test case right now.
It happens after PSM's call to CERT_PKIXVerifyCert in nsNSSCertificate::hasValidEVOidTag

  CERTValInParam cvin[2];
  cvin[0].type = cert_pi_policyOID;
  cvin[0].value.arraySize = 1; 
  cvin[0].value.array.oids = &oid_tag; // "2.16.840.1.113733.1.7.23.6"

  cvin[1].type = cert_pi_end;

  CERTValOutParam cvout[2];
  cvout[0].type = cert_po_trustAnchor;
  cvout[1].type = cert_po_end;

  rv = CERT_PKIXVerifyCert(mCert, certificateUsageSSLServer,
                           cvin, cvout, nsnull);

I don't crash immediately, but during free calls later on.
(Assignee)

Comment 7

10 years ago
Seems like I've found the problem related to this bug in function cert_GetTargetCertConstraints: it uses a reference to a cert without population of the cert ref count. Function  pkix_pl_Cert_CreateWithNSSCert consumes the cert without populating the ref count - thus, you may miss a ref count unless you dupe the cert before calling CERT_PKIXVerifyCert.

The correct function to use would be PKIX_PL_Cert_CreateFromCERTCertificate.
(Reporter)

Updated

10 years ago
Summary: Application crashes after having called PKIX_VerifyCert → Application crashes after having called CERT_PKIXVerifyCert
Alexei, you're suggesting that some function(s) be changed to call 
PKIX_PL_Cert_CreateFromCERTCertificate instead of something else.
Please tell us what function(s) would be changed, and what function call
they are now making that would be changed.
(Assignee)

Comment 9

10 years ago
Only one function is needed to be change: cert_GetTargetCertConstraints.
(Reporter)

Comment 10

10 years ago
Created attachment 288997 [details] [diff] [review]
Patch v1

Alexei, thanks for your analysis and your proposal!

I think this patch is what you suggest. 
I tried it, and indeed, it fixes the crash!
Attachment #288997 - Flags: review?(alexei.volkov.bugs)
(Reporter)

Updated

10 years ago
Duplicate of this bug: 403698
(Reporter)

Comment 12

10 years ago
Created attachment 289377 [details] [diff] [review]
Patch v2

Slightly improved version of the patch that checks the return value.
Attachment #288997 - Attachment is obsolete: true
Attachment #289377 - Flags: review?(alexei.volkov.bugs)
Attachment #288997 - Flags: review?(alexei.volkov.bugs)

Comment 13

10 years ago
Comment on attachment 289377 [details] [diff] [review]
Patch v2

r+ This looks good to me, kaie. go ahead and check it in.

Alexei I'd still like a review from you, though.

Thanks,

bob
Attachment #289377 - Flags: superreview+
(Reporter)

Comment 14

10 years ago
Patch checked in, bug is fixed.

Not yet marking as fixed, to keep this bug on the review radar.
(Assignee)

Updated

10 years ago
Attachment #289377 - Flags: review?(alexei.volkov.bugs) → review+
(Reporter)

Comment 15

10 years ago
fixed
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.