Last Comment Bug 403685 - Application crashes after having called CERT_PKIXVerifyCert
: Application crashes after having called CERT_PKIXVerifyCert
: crash
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: trunk
: All All
P1 critical (vote)
: 3.12
Assigned To: Alexei Volkov
: 403698 (view as bug list)
Depends on:
Blocks: evtracker
  Show dependency treegraph
Reported: 2007-11-13 14:12 PST by Kai Engert (:kaie)
Modified: 2007-11-20 15:28 PST (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Patch v1 (1.16 KB, patch)
2007-11-16 06:43 PST, Kai Engert (:kaie)
no flags Details | Diff | Splinter Review
Patch v2 (782 bytes, patch)
2007-11-19 13:27 PST, Kai Engert (:kaie)
alvolkov.bgs: review+
rrelyea: superreview+
Details | Diff | Splinter Review

Description User image Kai Engert (:kaie) 2007-11-13 14:12:50 PST
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.
Comment 1 User image Nelson Bolyard (seldom reads bugmail) 2007-11-13 15:19:58 PST
Alexei just fixed a double-free bug.  
Alexei, was it this one?
Comment 2 User image Alexei Volkov 2007-11-13 15:40:00 PST
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.  
Comment 3 User image Nelson Bolyard (seldom reads bugmail) 2007-11-13 15:42:53 PST

*** This bug has been marked as a duplicate of bug 403470 ***
Comment 4 User image Nelson Bolyard (seldom reads bugmail) 2007-11-13 15:55:29 PST
Kai, please retest with the current trunk, and reopen this bug if it does.
Comment 5 User image Kai Engert (:kaie) 2007-11-13 16:10:43 PST
I still get the crash after updating firefox, nspr and nss to the latest trunk.
Comment 6 User image Kai Engert (:kaie) 2007-11-13 16:16:14 PST
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."

  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.
Comment 7 User image Alexei Volkov 2007-11-14 16:43:18 PST
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.
Comment 8 User image Nelson Bolyard (seldom reads bugmail) 2007-11-15 00:08:15 PST
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.
Comment 9 User image Alexei Volkov 2007-11-15 11:16:01 PST
Only one function is needed to be change: cert_GetTargetCertConstraints.
Comment 10 User image Kai Engert (:kaie) 2007-11-16 06:43:15 PST
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!
Comment 11 User image Kai Engert (:kaie) 2007-11-16 06:52:45 PST
*** Bug 403698 has been marked as a duplicate of this bug. ***
Comment 12 User image Kai Engert (:kaie) 2007-11-19 13:27:37 PST
Created attachment 289377 [details] [diff] [review]
Patch v2

Slightly improved version of the patch that checks the return value.
Comment 13 User image Robert Relyea 2007-11-20 11:14:48 PST
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.


Comment 14 User image Kai Engert (:kaie) 2007-11-20 11:22:00 PST
Patch checked in, bug is fixed.

Not yet marking as fixed, to keep this bug on the review radar.
Comment 15 User image Kai Engert (:kaie) 2007-11-20 15:28:55 PST

Note You need to log in before you can comment on or make changes to this bug.