403685 - Application crashes after having called CERT_PKIXVerifyCert

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Kai Engert (:KaiE:)]

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

[Nelson Bolyard (seldom reads bugmail)]

Alexei just fixed a double-free bug.  
Alexei, was it this one?

Comment 2

[Alexei Volkov]

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 4

[Nelson Bolyard (seldom reads bugmail)]

Kai, please retest with the current trunk, and reopen this bug if it does.

Comment 5

[Kai Engert (:KaiE:)]

I still get the crash after updating firefox, nspr and nss to the latest trunk.

Comment 6

[Kai Engert (:KaiE:)]

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.

Comment 7

[Alexei Volkov]

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

[Nelson Bolyard (seldom reads bugmail)]

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

[Alexei Volkov]

Only one function is needed to be change: cert_GetTargetCertConstraints.

Comment 10

[Kai Engert (:KaiE:)]

Attachment: 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 12

[Kai Engert (:KaiE:)]

Attachment: Patch v2

Slightly improved version of the patch that checks the return value.

Comment 13

[Robert Relyea]

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

Comment 14

[Kai Engert (:KaiE:)]

Patch checked in, bug is fixed.

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

Comment 15

[Kai Engert (:KaiE:)]

fixed