232377 - vfychain asserts on shutdown with DSA cert

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Bishakha Banerjee]

This is regarding the crash reported in vfychain in bug 231051

I will attach two scripts to reproduce the bug

But first here, I will paste Julien's observations on the vfychain crash from
bug 231051:

3) There is a remaining failure in vfychain. It is also an assertion caused by
strict shutdown. I have not yet figured out the cause for it or a patch.


I found that the vfychain trap is related to the DSA cert verification. If I
take out signature verification in lib/certhigh/certvfy.c :
	    /* rv = CERT_VerifySignedData(&subjectCert->signatureWrap,
				       issuerCert, t, wincx);
                                       */
and replace it with rv = SECSuccess;

Then the assertion does not occur in NSS_Shutdown().
I have tried debugging the code further. Simply removing the call to
CERT_VerifySignedDataWithPublicKey within CERT_VerifySignedData does not suffice
to fix the assertion. I have to also remove the code that extracts the key and
frees it, making CERT_VerifySignedData a no-op, for the test to pass.
I spent some time looking at seckey_ExtractPublicKey, but did not find anything
wrong with the code there.
I probably have to run only the vfychain program with my changes to find out
what the bug is, rather than re-rerunning the whole script.

Comment 1

[Bishakha Banerjee]

Attachment: test script to reproduce the crash

This test script contains two tests, the vfychain command that is reporting a
core, and an earlier test preceding it.

Comment 2

[Bishakha Banerjee]

Attachment: test script with sole testcase in it

This script contains just the one testcase that is reporting a crash (from the
earlier attachment).
If you check these scripts out as test1 and test2 in nss/tests/pkits, you will
see that this script does not report a crash.

I am going to carry on investigating this a bit more, these are preliminary
observations.
also btw, I checked out a copy of NSS 38 RTM, but was unable to reproduce this
crash in it.

Comment 3

[Julien Pierre]

Comment on attachment 140032 [details]
test script with sole testcase in it

This script does not reproducet the problem for me.

Comment 4

[Julien Pierre]

It took a while, but I was finally able to track this one down to a cert leak in
the seckey_UpdateCertPQGChain function . After finding the issuer certificate,
it was never freed.

Comment 5

[Julien Pierre]

Attachment: free issuer cert

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 140044 [details] [diff] [review]
free issuer cert

Good catch.  

While reviewing the code in this function, I found other bugs in it, such as
failing to consider key IDs when determining whether or not the cert is a root. 

I'll file a separate bug on that.  

Also, this does not apepar to be a recent regression.  Regarding the report
that this bug is not seen in NSS 3.8, I think the explanation is that NSS 3.8's
test script does not set the  NSS_STRICT_SHUTDOWN environment variable.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 140044 [details] [diff] [review]
free issuer cert

r=MisterSSL

Comment 8

[Wan-Teh Chang]

Comment on attachment 140044 [details] [diff] [review]
free issuer cert

r=wtc.	Good job tracking this down, Julien.

Although this patch is correct, it is incomplete.
The following "return rv" needs to be changed to
"goto loser":

   528	    rv = seckey_UpdateCertPQGChain(issuerCert, count);
   529	    if (rv != SECSuccess) return rv;

Please remember to make this additional change
before checking the patch in.

Comment 9

[Julien Pierre]

Thanks for catching that one, Wan-Teh.

Checked in to tip.

Checking in seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.31; previous revision: 1.30

And 3.9 branch.

Checking in seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.30.4.1; previous revision: 1.30