Bug 1032947: Change CheckNameConstraints to construct CERTCertificate instances when needed, r=keeler

--HG--
extra : rebase_source : d0bf802f4ff3fe9900ed7444c046617aa27faea9

security/pkix/lib/pkixbuild.cpp

@@ -406,13 +406,4 @@ 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,56 +404,49 @@ 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(BackCert& cert)
+CheckNameConstraints(const BackCert& cert)
 {
   if (!cert.encodedNameConstraints) {
     return Success;
   }
 
-  PLArenaPool* arena = cert.GetArena();
+  ScopedPLArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE));
   if (!arena) {
-    return FatalError;
+    return MapSECStatus(SECFailure);
   }
 
   // Owned by arena
   const CERTNameConstraints* constraints =
-    CERT_DecodeNameConstraintsExtension(arena, cert.encodedNameConstraints);
+    CERT_DecodeNameConstraintsExtension(arena.get(), cert.encodedNameConstraints);
   if (!constraints) {
     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;
+  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);
     }
-    PORT_Assert(names);
+
+    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);
+    }
+
     CERTGeneralName* currentName = const_cast<CERTGeneralName*>(names);
     do {
-      if (CERT_CheckNameSpace(arena, constraints, currentName) != SECSuccess) {
+      if (CERT_CheckNameSpace(arena.get(), 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(BackCert& cert);
+Result CheckNameConstraints(const BackCert& cert);
 
 } } // namespace mozilla::pkix
 

security/pkix/lib/pkixutil.h

@@ -90,9 +90,9 @@ MapSECStatus(SECStatus srv)
 class BackCert
 {
 public:
-  // 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.
+  // 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 };
 
   // nssCert and childCert must be valid for the lifetime of BackCert
@@ -105,7 +105,6 @@ public:
     , encodedNameConstraints(nullptr)
     , encodedInhibitAnyPolicy(nullptr)
     , childCert(childCert)
-    , constrainedNames(nullptr)
     , includeCN(includeCN)
   {
   }
@@ -135,6 +134,7 @@ 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,21 +143,9 @@ 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 */;
 };