Bug 1434936 - Rework ChainHasValidPins to use nsNSSCertList r=keeler r=fkiefer

This commit reworks PublicKeyPinningService::ChainHasValidPins and
PublicKeyPinningService::EvalChain to use nsNSSCertList directly.  It also
updates nsSiteSecurityService::ProcessPKPHeader. This will be made more
efficient in Bug 1406854, where the call to VerifySSLServerCert gets replaced
with one to GetSucceededCertChain. (Such a change is premeature now because
before Bug 731478 lands this would lead to a session resumption regression
causing pins to not be set properly, which is triggered repeatedly in the
xpcshell tests.)

MozReview-Commit-ID: 1l186n1lXLH

--HG--
extra : rebase_source : 88e40bbf41b324ece762abfa84a758380102e199
extra : histedit_source : addcddf253c2901a25b29f65046908f52df61345

security/certverifier/NSSCertDBTrustDomain.cpp

@@ -820,7 +820,7 @@ NSSCertDBTrustDomain::IsChainValid(const DERArray& certArray, Time time,
       (mPinningMode == CertVerifier::pinningEnforceTestMode);
     bool chainHasValidPins;
     nsrv = PublicKeyPinningService::ChainHasValidPins(
-      certList, mHostname, time, enforceTestMode, mOriginAttributes,
+      nssCertList, mHostname, time, enforceTestMode, mOriginAttributes,
       chainHasValidPins, mPinningTelemetryInfo);
     if (NS_FAILED(nsrv)) {
       return Result::FATAL_ERROR_LIBRARY_FAILURE;

security/manager/ssl/PublicKeyPinningService.cpp

@@ -100,37 +100,41 @@ EvalCert(const CERTCertificate* cert, const StaticFingerprints* fingerprints,
  * dynamicFingerprints array, or to false otherwise.
  */
 static nsresult
-EvalChain(const UniqueCERTCertList& certList,
+EvalChain(const RefPtr<nsNSSCertList>& certList,
           const StaticFingerprints* fingerprints,
           const nsTArray<nsCString>* dynamicFingerprints,
   /*out*/ bool& certListIntersectsPinset)
 {
   certListIntersectsPinset = false;
-  CERTCertificate* currentCert;
-
   if (!fingerprints && !dynamicFingerprints) {
     MOZ_ASSERT(false, "Must pass in at least one type of pinset");
     return NS_ERROR_FAILURE;
   }
 
-  CERTCertListNode* node;
+  certList->ForEachCertificateInChain(
-  for (node = CERT_LIST_HEAD(certList); !CERT_LIST_END(node, certList);
+    [&certListIntersectsPinset, &fingerprints, &dynamicFingerprints]
-       node = CERT_LIST_NEXT(node)) {
+      (nsCOMPtr<nsIX509Cert> aCert, bool aHasMore, /* out */ bool& aContinue) {
-    currentCert = node->cert;
+      // We need an owning handle when calling nsIX509Cert::GetCert().
-    MOZ_LOG(gPublicKeyPinningLog, LogLevel::Debug,
+      UniqueCERTCertificate nssCert(aCert->GetCert());
-           ("pkpin: certArray subject: '%s'\n", currentCert->subjectName));
+      MOZ_LOG(gPublicKeyPinningLog, LogLevel::Debug,
-    MOZ_LOG(gPublicKeyPinningLog, LogLevel::Debug,
+             ("pkpin: certArray subject: '%s'\n", nssCert->subjectName));
-           ("pkpin: certArray issuer: '%s'\n", currentCert->issuerName));
+      MOZ_LOG(gPublicKeyPinningLog, LogLevel::Debug,
-    nsresult rv = EvalCert(currentCert, fingerprints, dynamicFingerprints,
+             ("pkpin: certArray issuer: '%s'\n", nssCert->issuerName));
-                           certListIntersectsPinset);
+      nsresult rv = EvalCert(nssCert.get(), fingerprints, dynamicFingerprints,
-    if (NS_FAILED(rv)) {
+                             certListIntersectsPinset);
-      return rv;
+      if (NS_FAILED(rv)) {
-    }
+        return rv;
-    if (certListIntersectsPinset) {
+      }
+
+      if (certListIntersectsPinset) {
+        aContinue = false;
+      }
       return NS_OK;
-    }
+  });
+
+  if (!certListIntersectsPinset) {
+    MOZ_LOG(gPublicKeyPinningLog, LogLevel::Debug, ("pkpin: no matches found\n"));
   }
-  MOZ_LOG(gPublicKeyPinningLog, LogLevel::Debug, ("pkpin: no matches found\n"));
   return NS_OK;
 }
 
@@ -151,7 +155,7 @@ private:
 };
 
 nsresult
-PublicKeyPinningService::ChainMatchesPinset(const UniqueCERTCertList& certList,
+PublicKeyPinningService::ChainMatchesPinset(const RefPtr<nsNSSCertList>& certList,
                                             const nsTArray<nsCString>& aSHA256keys,
                                     /*out*/ bool& chainMatchesPinset)
 {
@@ -240,7 +244,7 @@ FindPinningInformation(const char* hostname, mozilla::pkix::Time time,
 // subject public key info data in the list and the most relevant non-expired
 // pinset for the host or there is no pinning information for the host.
 static nsresult
-CheckPinsForHostname(const UniqueCERTCertList& certList, const char* hostname,
+CheckPinsForHostname(const RefPtr<nsNSSCertList>& certList, const char* hostname,
                      bool enforceTestMode, mozilla::pkix::Time time,
                      const OriginAttributes& originAttributes,
              /*out*/ bool& chainHasValidPins,
@@ -305,11 +309,18 @@ CheckPinsForHostname(const UniqueCERTCertList& certList, const char* hostname,
     }
 
     // We only collect per-CA pinning statistics upon failures.
-    CERTCertListNode* rootNode = CERT_LIST_TAIL(certList);
+    nsCOMPtr<nsIX509Cert> rootCert;
+    rv = certList->GetRootCertificate(rootCert);
+    if (NS_FAILED(rv)) {
+      return rv;
+    }
+
     // Only log telemetry if the certificate list is non-empty.
-    if (!CERT_LIST_END(rootNode, certList)) {
+    if (rootCert && !enforceTestModeResult && pinningTelemetryInfo) {
-      if (!enforceTestModeResult && pinningTelemetryInfo) {
+      UniqueCERTCertificate rootCertObj =
-        int32_t binNumber = RootCABinNumber(&rootNode->cert->derCert);
+        UniqueCERTCertificate(rootCert.get()->GetCert());
+      if (rootCertObj) {
+        int32_t binNumber = RootCABinNumber(&rootCertObj->derCert);
         if (binNumber != ROOT_CERTIFICATE_UNKNOWN ) {
           pinningTelemetryInfo->accumulateForRoot = true;
           pinningTelemetryInfo->rootBucket = binNumber;
@@ -329,7 +340,7 @@ CheckPinsForHostname(const UniqueCERTCertList& certList, const char* hostname,
 
 nsresult
 PublicKeyPinningService::ChainHasValidPins(
-  const UniqueCERTCertList& certList,
+  const RefPtr<nsNSSCertList>& certList,
   const char* hostname,
   mozilla::pkix::Time time,
   bool enforceTestMode,

security/manager/ssl/PublicKeyPinningService.h

@@ -8,6 +8,7 @@
 #include "CertVerifier.h"
 #include "ScopedNSSTypes.h"
 #include "cert.h"
+#include "nsNSSCertificate.h"
 #include "nsString.h"
 #include "nsTArray.h"
 #include "pkix/Time.h"
@@ -33,7 +34,7 @@ public:
    * Note: if an alt name is a wildcard, it won't necessarily find a pinset
    * that would otherwise be valid for it
    */
-  static nsresult ChainHasValidPins(const UniqueCERTCertList& certList,
+  static nsresult ChainHasValidPins(const RefPtr<nsNSSCertList>& certList,
                                     const char* hostname,
                                     mozilla::pkix::Time time,
                                     bool enforceTestMode,
@@ -45,7 +46,7 @@ public:
    * certificate list and the pins specified in the aSHA256keys array.
    * Values passed in are assumed to be in base64 encoded form.
    */
-  static nsresult ChainMatchesPinset(const UniqueCERTCertList& certList,
+  static nsresult ChainMatchesPinset(const RefPtr<nsNSSCertList>& certList,
                                      const nsTArray<nsCString>& aSHA256keys,
                              /*out*/ bool& chainMatchesPinset);
 

security/manager/ssl/nsSiteSecurityService.cpp

@@ -1109,6 +1109,7 @@ nsSiteSecurityService::ProcessPKPHeader(
   UniqueCERTCertificate nssCert(cert->GetCert());
   NS_ENSURE_TRUE(nssCert, NS_ERROR_FAILURE);
 
+  // This use of VerifySSLServerCert should be able to be removed in Bug #1406854
   mozilla::pkix::Time now(mozilla::pkix::Now());
   UniqueCERTCertList certList;
   RefPtr<SharedCertVerifier> certVerifier(GetDefaultCertVerifier());
@@ -1134,13 +1135,22 @@ nsSiteSecurityService::ProcessPKPHeader(
     return NS_ERROR_FAILURE;
   }
 
-  CERTCertListNode* rootNode = CERT_LIST_TAIL(certList);
+  // This copy to produce an nsNSSCertList should also be removed in Bug #1406854
-  if (CERT_LIST_END(rootNode, certList)) {
+  nsCOMPtr<nsIX509CertList> x509CertList = new nsNSSCertList(Move(certList));
-    return NS_ERROR_FAILURE;
+  if (!x509CertList) {
+    return rv;
   }
+
+  RefPtr<nsNSSCertList> nssCertList = x509CertList->GetCertList();
+  nsCOMPtr<nsIX509Cert> rootCert;
+  rv = nssCertList->GetRootCertificate(rootCert);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+
   bool isBuiltIn = false;
-  mozilla::pkix::Result result = IsCertBuiltInRoot(rootNode->cert, isBuiltIn);
+  rv = rootCert->GetIsBuiltInRoot(&isBuiltIn);
-  if (result != mozilla::pkix::Success) {
+  if (NS_FAILED(rv)) {
     return NS_ERROR_FAILURE;
   }
 
@@ -1162,7 +1172,7 @@ nsSiteSecurityService::ProcessPKPHeader(
   }
 
   bool chainMatchesPinset;
-  rv = PublicKeyPinningService::ChainMatchesPinset(certList, sha256keys,
+  rv = PublicKeyPinningService::ChainMatchesPinset(nssCertList, sha256keys,
                                                    chainMatchesPinset);
   if (NS_FAILED(rv)) {
     return rv;
@@ -1183,7 +1193,7 @@ nsSiteSecurityService::ProcessPKPHeader(
   for (uint32_t i = 0; i < sha256keys.Length(); i++) {
     nsTArray<nsCString> singlePin;
     singlePin.AppendElement(sha256keys[i]);
-    rv = PublicKeyPinningService::ChainMatchesPinset(certList, singlePin,
+    rv = PublicKeyPinningService::ChainMatchesPinset(nssCertList, singlePin,
                                                      chainMatchesPinset);
     if (NS_FAILED(rv)) {
       return rv;