If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

vfychain asserts on shutdown with DSA cert

RESOLVED FIXED in 3.9.1

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Bishakha Banerjee, Assigned: Julien Pierre)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

14 years ago
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.
(Reporter)

Comment 1

14 years ago
Created attachment 140030 [details]
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.
(Reporter)

Comment 2

14 years ago
Created attachment 140032 [details]
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.
(Assignee)

Comment 3

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

This script does not reproducet the problem for me.
Attachment #140032 - Flags: review-
(Assignee)

Comment 4

14 years ago
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.
(Assignee)

Comment 5

14 years ago
Created attachment 140044 [details] [diff] [review]
free issuer cert
Assignee: MisterSSL → jpierre
Status: NEW → ASSIGNED
(Assignee)

Updated

14 years ago
Attachment #140044 - Flags: superreview?(nelsonbhchan)
Attachment #140044 - Flags: review?(wchang0222)
(Assignee)

Updated

14 years ago
Attachment #140044 - Flags: superreview?(MisterSSL)
(Assignee)

Updated

14 years ago
Attachment #140044 - Flags: superreview?(nelsonbhchan)
(Assignee)

Updated

14 years ago
OS: Linux → All
Priority: -- → P1
Target Milestone: --- → 3.9.1
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.
Attachment #140044 - Flags: review?(wchang0222) → review+
Comment on attachment 140044 [details] [diff] [review]
free issuer cert

r=MisterSSL
Attachment #140044 - Flags: superreview?(MisterSSL) → superreview+
Attachment #140044 - Flags: review+
(Assignee)

Updated

14 years ago
Attachment #140044 - Flags: review?(wchang0222)

Comment 8

14 years ago
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.
Attachment #140044 - Flags: review?(wchang0222) → review+

Updated

14 years ago
Component: Tools → Libraries
(Assignee)

Comment 9

14 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Summary: crash reported in vfychain: assertion → vfychain asserts on shutdown with DSA cert
You need to log in before you can comment on or make changes to this bug.