Bug 1434936 - Rework ChainHasValidPins to use nsNSSCertList r?keeler r?fkiefer draft
authorJ.C. Jones <jjones@mozilla.com>
Wed, 31 Jan 2018 18:50:29 -0700
changeset 752575 be7b44ac23f250ed0ea73d83bf4e64fd30f7814b
parent 752574 d441476f7ef4e4e9850bd333b1c83dcfa6962537
push id98298
push userbmo:jjones@mozilla.com
push dateThu, 08 Feb 2018 15:01:36 +0000
reviewerskeeler, fkiefer
bugs1434936, 1406854, 731478
milestone60.0a1
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
security/certverifier/NSSCertDBTrustDomain.cpp
security/manager/ssl/PublicKeyPinningService.cpp
security/manager/ssl/PublicKeyPinningService.h
security/manager/ssl/nsSiteSecurityService.cpp
--- a/security/certverifier/NSSCertDBTrustDomain.cpp
+++ b/security/certverifier/NSSCertDBTrustDomain.cpp
@@ -815,17 +815,17 @@ NSSCertDBTrustDomain::IsChainValid(const
   // If mHostname isn't set, we're not verifying in the context of a TLS
   // handshake, so don't verify HPKP in those cases.
   if (mHostname && (mPinningMode != CertVerifier::pinningDisabled) &&
       !skipPinningChecksBecauseOfMITMMode) {
     bool enforceTestMode =
       (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;
     }
     if (!chainHasValidPins) {
       return Result::ERROR_KEY_PINNING_FAILURE;
     }
   }
--- a/security/manager/ssl/PublicKeyPinningService.cpp
+++ b/security/manager/ssl/PublicKeyPinningService.cpp
@@ -95,47 +95,51 @@ EvalCert(const CERTCertificate* cert, co
 }
 
 /*
  * Sets certListIntersectsPinset to true if a given chain matches any
  * fingerprints from the given static fingerprints or the
  * 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;
-  for (node = CERT_LIST_HEAD(certList); !CERT_LIST_END(node, certList);
-       node = CERT_LIST_NEXT(node)) {
-    currentCert = node->cert;
-    MOZ_LOG(gPublicKeyPinningLog, LogLevel::Debug,
-           ("pkpin: certArray subject: '%s'\n", currentCert->subjectName));
-    MOZ_LOG(gPublicKeyPinningLog, LogLevel::Debug,
-           ("pkpin: certArray issuer: '%s'\n", currentCert->issuerName));
-    nsresult rv = EvalCert(currentCert, fingerprints, dynamicFingerprints,
-                           certListIntersectsPinset);
-    if (NS_FAILED(rv)) {
-      return rv;
-    }
-    if (certListIntersectsPinset) {
+  certList->ForEachCertificateInChain(
+    [&certListIntersectsPinset, &fingerprints, &dynamicFingerprints]
+      (nsCOMPtr<nsIX509Cert> aCert, bool aHasMore, /* out */ bool& aContinue) {
+      // We need an owning handle when calling nsIX509Cert::GetCert().
+      UniqueCERTCertificate nssCert(aCert->GetCert());
+      MOZ_LOG(gPublicKeyPinningLog, LogLevel::Debug,
+             ("pkpin: certArray subject: '%s'\n", nssCert->subjectName));
+      MOZ_LOG(gPublicKeyPinningLog, LogLevel::Debug,
+             ("pkpin: certArray issuer: '%s'\n", nssCert->issuerName));
+      nsresult rv = EvalCert(nssCert.get(), fingerprints, dynamicFingerprints,
+                             certListIntersectsPinset);
+      if (NS_FAILED(rv)) {
+        return rv;
+      }
+
+      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;
 }
 
 class TransportSecurityPreloadBinarySearchComparator
 {
 public:
   explicit TransportSecurityPreloadBinarySearchComparator(
     const char* aTargetHost)
@@ -146,17 +150,17 @@ public:
     return strcmp(mTargetHost, val.mHost);
   }
 
 private:
   const char* mTargetHost; // non-owning
 };
 
 nsresult
-PublicKeyPinningService::ChainMatchesPinset(const UniqueCERTCertList& certList,
+PublicKeyPinningService::ChainMatchesPinset(const RefPtr<nsNSSCertList>& certList,
                                             const nsTArray<nsCString>& aSHA256keys,
                                     /*out*/ bool& chainMatchesPinset)
 {
   return EvalChain(certList, nullptr, &aSHA256keys, chainMatchesPinset);
 }
 
 // Returns via one of the output parameters the most relevant pinning
 // information that is valid for the given host at the given time.
@@ -235,17 +239,17 @@ FindPinningInformation(const char* hostn
 }
 
 // Returns true via the output parameter if the given certificate list meets
 // pinning requirements for the given host at the given time. It must be the
 // case that either there is an intersection between the set of hashes of
 // 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,
     /*optional out*/ PinningTelemetryInfo* pinningTelemetryInfo)
 {
   chainHasValidPins = false;
   if (!certList) {
     return NS_ERROR_INVALID_ARG;
@@ -300,21 +304,28 @@ CheckPinsForHostname(const UniqueCERTCer
         pinningTelemetryInfo->certPinningResultBucket =
             enforceTestModeResult ? 1 : 0;
       }
       pinningTelemetryInfo->accumulateResult = true;
       pinningTelemetryInfo->certPinningResultHistogram = histogram;
     }
 
     // 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 (!enforceTestModeResult && pinningTelemetryInfo) {
-        int32_t binNumber = RootCABinNumber(&rootNode->cert->derCert);
+    if (rootCert && !enforceTestModeResult && pinningTelemetryInfo) {
+      UniqueCERTCertificate rootCertObj =
+        UniqueCERTCertificate(rootCert.get()->GetCert());
+      if (rootCertObj) {
+        int32_t binNumber = RootCABinNumber(&rootCertObj->derCert);
         if (binNumber != ROOT_CERTIFICATE_UNKNOWN ) {
           pinningTelemetryInfo->accumulateForRoot = true;
           pinningTelemetryInfo->rootBucket = binNumber;
         }
       }
     }
 
     MOZ_LOG(gPublicKeyPinningLog, LogLevel::Debug,
@@ -324,17 +335,17 @@ CheckPinsForHostname(const UniqueCERTCer
             hostname, staticFingerprints->mTestMode ? "test" : "production"));
   }
 
   return NS_OK;
 }
 
 nsresult
 PublicKeyPinningService::ChainHasValidPins(
-  const UniqueCERTCertList& certList,
+  const RefPtr<nsNSSCertList>& certList,
   const char* hostname,
   mozilla::pkix::Time time,
   bool enforceTestMode,
   const OriginAttributes& originAttributes,
   /*out*/ bool& chainHasValidPins,
   /*optional out*/ PinningTelemetryInfo* pinningTelemetryInfo)
 {
   chainHasValidPins = false;
--- a/security/manager/ssl/PublicKeyPinningService.h
+++ b/security/manager/ssl/PublicKeyPinningService.h
@@ -3,16 +3,17 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef PublicKeyPinningService_h
 #define PublicKeyPinningService_h
 
 #include "CertVerifier.h"
 #include "ScopedNSSTypes.h"
 #include "cert.h"
+#include "nsNSSCertificate.h"
 #include "nsString.h"
 #include "nsTArray.h"
 #include "pkix/Time.h"
 
 namespace mozilla {
 class OriginAttributes;
 }
 
@@ -28,29 +29,29 @@ public:
    * Sets chainHasValidPins to true if the given (host, certList) passes pinning
    * checks, or to false otherwise. If the host is pinned, returns true via
    * chainHasValidPins if one of the keys in the given certificate chain matches
    * the pin set specified by the hostname. The certList's head is the EE cert
    * and the tail is the trust anchor.
    * 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,
                                     const OriginAttributes& originAttributes,
                             /*out*/ bool& chainHasValidPins,
                    /*optional out*/ PinningTelemetryInfo* pinningTelemetryInfo);
   /**
    * Sets chainMatchesPinset to true if there is any intersection between the
    * 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);
 
   /**
    * Returns true via the output parameter hostHasPins if there is pinning
    * information for the given host that is valid at the given time, and false
    * otherwise.
    */
--- a/security/manager/ssl/nsSiteSecurityService.cpp
+++ b/security/manager/ssl/nsSiteSecurityService.cpp
@@ -1104,16 +1104,17 @@ nsSiteSecurityService::ProcessPKPHeader(
   NS_ENSURE_SUCCESS(rv, rv);
   nsCOMPtr<nsIX509Cert> cert;
   rv = aSSLStatus->GetServerCert(getter_AddRefs(cert));
   NS_ENSURE_SUCCESS(rv, rv);
   NS_ENSURE_TRUE(cert, NS_ERROR_FAILURE);
   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());
   NS_ENSURE_TRUE(certVerifier, NS_ERROR_UNEXPECTED);
   // We don't want this verification to cause any network traffic that would
   // block execution. Also, since we don't have access to the original stapled
   // OCSP response, we can't enforce this aspect of the TLS Feature extension.
   // This is ok, because it will have been enforced when we originally connected
@@ -1129,23 +1130,32 @@ nsSiteSecurityService::ProcessPKPHeader(
                                         certList,
                                         false, // don't store intermediates
                                         flags,
                                         aOriginAttributes)
         != mozilla::pkix::Success) {
     return NS_ERROR_FAILURE;
   }
 
-  CERTCertListNode* rootNode = CERT_LIST_TAIL(certList);
-  if (CERT_LIST_END(rootNode, certList)) {
-    return NS_ERROR_FAILURE;
+  // This copy to produce an nsNSSCertList should also be removed in Bug #1406854
+  nsCOMPtr<nsIX509CertList> x509CertList = new nsNSSCertList(Move(certList));
+  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);
-  if (result != mozilla::pkix::Success) {
+  rv = rootCert->GetIsBuiltInRoot(&isBuiltIn);
+  if (NS_FAILED(rv)) {
     return NS_ERROR_FAILURE;
   }
 
   if (!isBuiltIn && !mProcessPKPHeadersFromNonBuiltInRoots) {
     if (aFailureResult) {
       *aFailureResult = nsISiteSecurityService::ERROR_ROOT_NOT_BUILT_IN;
     }
     return NS_ERROR_FAILURE;
@@ -1157,17 +1167,17 @@ nsSiteSecurityService::ProcessPKPHeader(
   }
 
   // clamp maxAge to the maximum set by pref
   if (maxAge > mMaxMaxAge) {
     maxAge = mMaxMaxAge;
   }
 
   bool chainMatchesPinset;
-  rv = PublicKeyPinningService::ChainMatchesPinset(certList, sha256keys,
+  rv = PublicKeyPinningService::ChainMatchesPinset(nssCertList, sha256keys,
                                                    chainMatchesPinset);
   if (NS_FAILED(rv)) {
     return rv;
   }
   if (!chainMatchesPinset) {
     // is invalid
     SSSLOG(("SSS: Pins provided by %s are invalid no match with certList\n", host.get()));
     if (aFailureResult) {
@@ -1178,17 +1188,17 @@ nsSiteSecurityService::ProcessPKPHeader(
 
   // finally we need to ensure that there is a "backup pin" ie. There must be
   // at least one fingerprint hash that does NOT validate against the verified
   // chain (Section 2.5 of the spec)
   bool hasBackupPin = false;
   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;
     }
     if (!chainMatchesPinset) {
       hasBackupPin = true;
     }
   }