231030 - NSS fails NIST name constraint test 4.13.19 on self issued cert

Status: VERIFIED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

The NIST NISCC tests contain some test cases that (I am told) have self-issued
certs in the middle of the cert chain.  NSS fails all these tests.  
One such test is Section 4.13: Name Constraints, case 19.
Others are documented in other mozilla NSS bugs about NIST.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

The reference to NISCC above is mistaken.  It should have read PKITS.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

The command for the Name Constraints test 19 is apparently

vfychain -v -d d:/tmp/pkits -u 4 \
  ValidDNnameConstraintsTest19EE.crt \
  nameConstraintsDN1SelfIssuedCACert.crt \
  nameConstraintsDN1CACert.crt \
  TrustAnchorRootCertificate.crt

NSS fails this test, claiming 

PROBLEM WITH THE CERT CHAIN:
CERT 2. CN=nameConstraints DN1 CA,O=Test Certificates,C=US [Certificate Authorit
y]:
  ERROR -8080: The Certifying Authority for this certificate is not permitted to
 issue a certificate with this name.

The test is supposed to declare the chain valid.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Adding reviewers to CC list.

I have found the problem for test case 19, and am about to attach a patch 
for that test case.  

Bishakha, After this patch and the fix for bug 221644 are checked in, all the
PKITS tests must be redone.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1 - both unified and context diffs

I included both the unified and context diffs.	
unified diffs are considered the norm for mozilla bugs, 
but in this case, I think it is much easier to read the context diff.
The context diff shows that the change is really just adding one test,
and indenting a bunch of code.

Comment 5

[Wan-Teh Chang]

Comment on attachment 139228 [details] [diff] [review]
patch v1 - both unified and context diffs

Nelson, "cvs diff -uw" is a good way to show
that the change is just adding one test and
indenting a bunch of code.  (The indentation
is screwed up by the -w flag but it clears
shows what the new test is.)

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2 - from diff -uw

This patch combines the fix for this bug, and the fix for bug 221644.
It incorporates review feedback from Wan-Teh, and it further clarifies
and simplifies the self-issued test.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

taking bug.  Adding reviewers to CC list.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 139247 [details] [diff] [review]
patch v2 - from diff -uw 

Wan-Teh, please review

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Now that the pkits test script has been fixed, 
we see that the above patch fixes some but not all of these issues. 
With that patch in place, the following self-issued cert test cases still fail:

Invalid Basic Self-Issued Old With New Test2 
Invalid Basic Self-Issued New With Old Test5
Invalid Basic Self-Issued CRL Signing Key Test7

These are all negative test cases that previously were reported as succeeding.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Regarding comment 9, the reason that tests 2, 5 and 7 of section 4.5 fail
is that the pkits.sh script is not loading the requisite CRLs.  The cert 
chain verifications done in these tests are supposed to fail because a CA
cert has been revoked, which can only be detected if the CRL is loaded.

So, the real bug here is that the CRLs are not being loaded.  I will file a
separate bug about that, and change this bug so that it only covers test case
4.13.19

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 139247 [details] [diff] [review]
patch v2 - from diff -uw 

Removing review request from this obsolete patch.  New patch forthcoming.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v3 - diff -uw, supplemental to patch v2 for bug 221644

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 139378 [details] [diff] [review]
patch v3 -  diff -uw, supplemental to patch v2 for bug 221644

This patch is to be applied after (on top of) patch v2 from bug 221644.
This patch modifies code added in that patch. 
Wan-Teh, please review.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 139228 [details] [diff] [review]
patch v1 - both unified and context diffs

remove old review request for this obsolete patch

Comment 15

[Bishakha Banerjee]

Attachment: test script with CRLs (for bug 231230 )

here is the revised script, with CRLs loaded, for the previous 6 sections. With
the CRLS loaded, the particular tests 2,5 and 7 pass. Test 4.13.38 still fails.


Some notes about the loading of CRLs for these tests. After discussing with
both Nelson and Julien, I chose to import the particular cert associated with
the CRL also into the database. I could have used the crlutil -B option to
bypass the check and inport the CRL anyway, but upon discussion with Julien, I
think it is more correct to the NIST tests not to do this.
Hence, when a particular test is over, I delete both the CRL and associated
cert from the database.

Another note about the loading of CRLs is that, one should run each NIST test
section in complete. This is because if test1 uses testCRL1, and test2 also
uses it, I do not delete testCRL1 from the database after the completion of
test1. However, if test3 does not use testCRL1, then it *is* deleted after
test2 is over.

The newer sections of the complete test suite (alongwith the modified previous
sections) to be attached to the original bug (177398) shortly.

Comment 16

[Wan-Teh Chang]

Comment on attachment 139378 [details] [diff] [review]
patch v3 -  diff -uw, supplemental to patch v2 for bug 221644

r=wtc.	Suggest two comment changes.

1. Explain why subjectIsSelfIssued has the value
PR_FALSE in the first iteration of the for loop.     

2. In this comment:

>+	/* This cert will be the subject cert in the next loop.

change "This cert" to "The issuer cert".

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

/cvsroot/mozilla/security/nss/lib/certhigh/certvfy.c,v  <--  certvfy.c
new revision: 1.39; previous revision: 1.38

Tbanks for the review.

Comment 18

[Bishakha Banerjee]

All name constarints tests pass now, pulled cvd this morning. Hence, marking
this verified.

I happened to have this bug open, I'm going to do likewise for the other NIST
bugs as well.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 139702 [details]
test script with CRLs (for bug 231230 )

(I think this attachment was intended for bug 231230 )

This latest version of pkits.sh puts all the certs into the cert db,
and then only supplies the leaf (EE) cert on the vfychain command line.
That does not simulate the actual behavior as used in client programs.
Please put ONLY the trusted CA certs (the ones with trust flags), and
those few others that are needed to import CLRs, into the certdb, and 
supply all other CA certs on the vfychain command line.
That's important for valid test results.