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.
Alexei just fixed a double-free bug. Alexei, was it this one?
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.
Kai, please retest with the current trunk, and reopen this bug if it does.
I still get the crash after updating firefox, nspr and nss to the latest trunk.
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; cvin.type = cert_pi_policyOID; cvin.value.arraySize = 1; cvin.value.array.oids = &oid_tag; // "2.16.840.1.1137188.8.131.52.6" cvin.type = cert_pi_end; CERTValOutParam cvout; cvout.type = cert_po_trustAnchor; cvout.type = cert_po_end; rv = CERT_PKIXVerifyCert(mCert, certificateUsageSSLServer, cvin, cvout, nsnull); I don't crash immediately, but during free calls later on.
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.
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.
Only one function is needed to be change: cert_GetTargetCertConstraints.
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!
Created attachment 289377 [details] [diff] [review] Patch v2 Slightly improved version of the patch that checks the return value.
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
Patch checked in, bug is fixed. Not yet marking as fixed, to keep this bug on the review radar.