798 bytes, application/x-x509-ca-cert
774 bytes, application/x-x509-ca-cert
885 bytes, application/x-x509-ca-cert
935 bytes, patch
|Details | Diff | Splinter Review|
The namelist used to test against name constraints is built by CERT_GetConstrainedCertificateNames however this list excludes the common name when the number of DNS names is different than 0. This makes it trivial for a CA to issue certs that pass the namecontraints even tough they are technically constrained.
Created attachment 8347319 [details] [diff] [review] int-ca-nc-perm-example.com.der intermediate siged bu ca-nc, name constrainted to example.com
Created attachment 8347322 [details] cn-www.example.org-alt-example.com-int-ca-nc-perm-example.com.der EE: CN example.org altname=example.com
To reproduce: certutil -N /tmp/fail-demo/ certutil -d /tmp/fail-demo/ -A -i ca-nc.der -t 'C,,' vfychain -d /tmp/fail-demo/ -u 1 -v cn-www.example.org-alt-example.com-int-nc-perm-example.com-ca-nc.der int-nc-perm-example.com-ca-nc.der Chain is good!
Created attachment 8347325 [details] int-nc-perm-example.com-ca-nc.der oops, wront int
Attachment #8347319 - Attachment is obsolete: true
See also bug 394919. In particular, see bug 394919 comment 28 and comments following it.
See Also: → bug 394919
Created attachment 8347331 [details] [diff] [review] fix-nameconstraints2 Potential fix. I dont understand why is the explicit option gated by the presence or not of altDNS names.
Camilo, could you please package up your testcase as a patch to the chains.sh test and/or as a patch to the name constraint tests in Gecko, so that I can step through the code in a debugger to review it? Thanks.
(In reply to Camilo Viecco (:cviecco) from comment #9) > I dont understand why is the explicit option gated by the > presence or not of altDNS names. This may be by design. For SSL server certificate name matching, if the subjectAltName extension exists, we need to ignore the common name in the Subject field. See http://tools.ietf.org/html/rfc2818#section-3.1 If a subjectAltName extension of type dNSName is present, that MUST be used as the identity. Otherwise, the (most specific) Common Name field in the Subject field of the certificate MUST be used. Although the use of the Common Name is existing practice, it is deprecated and Certification Authorities are encouraged to use the dNSName instead. which is reiterated in http://tools.ietf.org/html/rfc6125#appendix-B.2
(In reply to Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if you want a response) from comment #10) > Camilo, could you please package up your testcase as a patch to the > chains.sh test and/or as a patch to the name constraint tests in Gecko, so > that I can step through the code in a debugger to review it? Thanks. Use tha patch from: https://bugzilla.mozilla.org/attachment.cgi?id=8346671 ./mach xpcshell-test security/manager/ssl/tests/unit/test_name_constraints.js The problematic cases are cn-www.example.org-alt-example.com-int-nc-perm-example.com-ca-nc (test8) cn-www.example.com-alt-example.org-int-nc-excl-example.com-ca-nc (test11). This does not affect firefox users as wihtout user invervention, (protected by CERT_VerifyCertName), but I believe in the case of these certs: cn= example.com alt=example.org with an intermediate that is name constrainted to example.com the chain verification should return failure not success. (ditto with the other test case cn=example.org, alt=example.com name contrains explicitly disallowing example.com). In the case of FF, the error should be fatal(name constrains, verificert fail) and not overridable (bad_cert_domain, CERT_VerifyCertName fail). My impression right now is that the fix in bug Bug 394919 is incomplete (but now I am carefuly reading http://tools.ietf.org/html/rfc6125).
(In reply to Wan-Teh Chang from comment #11) > This may be by design. For SSL server certificate name matching, > if the subjectAltName extension exists, we need to ignore the > common name in the Subject field. See > http://tools.ietf.org/html/rfc2818#section-3.1 > > If a subjectAltName extension of type dNSName is present, that MUST > be used as the identity. Otherwise, the (most specific) Common Name > field in the Subject field of the certificate MUST be used. Although > the use of the Common Name is existing practice, it is deprecated and > Certification Authorities are encouraged to use the dNSName instead. > > which is reiterated in > http://tools.ietf.org/html/rfc6125#appendix-B.2 Wan-Teh, I agree that it might actually not be a huge problem, as long as CERT_VerifyCertName ignores the CN when subjectAltNames are present. However, do we really want to consider the certificate to be a valid SSL certificate if it has a DNS name in the CN that doesn't meet the name constraints and the subjectAltName and the certificate has id-kp-serverAuth? I would say no. However, my understanding is that "example.org" in "CN=example.org" isn't supposed to be considered a hostname when there are any dNSNames (or ipAddress?) subjectAltNames, but rather just an arbitrary string. So, I think the severity of this should be gated on what CERT_VerifyCertName does when it encounters such a certificate. If CERT_VerifyCertName rejects the certificate, then this is either INVALID or a lower-priority, non-security bug, AFAICT.
CERT_VerifyCertName uses only the the cn only if there is no subjectAltNames. There is a problem here but not a critical one. Firefox users are not affected by this on SSL server verification. There is a case where for end endity case that are explicitly marked as server certs in the EKU that the current behavior is not preferred.
When a certificate has a subjectAltName extension with DNS names, the DNS name in the subject common name is intended for older applications that don't process subjectAltName. So, NSS doesn't need to check the subject common name for name constraint violation in this case. Unless we are trying to detect certificates that try to fool older applications, we don't need to perform the extra check. Does this make sense?
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.