Bug 1020682: Simplify mozilla::pkix results cert chain construction and make it more efficient, r=cviecco
authorBrian Smith <brian@briansmith.org>
Wed, 04 Jun 2014 01:28:44 -0700
changeset 206534 77f2f8f2c506459e82a1d0cc34dccb605fddc214
parent 206533 dc9d168ba8fb8366f875a2556bcacd06bacf5e75
child 206535 dea1f56f3feb0fa437a9449ef8907455af235056
push id3741
push userasasaki@mozilla.com
push dateMon, 21 Jul 2014 20:25:18 +0000
treeherdermozilla-beta@4d6f46f5af68 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerscviecco
bugs1020682
milestone32.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 1020682: Simplify mozilla::pkix results cert chain construction and make it more efficient, r=cviecco
security/pkix/lib/pkixbuild.cpp
security/pkix/lib/pkixutil.h
--- a/security/pkix/lib/pkixbuild.cpp
+++ b/security/pkix/lib/pkixbuild.cpp
@@ -226,48 +226,40 @@ BuildForward(TrustDomain& trustDomain,
         trustLevel != TrustLevel::TrustAnchor) {
       deferredEndEntityError = PR_GetError();
     } else {
       return rv;
     }
   }
 
   if (trustLevel == TrustLevel::TrustAnchor) {
-    ScopedCERTCertList certChain(CERT_NewCertList());
-    if (!certChain) {
-      PR_SetError(SEC_ERROR_NO_MEMORY, 0);
+    // End of the recursion.
+
+    // Construct the results cert chain.
+    results = CERT_NewCertList();
+    if (!results) {
       return MapSECStatus(SECFailure);
     }
+    for (BackCert* cert = &subject; cert; cert = cert->childCert) {
+      CERTCertificate* dup = CERT_DupCertificate(cert->GetNSSCert());
+      if (CERT_AddCertToListHead(results.get(), dup) != SECSuccess) {
+        CERT_DestroyCertificate(dup);
+        return MapSECStatus(SECFailure);
+      }
+      // dup is now owned by results.
+    }
 
-    rv = subject.PrependNSSCertToList(certChain.get());
-    if (rv != Success) {
-      return rv;
-    }
-    BackCert* child = subject.childCert;
-    while (child) {
-      rv = child->PrependNSSCertToList(certChain.get());
-      if (rv != Success) {
-        return rv;
-      }
-      child = child->childCert;
-    }
-
-    SECStatus srv = trustDomain.IsChainValid(certChain.get());
+    // This must be done here, after the chain is built but before any
+    // revocation checks have been done.
+    SECStatus srv = trustDomain.IsChainValid(results.get());
     if (srv != SECSuccess) {
       return MapSECStatus(srv);
     }
 
-    // End of the recursion. Create the result list and add the trust anchor to
-    // it.
-    results = CERT_NewCertList();
-    if (!results) {
-      return FatalError;
-    }
-    rv = subject.PrependNSSCertToList(results.get());
-    return rv;
+    return Success;
   }
 
   if (endEntityOrCA == EndEntityOrCA::MustBeCA) {
     // Avoid stack overflows and poor performance by limiting cert chain
     // length.
     static const unsigned int MAX_SUBCA_COUNT = 6;
     if (subCACount >= MAX_SUBCA_COUNT) {
       return Fail(RecoverableError, SEC_ERROR_UNKNOWN_ISSUER);
@@ -306,17 +298,18 @@ BuildForward(TrustDomain& trustDomain,
                                                   subject.GetNSSCert(),
                                                   n->cert, time,
                                                   stapledOCSPResponse);
       if (srv != SECSuccess) {
         return MapSECStatus(SECFailure);
       }
 
       // We found a trusted issuer. At this point, we know the cert is valid
-      return subject.PrependNSSCertToList(results.get());
+      // and results contains the complete cert chain.
+      return Success;
     }
     if (rv != RecoverableError) {
       return rv;
     }
 
     PRErrorCode currentError = PR_GetError();
     switch (currentError) {
       case 0:
@@ -388,23 +381,9 @@ PLArenaPool*
 BackCert::GetArena()
 {
   if (!arena) {
     arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
   }
   return arena.get();
 }
 
-Result
-BackCert::PrependNSSCertToList(CERTCertList* results)
-{
-  PORT_Assert(results);
-
-  CERTCertificate* dup = CERT_DupCertificate(nssCert.get());
-  if (CERT_AddCertToListHead(results, dup) != SECSuccess) { // takes ownership
-    CERT_DestroyCertificate(dup);
-    return FatalError;
-  }
-
-  return Success;
-}
-
 } } // namespace mozilla::pkix
--- a/security/pkix/lib/pkixutil.h
+++ b/security/pkix/lib/pkixutil.h
@@ -138,20 +138,16 @@ public:
   /*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);
 
-  // This is the only place where we should be dealing with non-const
-  // CERTCertificates.
-  Result PrependNSSCertToList(CERTCertList* results);
-
   PLArenaPool* GetArena();
 
 private:
   ScopedCERTCertificate nssCert;
 
   ScopedPLArenaPool arena;
   CERTGeneralName* constrainedNames;
   IncludeCN includeCN;