Backed out changeset f97578949399 (bug 1032947)
authorWes Kocher <wkocher@mozilla.com>
Tue, 01 Jul 2014 17:43:33 -0700
changeset 191813 fc79d63461d28c705d47fdd30145e62a7a3d469b
parent 191812 84131ac42e0e497b9fdf009a549fe33751886b98
child 191814 7b0a6c01c4296ec6016eebe0c6701675856487c1
push id45666
push userkwierso@gmail.com
push dateWed, 02 Jul 2014 00:45:08 +0000
treeherdermozilla-inbound@58806b74674b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1032947
milestone33.0a1
backs outf97578949399bc6b5c768677cb7486ceb5456609
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
Backed out changeset f97578949399 (bug 1032947)
security/pkix/lib/pkixbuild.cpp
security/pkix/lib/pkixcheck.cpp
security/pkix/lib/pkixcheck.h
security/pkix/lib/pkixutil.h
--- a/security/pkix/lib/pkixbuild.cpp
+++ b/security/pkix/lib/pkixbuild.cpp
@@ -401,9 +401,18 @@ BuildCertChain(TrustDomain& trustDomain,
   if (rv != Success) {
     results = nullptr;
     return SECFailure;
   }
 
   return SECSuccess;
 }
 
+PLArenaPool*
+BackCert::GetArena()
+{
+  if (!arena) {
+    arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
+  }
+  return arena.get();
+}
+
 } } // namespace mozilla::pkix
--- a/security/pkix/lib/pkixcheck.cpp
+++ b/security/pkix/lib/pkixcheck.cpp
@@ -399,59 +399,66 @@ CheckBasicConstraints(EndEntityOrCA endE
   if (pathLenConstraint >= 0 &&
       static_cast<long>(subCACount) > pathLenConstraint) {
     return Fail(RecoverableError, SEC_ERROR_PATH_LEN_CONSTRAINT_INVALID);
   }
 
   return Success;
 }
 
+Result
+BackCert::GetConstrainedNames(/*out*/ const CERTGeneralName** result)
+{
+  if (!constrainedNames) {
+    if (!GetArena()) {
+      return FatalError;
+    }
+
+    constrainedNames =
+      CERT_GetConstrainedCertificateNames(GetNSSCert(), arena.get(),
+                                          includeCN == IncludeCN::Yes);
+    if (!constrainedNames) {
+      return MapSECStatus(SECFailure);
+    }
+  }
+
+  *result = constrainedNames;
+  return Success;
+}
+
 // 4.2.1.10. Name Constraints
 Result
-CheckNameConstraints(const BackCert& cert)
+CheckNameConstraints(BackCert& cert)
 {
   if (!cert.encodedNameConstraints) {
     return Success;
   }
 
-  ScopedPLArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE));
+  PLArenaPool* arena = cert.GetArena();
   if (!arena) {
-    return MapSECStatus(SECFailure);
+    return FatalError;
   }
 
   // Owned by arena
   const CERTNameConstraints* constraints =
-    CERT_DecodeNameConstraintsExtension(arena.get(), cert.encodedNameConstraints);
+    CERT_DecodeNameConstraintsExtension(arena, cert.encodedNameConstraints);
   if (!constraints) {
     return MapSECStatus(SECFailure);
   }
 
-  for (const BackCert* child = cert.childCert; child;
-       child = child->childCert) {
-    ScopedCERTCertificate
-      nssCert(CERT_NewTempCertificate(CERT_GetDefaultCertDB(),
-                                      const_cast<SECItem*>(&child->GetDER()),
-                                      nullptr, false, true));
-    if (!nssCert) {
-      return MapSECStatus(SECFailure);
+  for (BackCert* prev = cert.childCert; prev; prev = prev->childCert) {
+    const CERTGeneralName* names = nullptr;
+    Result rv = prev->GetConstrainedNames(&names);
+    if (rv != Success) {
+      return rv;
     }
-
-    bool includeCN = child->includeCN == BackCert::IncludeCN::Yes;
-    // owned by arena
-    const CERTGeneralName*
-      names(CERT_GetConstrainedCertificateNames(nssCert.get(), arena.get(),
-                                                includeCN));
-    if (!names) {
-      return MapSECStatus(SECFailure);
-    }
-
+    PORT_Assert(names);
     CERTGeneralName* currentName = const_cast<CERTGeneralName*>(names);
     do {
-      if (CERT_CheckNameSpace(arena.get(), constraints, currentName)
-            != SECSuccess) {
+      if (CERT_CheckNameSpace(arena, constraints, currentName) != SECSuccess) {
         // XXX: It seems like CERT_CheckNameSpace doesn't always call
         // PR_SetError when it fails. We set the error code here, though this
         // may be papering over some fatal errors. NSS's
         // cert_VerifyCertChainOld does something similar.
         return Fail(RecoverableError, SEC_ERROR_CERT_NOT_IN_NAME_SPACE);
       }
       currentName = CERT_GetNextGeneralName(currentName);
     } while (currentName != names);
--- a/security/pkix/lib/pkixcheck.h
+++ b/security/pkix/lib/pkixcheck.h
@@ -37,13 +37,13 @@ Result CheckIssuerIndependentProperties(
           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(BackCert& cert);
 
 } } // namespace mozilla::pkix
 
 #endif // mozilla_pkix__pkixcheck_h
--- a/security/pkix/lib/pkixutil.h
+++ b/security/pkix/lib/pkixutil.h
@@ -85,31 +85,32 @@ 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.
+  // IncludeCN::No means that GetConstrainedNames won't include the subject CN
+  // in its results. IncludeCN::Yes means that GetConstrainedNames will include
+  // the subject CN in its results.
   MOZILLA_PKIX_ENUM_CLASS IncludeCN { No = 0, Yes = 1 };
 
   // nssCert and childCert must be valid for the lifetime of BackCert
   BackCert(BackCert* childCert, IncludeCN includeCN)
     : encodedAuthorityInfoAccess(nullptr)
     , encodedBasicConstraints(nullptr)
     , encodedCertificatePolicies(nullptr)
     , encodedExtendedKeyUsage(nullptr)
     , encodedKeyUsage(nullptr)
     , encodedNameConstraints(nullptr)
     , encodedInhibitAnyPolicy(nullptr)
     , childCert(childCert)
+    , constrainedNames(nullptr)
     , includeCN(includeCN)
   {
   }
 
   Result Init(const SECItem& certDER);
 
   const SECItem& GetDER() const { return nssCert->derCert; }
   const SECItem& GetIssuer() const { return nssCert->derIssuer; }
@@ -129,27 +130,38 @@ public:
   const SECItem* encodedBasicConstraints;
   const SECItem* encodedCertificatePolicies;
   const SECItem* encodedExtendedKeyUsage;
   const SECItem* encodedKeyUsage;
   const SECItem* encodedNameConstraints;
   const SECItem* encodedInhibitAnyPolicy;
 
   BackCert* const childCert;
-  const IncludeCN includeCN;
 
   // Only non-const so that we can pass this to TrustDomain::IsRevoked,
   // which only takes a non-const pointer because VerifyEncodedOCSPResponse
   // requires it, and that is only because the implementation of
   // VerifyEncodedOCSPResponse does a CERT_DupCertificate. TODO: get rid
   // of that CERT_DupCertificate so that we can make all these things const.
   /*const*/ CERTCertificate* GetNSSCert() const { return nssCert.get(); }
 
+  // Returns the names that should be considered when evaluating name
+  // constraints. The list is constructed lazily and cached. The result is a
+  // weak reference; do not try to free it, and do not hold long-lived
+  // references to it.
+  Result GetConstrainedNames(/*out*/ const CERTGeneralName** result);
+
+  PLArenaPool* GetArena();
+
 private:
   ScopedCERTCertificate nssCert;
 
+  ScopedPLArenaPool arena;
+  CERTGeneralName* constrainedNames;
+  IncludeCN includeCN;
+
   BackCert(const BackCert&) /* = delete */;
   void operator=(const BackCert&); /* = delete */;
 };
 
 } } // namespace mozilla::pkix
 
 #endif // mozilla_pkix__pkixutil_h