Bug 1035942: Decide whether to consider end-entity CN as a dnsName in CheckNameConstraints instead of in BuildCertChain, r=cviecco
authorBrian Smith <brian@briansmith.org>
Tue, 08 Jul 2014 13:04:17 -0700
changeset 193102 3a4d57c7ffdf74dae7e1c3e1cdadba44750dab15
parent 193101 0ed88d692f42f34802beafcea77797f61c918155
child 193103 beb201c2abbdd319c6bbd999ee43c0634742b219
push id46020
push userbrian@briansmith.org
push dateWed, 09 Jul 2014 17:41:36 +0000
treeherdermozilla-inbound@3a4d57c7ffdf [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerscviecco
bugs1035942
milestone33.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1035942: Decide whether to consider end-entity CN as a dnsName in CheckNameConstraints instead of in BuildCertChain, r=cviecco
security/pkix/lib/pkixbuild.cpp
security/pkix/lib/pkixcheck.cpp
security/pkix/lib/pkixcheck.h
security/pkix/lib/pkixocsp.cpp
security/pkix/lib/pkixutil.h
--- a/security/pkix/lib/pkixbuild.cpp
+++ b/security/pkix/lib/pkixbuild.cpp
@@ -28,41 +28,38 @@
 
 #include "pkixcheck.h"
 
 namespace mozilla { namespace pkix {
 
 static Result BuildForward(TrustDomain& trustDomain,
                            const BackCert& subject,
                            PRTime time,
-                           EndEntityOrCA endEntityOrCA,
                            KeyUsage requiredKeyUsageIfPresent,
                            KeyPurposeId requiredEKUIfPresent,
                            const CertPolicyId& requiredPolicy,
                            /*optional*/ const SECItem* stapledOCSPResponse,
                            unsigned int subCACount);
 
 TrustDomain::IssuerChecker::IssuerChecker() { }
 TrustDomain::IssuerChecker::~IssuerChecker() { }
 
 // The implementation of TrustDomain::IssuerTracker is in a subclass only to
 // hide the implementation from external users.
 class PathBuildingStep : public TrustDomain::IssuerChecker
 {
 public:
   PathBuildingStep(TrustDomain& trustDomain, const BackCert& subject,
-                   PRTime time, EndEntityOrCA endEntityOrCA,
-                   KeyPurposeId requiredEKUIfPresent,
+                   PRTime time, KeyPurposeId requiredEKUIfPresent,
                    const CertPolicyId& requiredPolicy,
                    /*optional*/ const SECItem* stapledOCSPResponse,
                    unsigned int subCACount)
     : trustDomain(trustDomain)
     , subject(subject)
     , time(time)
-    , endEntityOrCA(endEntityOrCA)
     , requiredEKUIfPresent(requiredEKUIfPresent)
     , requiredPolicy(requiredPolicy)
     , stapledOCSPResponse(stapledOCSPResponse)
     , subCACount(subCACount)
     , result(SEC_ERROR_LIBRARY_FAILURE)
     , resultWasSet(false)
   {
   }
@@ -71,17 +68,16 @@ public:
                   /*out*/ bool& keepGoing);
 
   Result CheckResult() const;
 
 private:
   TrustDomain& trustDomain;
   const BackCert& subject;
   const PRTime time;
-  const EndEntityOrCA endEntityOrCA;
   const KeyPurposeId requiredEKUIfPresent;
   const CertPolicyId& requiredPolicy;
   /*optional*/ SECItem const* const stapledOCSPResponse;
   const unsigned int subCACount;
 
   SECStatus RecordResult(PRErrorCode currentResult, /*out*/ bool& keepGoing);
   PRErrorCode result;
   bool resultWasSet;
@@ -131,18 +127,18 @@ PathBuildingStep::CheckResult() const
   return MapSECStatus(SECFailure);
 }
 
 // The code that executes in the inner loop of BuildForward
 SECStatus
 PathBuildingStep::Check(const SECItem& potentialIssuerDER,
                         /*out*/ bool& keepGoing)
 {
-  BackCert potentialIssuer(potentialIssuerDER, &subject,
-                           BackCert::IncludeCN::No);
+  BackCert potentialIssuer(potentialIssuerDER, EndEntityOrCA::MustBeCA,
+                           &subject);
   Result rv = potentialIssuer.Init();
   if (rv != Success) {
     return RecordResult(PR_GetError(), keepGoing);
   }
 
   // RFC5280 4.2.1.1. Authority Key Identifier
   // RFC5280 4.2.1.2. Subject Key Identifier
 
@@ -156,41 +152,40 @@ PathBuildingStep::Check(const SECItem& p
                               &prev->GetSubjectPublicKeyInfo()) &&
         SECITEM_ItemsAreEqual(&potentialIssuer.GetSubject(),
                               &prev->GetSubject())) {
       // XXX: error code
       return RecordResult(SEC_ERROR_UNKNOWN_ISSUER, keepGoing);
     }
   }
 
-  rv = CheckNameConstraints(potentialIssuer);
+  rv = CheckNameConstraints(potentialIssuer, requiredEKUIfPresent);
   if (rv != Success) {
     return RecordResult(PR_GetError(), keepGoing);
   }
 
   // RFC 5280, Section 4.2.1.3: "If the keyUsage extension is present, then the
   // subject public key MUST NOT be used to verify signatures on certificates
   // or CRLs unless the corresponding keyCertSign or cRLSign bit is set."
-  rv = BuildForward(trustDomain, potentialIssuer, time, EndEntityOrCA::MustBeCA,
-                    KeyUsage::keyCertSign, requiredEKUIfPresent,
-                    requiredPolicy, nullptr, subCACount);
+  rv = BuildForward(trustDomain, potentialIssuer, time, KeyUsage::keyCertSign,
+                    requiredEKUIfPresent, requiredPolicy, nullptr, subCACount);
   if (rv != Success) {
     return RecordResult(PR_GetError(), keepGoing);
   }
 
   SECStatus srv = trustDomain.VerifySignedData(
                                 subject.GetSignedData(),
                                 potentialIssuer.GetSubjectPublicKeyInfo());
   if (srv != SECSuccess) {
     return RecordResult(PR_GetError(), keepGoing);
   }
 
   CertID certID(subject.GetIssuer(), potentialIssuer.GetSubjectPublicKeyInfo(),
                 subject.GetSerialNumber());
-  srv = trustDomain.CheckRevocation(endEntityOrCA, certID, time,
+  srv = trustDomain.CheckRevocation(subject.endEntityOrCA, certID, time,
                                     stapledOCSPResponse,
                                     subject.GetAuthorityInfoAccess());
   if (srv != SECSuccess) {
     return RecordResult(PR_GetError(), keepGoing);
   }
 
   return RecordResult(0/*PRErrorCode::success*/, keepGoing);
 }
@@ -200,37 +195,35 @@ PathBuildingStep::Check(const SECItem& p
 // Be very careful about changing the order of checks. The order is significant
 // because it affects which error we return when a certificate or certificate
 // chain has multiple problems. See the error ranking documentation in
 // pkix/pkix.h.
 static Result
 BuildForward(TrustDomain& trustDomain,
              const BackCert& subject,
              PRTime time,
-             EndEntityOrCA endEntityOrCA,
              KeyUsage requiredKeyUsageIfPresent,
              KeyPurposeId requiredEKUIfPresent,
              const CertPolicyId& requiredPolicy,
              /*optional*/ const SECItem* stapledOCSPResponse,
              unsigned int subCACount)
 {
   Result rv;
 
   TrustLevel trustLevel;
   // If this is an end-entity and not a trust anchor, we defer reporting
   // any error found here until after attempting to find a valid chain.
   // See the explanation of error prioritization in pkix.h.
   rv = CheckIssuerIndependentProperties(trustDomain, subject, time,
-                                        endEntityOrCA,
                                         requiredKeyUsageIfPresent,
                                         requiredEKUIfPresent, requiredPolicy,
                                         subCACount, &trustLevel);
   PRErrorCode deferredEndEntityError = 0;
   if (rv != Success) {
-    if (endEntityOrCA == EndEntityOrCA::MustBeEndEntity &&
+    if (subject.endEntityOrCA == EndEntityOrCA::MustBeEndEntity &&
         trustLevel != TrustLevel::TrustAnchor) {
       deferredEndEntityError = PR_GetError();
     } else {
       return rv;
     }
   }
 
   if (trustLevel == TrustLevel::TrustAnchor) {
@@ -250,34 +243,34 @@ BuildForward(TrustDomain& trustDomain,
     SECStatus srv = trustDomain.IsChainValid(chain);
     if (srv != SECSuccess) {
       return MapSECStatus(srv);
     }
 
     return Success;
   }
 
-  if (endEntityOrCA == EndEntityOrCA::MustBeCA) {
+  if (subject.endEntityOrCA == EndEntityOrCA::MustBeCA) {
     // Avoid stack overflows and poor performance by limiting cert chain
     // length.
     static const unsigned int MAX_SUBCA_COUNT = 6;
     static_assert(1/*end-entity*/ + MAX_SUBCA_COUNT + 1/*root*/ ==
                   NonOwningDERArray::MAX_LENGTH,
                   "MAX_SUBCA_COUNT and NonOwningDERArray::MAX_LENGTH mismatch.");
     if (subCACount >= MAX_SUBCA_COUNT) {
       return Fail(RecoverableError, SEC_ERROR_UNKNOWN_ISSUER);
     }
     ++subCACount;
   } else {
     PR_ASSERT(subCACount == 0);
   }
 
   // Find a trusted issuer.
 
-  PathBuildingStep pathBuilder(trustDomain, subject, time, endEntityOrCA,
+  PathBuildingStep pathBuilder(trustDomain, subject, time,
                                requiredEKUIfPresent, requiredPolicy,
                                stapledOCSPResponse, subCACount);
 
   // TODO(bug 965136): Add SKI/AKI matching optimizations
   if (trustDomain.FindIssuer(subject.GetIssuer(), pathBuilder, time)
         != SECSuccess) {
     return MapSECStatus(SECFailure);
   }
@@ -302,31 +295,25 @@ BuildCertChain(TrustDomain& trustDomain,
                PRTime time, EndEntityOrCA endEntityOrCA,
                KeyUsage requiredKeyUsageIfPresent,
                KeyPurposeId requiredEKUIfPresent,
                const CertPolicyId& requiredPolicy,
                /*optional*/ const SECItem* stapledOCSPResponse)
 {
   // XXX: Support the legacy use of the subject CN field for indicating the
   // domain name the certificate is valid for.
-  BackCert::IncludeCN includeCN
-    = endEntityOrCA == EndEntityOrCA::MustBeEndEntity &&
-      requiredEKUIfPresent == KeyPurposeId::id_kp_serverAuth
-    ? BackCert::IncludeCN::Yes
-    : BackCert::IncludeCN::No;
-
-  BackCert cert(certDER, nullptr, includeCN);
+  BackCert cert(certDER, endEntityOrCA, nullptr);
   Result rv = cert.Init();
   if (rv != Success) {
     return SECFailure;
   }
 
-  rv = BuildForward(trustDomain, cert, time, endEntityOrCA,
-                    requiredKeyUsageIfPresent, requiredEKUIfPresent,
-                    requiredPolicy, stapledOCSPResponse, 0);
+  rv = BuildForward(trustDomain, cert, time, requiredKeyUsageIfPresent,
+                    requiredEKUIfPresent, requiredPolicy, stapledOCSPResponse,
+                    0/*subCACount*/);
   if (rv != Success) {
     return SECFailure;
   }
 
   return SECSuccess;
 }
 
 } } // namespace mozilla::pkix
--- a/security/pkix/lib/pkixcheck.cpp
+++ b/security/pkix/lib/pkixcheck.cpp
@@ -407,17 +407,17 @@ CheckBasicConstraints(EndEntityOrCA endE
 inline void
 PORT_FreeArena_false(PLArenaPool* arena) {
   // PL_FreeArenaPool can't be used because it doesn't actually free the
   // memory, which doesn't work well with memory analysis tools
   return PORT_FreeArena(arena, PR_FALSE);
 }
 
 Result
-CheckNameConstraints(const BackCert& cert)
+CheckNameConstraints(const BackCert& cert, KeyPurposeId requiredEKUIfPresent)
 {
   // These hardcoded consts are to handle a post certificate creation
   // name constraints. In this case for ANSSI.
   static const char constraintFranceGov[] =
                                      "\x30\x5D" /* sequence len 93*/
                                      "\xA0\x5B" /* element len 91 */
                                      "\x30\x05" /* sequence len 5 */
                                      "\x82\x03" /* entry len 3 */
@@ -505,17 +505,18 @@ CheckNameConstraints(const BackCert& cer
     ScopedPtr<CERTCertificate, CERT_DestroyCertificate>
       nssCert(CERT_NewTempCertificate(CERT_GetDefaultCertDB(),
                                       const_cast<SECItem*>(&child->GetDER()),
                                       nullptr, false, true));
     if (!nssCert) {
       return MapSECStatus(SECFailure);
     }
 
-    bool includeCN = child->includeCN == BackCert::IncludeCN::Yes;
+    bool includeCN = child->endEntityOrCA == EndEntityOrCA::MustBeEndEntity &&
+                     requiredEKUIfPresent == KeyPurposeId::id_kp_serverAuth;
     // owned by arena
     const CERTGeneralName*
       names(CERT_GetConstrainedCertificateNames(nssCert.get(), arena.get(),
                                                 includeCN));
     if (!names) {
       return MapSECStatus(SECFailure);
     }
 
@@ -692,25 +693,26 @@ CheckExtendedKeyUsage(EndEntityOrCA endE
 
   return Success;
 }
 
 Result
 CheckIssuerIndependentProperties(TrustDomain& trustDomain,
                                  const BackCert& cert,
                                  PRTime time,
-                                 EndEntityOrCA endEntityOrCA,
                                  KeyUsage requiredKeyUsageIfPresent,
                                  KeyPurposeId requiredEKUIfPresent,
                                  const CertPolicyId& requiredPolicy,
                                  unsigned int subCACount,
                 /*optional out*/ TrustLevel* trustLevelOut)
 {
   Result rv;
 
+  const EndEntityOrCA endEntityOrCA = cert.endEntityOrCA;
+
   TrustLevel trustLevel;
   rv = MapSECStatus(trustDomain.GetCertTrust(endEntityOrCA, requiredPolicy,
                                              cert.GetDER(), &trustLevel));
   if (rv != Success) {
     return rv;
   }
   if (trustLevel == TrustLevel::ActivelyDistrusted) {
     return Fail(RecoverableError, SEC_ERROR_UNTRUSTED_CERT);
--- a/security/pkix/lib/pkixcheck.h
+++ b/security/pkix/lib/pkixcheck.h
@@ -30,20 +30,20 @@
 #include "certt.h"
 
 namespace mozilla { namespace pkix {
 
 Result CheckIssuerIndependentProperties(
           TrustDomain& trustDomain,
           const BackCert& cert,
           PRTime time,
-          EndEntityOrCA endEntityOrCA,
           KeyUsage requiredKeyUsageIfPresent,
           KeyPurposeId requiredEKUIfPresent,
           const CertPolicyId& requiredPolicy,
           unsigned int subCACount,
           /*optional out*/ TrustLevel* trustLevel = nullptr);
 
-Result CheckNameConstraints(const BackCert& cert);
+Result CheckNameConstraints(const BackCert& cert,
+                            KeyPurposeId requiredEKUIfPresent);
 
 } } // namespace mozilla::pkix
 
 #endif // mozilla_pkix__pkixcheck_h
--- a/security/pkix/lib/pkixocsp.cpp
+++ b/security/pkix/lib/pkixocsp.cpp
@@ -133,17 +133,16 @@ CheckOCSPResponseSignerCert(TrustDomain&
   // Note that CheckIssuerIndependentProperties processes
   // SEC_OID_OCSP_RESPONDER in the way that the OCSP specification requires us
   // to--in particular, it doesn't allow SEC_OID_OCSP_RESPONDER to be implied
   // by a missing EKU extension, unlike other EKUs.
   //
   // TODO(bug 926261): If we're validating for a policy then the policy OID we
   // are validating for should be passed to CheckIssuerIndependentProperties.
   rv = CheckIssuerIndependentProperties(trustDomain, potentialSigner, time,
-                                        EndEntityOrCA::MustBeEndEntity,
                                         KeyUsage::noParticularKeyUsageRequired,
                                         KeyPurposeId::id_kp_OCSPSigning,
                                         CertPolicyId::anyPolicy, 0);
   if (rv != Success) {
     return rv;
   }
 
   // It is possible that there exists a certificate with the same key as the
@@ -267,17 +266,17 @@ VerifySignature(Context& context, Respon
     return rv;
   }
   if (match) {
     return VerifyOCSPSignedData(context.trustDomain, signedResponseData,
                                 context.certID.issuerSubjectPublicKeyInfo);
   }
 
   for (size_t i = 0; i < numCerts; ++i) {
-    BackCert cert(certs[i], nullptr, BackCert::IncludeCN::No);
+    BackCert cert(certs[i], EndEntityOrCA::MustBeEndEntity, nullptr);
     rv = cert.Init();
     if (rv != Success) {
       return rv;
     }
     rv = MatchResponderID(responderIDType, responderID,
                           cert.GetSubject(), cert.GetSubjectPublicKeyInfo(),
                           match);
     if (rv == FatalError) {
--- a/security/pkix/lib/pkixutil.h
+++ b/security/pkix/lib/pkixutil.h
@@ -85,27 +85,22 @@ MapSECStatus(SECStatus srv)
 // such as name constraints.
 //
 // Each BackCert contains pointers to all the given certificate's extensions
 // so that we can parse the extension block once and then process the
 // extensions in an order that may be different than they appear in the cert.
 class BackCert
 {
 public:
-  // IncludeCN::No means that name constraint enforcement should not consider
-  // the subject CN as a possible dNSName. IncludeCN::Yes means that name
-  // constraint enforcement will consider the subject CN as a possible dNSName.
-  MOZILLA_PKIX_ENUM_CLASS IncludeCN { No = 0, Yes = 1 };
-
   // certDER and childCert must be valid for the lifetime of BackCert.
-  BackCert(const SECItem& certDER, const BackCert* childCert,
-           IncludeCN includeCN)
+  BackCert(const SECItem& certDER, EndEntityOrCA endEntityOrCA,
+           const BackCert* childCert)
     : der(certDER)
+    , endEntityOrCA(endEntityOrCA)
     , childCert(childCert)
-    , includeCN(includeCN)
   {
   }
 
   Result Init();
 
   const SECItem& GetDER() const { return der; }
   const der::Version GetVersion() const { return version; }
   const CERTSignedData& GetSignedData() const { return signedData; }
@@ -142,18 +137,18 @@ public:
   {
     return MaybeSECItem(nameConstraints);
   }
 
 private:
   const SECItem& der;
 
 public:
+  const EndEntityOrCA endEntityOrCA;
   BackCert const* const childCert;
-  const IncludeCN includeCN;
 
 private:
   der::Version version;
 
   // When parsing certificates in BackCert::Init, we don't accept empty
   // extensions. Consequently, we don't have to store a distinction between
   // empty extensions and extensions that weren't included. However, when
   // *processing* extensions, we distinguish between whether an extension was