Backed out changeset f97578949399 (bug 1032947)

security/pkix/lib/pkixbuild.cpp

@@ -406,4 +406,13 @@ BuildCertChain(TrustDomain& trustDomain,
   return SECSuccess;
 }
 
+PLArenaPool*
+BackCert::GetArena()
+{
+  if (!arena) {
+    arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
+  }
+  return arena.get();
+}
+
 } } // namespace mozilla::pkix

security/pkix/lib/pkixcheck.cpp

@@ -404,49 +404,56 @@ CheckBasicConstraints(EndEntityOrCA endEntityOrCA,
   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

security/pkix/lib/pkixcheck.h

@@ -42,7 +42,7 @@ Result CheckIssuerIndependentProperties(
           unsigned int subCACount,
           /*optional out*/ TrustLevel* trustLevel = nullptr);
 
-Result CheckNameConstraints(const BackCert& cert);
+Result CheckNameConstraints(BackCert& cert);
 
 } } // namespace mozilla::pkix
 

security/pkix/lib/pkixutil.h

@@ -90,9 +90,9 @@ MapSECStatus(SECStatus srv)
 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
@@ -105,6 +105,7 @@ public:
     , encodedNameConstraints(nullptr)
     , encodedInhibitAnyPolicy(nullptr)
     , childCert(childCert)
+    , constrainedNames(nullptr)
     , includeCN(includeCN)
   {
   }
@@ -134,7 +135,6 @@ public:
   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
@@ -143,9 +143,21 @@ public:
   // 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 */;
 };