Closed Bug 200225 Opened 21 years ago Closed 21 years ago

NSS reports untrusted built in CA certs as invalid CA certs

Categories

(NSS :: Libraries, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nelson, Assigned: nelson)

Details

Attachments

(1 file, 1 obsolete file)

The command
certutil -V -d dbdir -h "Root Certs" -n "mycert" -u C 
checks the cert nicknamed "mycert" in dbdir/cert8.db to see if it is valid 
as an SSL client auth cert.  It checks the chain up to the root.  In my test
case, "mycert" was issued by an intermediate CA whose cert is also in cert8.db.
That intermediate CA cert was issued by a trusted root CA in 
dbdir/nssckbi.dll, but the root CA is not marked trusted to issue SSL client 
auth certs in that DLL.

certutil reports "invalid CA cert", even though all the certs involved are
completely valid CA certs.  The cert that is generating the error is the 
root CA cert.  We certainly don't want NSS to report that the built-in root 
CA certs are invalid!

The error code it should be reporting is "untrusted CA cert", since the real
problem is that the root CA cert is not trusted to issue SSL client certs.

The problem occurs because the root CA cert has no basic constraints extension.
If the cert was trusted for the usage we're testing for, cert_VerifyCertChain 
would be OK with this cert, but because it is not trusted for that usage, 
the function doesn't even recognize that it is a root CA cert at all.

The following one-line patch fixes this problem, but may not be correct in
all situations.  Bob, what do you think?

Index: certhigh/certvfy.c
===================================================================
RCS file: /cvsroot/mozilla/security/nss/lib/certhigh/certvfy.c,v
retrieving revision 1.30
diff -u -r1.30 certvfy.c
--- certhigh/certvfy.c  19 Dec 2002 00:26:27 -0000      1.30
+++ certhigh/certvfy.c  2 Apr 2003 04:50:46 -0000
@@ -831,7 +831,7 @@
            /* no basic constraints found, if we're fortezza, CA bit is already
             * verified (isca = PR_TRUE). otherwise, we aren't (yet) a ca
             * isca = PR_FALSE */
-           isca = isFortezzaV1;
+           isca = isFortezzaV1 || issuerCert->isRoot;
        } else  {
            if ( basicConstraint.isCA == PR_FALSE ) {
                PORT_SetError (SEC_ERROR_CA_CERT_INVALID);
Setting target to 3.8, but this may be too risky for inclusion this close 
to RTM.
Priority: -- → P1
Target Milestone: --- → 3.8
Additional info:

cert->isRoot is set in only one place.  It is set in function
CERT_DecodeDERCertificate() to the value returned by the function
cert_IsRootCert(cert).  

Does that make he risk of this patch acceptable for NSS 3.8?
I'd appreciate help in deciding whether or not to check this in, 
before 5 PM today.  
Assignee: wtc → nelsonb
Retargetting to 3.8.1.  
Status: NEW → ASSIGNED
Target Milestone: 3.8 → 3.8.1
I checked in the above patch on the trunk in April (rev 1.31).  
AFAIK, no release of NSS made since then has incorporated that change.
NSS 3.8 branched at revision 1.30.  

While this workaround is expedient, I believe it is incorrect.  RFC 3280 
exempts self-issued CA certs from certain requirements, but not from the 
requirement to have a basic-constraints extension.  I intend to remove 
this workaround before we ship NSS 3.9.

The real problem here seems to be that marking a cert as trusted ("C") or 
as a "valid CA" ("c") for SSL server auth should also imply that it is a 
"valid CA" (though not necessarily trusted) for SSL client auth use, but
presently does not.  This is an artifact of the way that SSL client auth
trust was implemented, as an irregular addition to SSL server auth, 
rather than as a new class of trust.  OCSP trust has a similar problem.
Target Milestone: 3.8.1 → 3.9
Attached patch patch v2 (obsolete) — Splinter Review
This patch backs out the previous change (rev 1.31), and attempts to clean
up and clarify the behavior of some of the special cases in the relevant code.
Finally, to address the subject of this bug, it adds a goto skip_ca_checks
for the case of a cert that is marked as a valid CA cert.  That trust flag
obviates any other ca validation checks.
Comment on attachment 132022 [details] [diff] [review]
patch v2

Bob and Wan-Teh, please review this code to confirm that it works the same as
before for the special certUsageStatusResponder case, and that it is correct
for the valid CA case.
Attachment #132022 - Flags: superreview?(wchang0222)
Attachment #132022 - Flags: review?(rrelyea0264)
Comment on attachment 132022 [details] [diff] [review]
patch v2

I gave my comments to Nelson offline.
Attachment #132022 - Flags: superreview?(wchang0222)
Attachment #132022 - Flags: superreview-
Attachment #132022 - Flags: review?(rrelyea0264)
Attached patch patch v2Splinter Review
Attachment #132022 - Attachment is obsolete: true
Comment on attachment 132550 [details] [diff] [review]
patch v2

This patch fixes one logic flaw in the previous patch.	
Also, unlike the v1 patch, this patch adds no gotos.
Attachment #132550 - Flags: superreview?(wchang0222)
Comment on attachment 132550 [details] [diff] [review]
patch v2

r=wtc.

In the first function (cert_VerifyCertChain), could you indent the
code after adding "if (!validCAOverride) { ... }" around it?
Attachment #132550 - Flags: superreview?(wchang0222) → superreview+
I made the indentation changes Wan-Teh suggested, and checked it in.

/cvsroot/mozilla/security/nss/lib/certhigh/certvfy.c,v  <--  certvfy.c
new revision: 1.36; previous revision: 1.35
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: