[tbb-commits] [tor-browser/tor-browser-45.2.0esr-6.5-1] Bug 1233328 - Part 1: Ignore SHA-1 pins in PublicKeyPinningService.cpp. r=keeler

gk at torproject.org gk at torproject.org
Fri Jun 3 20:52:32 UTC 2016


commit cc0fcf624033578259dab28ccaaa90bbd85d3a12
Author: Cykesiopka <cykesiopka.bmo at gmail.com>
Date:   Wed Jan 20 20:40:01 2016 -0800

    Bug 1233328 - Part 1: Ignore SHA-1 pins in PublicKeyPinningService.cpp. r=keeler
---
 security/manager/ssl/PublicKeyPinningService.cpp | 88 ++++++++----------------
 security/manager/ssl/PublicKeyPinningService.h   | 18 +++--
 2 files changed, 35 insertions(+), 71 deletions(-)

diff --git a/security/manager/ssl/PublicKeyPinningService.cpp b/security/manager/ssl/PublicKeyPinningService.cpp
index 073b3db..7fa7bf7 100644
--- a/security/manager/ssl/PublicKeyPinningService.cpp
+++ b/security/manager/ssl/PublicKeyPinningService.cpp
@@ -31,12 +31,11 @@ PRLogModuleInfo* gPublicKeyPinningLog =
  of the DER Encoded subject Public Key Info for the given cert
 */
 static nsresult
-GetBase64HashSPKI(const CERTCertificate* cert, SECOidTag hashType,
-                  nsACString& hashSPKIDigest)
+GetBase64HashSPKI(const CERTCertificate* cert, nsACString& hashSPKIDigest)
 {
   hashSPKIDigest.Truncate();
   Digest digest;
-  nsresult rv = digest.DigestBuf(hashType, cert->derPublicKey.data,
+  nsresult rv = digest.DigestBuf(SEC_OID_SHA256, cert->derPublicKey.data,
                                  cert->derPublicKey.len);
   if (NS_FAILED(rv)) {
     return rv;
@@ -48,24 +47,23 @@ GetBase64HashSPKI(const CERTCertificate* cert, SECOidTag hashType,
 }
 
 /*
- * Returns true if a given cert matches any hashType fingerprints from the
- * given pinset or the dynamicFingeprints array, false otherwise.
+ * Sets certMatchesPinset to true if a given cert matches any fingerprints from
+ * the given pinset or the dynamicFingerprints array, or to false otherwise.
  */
 static nsresult
-EvalCertWithHashType(const CERTCertificate* cert, SECOidTag hashType,
-                     const StaticFingerprints* fingerprints,
-                     const nsTArray<nsCString>* dynamicFingerprints,
-             /*out*/ bool& certMatchesPinset)
+EvalCert(const CERTCertificate* cert, const StaticFingerprints* fingerprints,
+         const nsTArray<nsCString>* dynamicFingerprints,
+ /*out*/ bool& certMatchesPinset)
 {
   certMatchesPinset = false;
   if (!fingerprints && !dynamicFingerprints) {
     MOZ_LOG(gPublicKeyPinningLog, LogLevel::Debug,
-           ("pkpin: No hashes found for hash type: %d\n", hashType));
+           ("pkpin: No hashes found\n"));
     return NS_ERROR_INVALID_ARG;
   }
 
   nsAutoCString base64Out;
-  nsresult rv = GetBase64HashSPKI(cert, hashType, base64Out);
+  nsresult rv = GetBase64HashSPKI(cert, base64Out);
   if (NS_FAILED(rv)) {
     MOZ_LOG(gPublicKeyPinningLog, LogLevel::Debug,
            ("pkpin: GetBase64HashSPKI failed!\n"));
@@ -96,30 +94,25 @@ EvalCertWithHashType(const CERTCertificate* cert, SECOidTag hashType,
 }
 
 /*
- * Returns true if a given chain matches any hashType fingerprints from the
- * given pinset or the dynamicFingerprints array, false otherwise.
+ * Sets certListIntersectsPinset to true if a given chain matches any
+ * fingerprints from the given pinset or the dynamicFingerprints array, or to
+ * false otherwise.
  */
 static nsresult
-EvalChainWithHashType(const CERTCertList* certList, SECOidTag hashType,
-                      const StaticPinset* pinset,
-                      const nsTArray<nsCString>* dynamicFingerprints,
-              /*out*/ bool& certListIntersectsPinset)
+EvalChain(const CERTCertList* certList, const StaticPinset* pinset,
+          const nsTArray<nsCString>* dynamicFingerprints,
+  /*out*/ bool& certListIntersectsPinset)
 {
   certListIntersectsPinset = false;
   CERTCertificate* currentCert;
 
   const StaticFingerprints* fingerprints = nullptr;
   if (pinset) {
-    if (hashType == SEC_OID_SHA256) {
-      fingerprints = pinset->sha256;
-    } else if (hashType == SEC_OID_SHA1) {
-      fingerprints = pinset->sha1;
-    }
+    fingerprints = pinset->sha256;
   }
-  // This can happen if dynamicFingerprints is null and the static pinset
-  // doesn't have any pins of this hash type.
   if (!fingerprints && !dynamicFingerprints) {
-    return NS_OK;
+    MOZ_ASSERT(false, "Must pass in at least one type of pinset");
+    return NS_ERROR_FAILURE;
   }
 
   CERTCertListNode* node;
@@ -130,9 +123,8 @@ EvalChainWithHashType(const CERTCertList* certList, SECOidTag hashType,
            ("pkpin: certArray subject: '%s'\n", currentCert->subjectName));
     MOZ_LOG(gPublicKeyPinningLog, LogLevel::Debug,
            ("pkpin: certArray issuer: '%s'\n", currentCert->issuerName));
-    nsresult rv = EvalCertWithHashType(currentCert, hashType, fingerprints,
-                                       dynamicFingerprints,
-                                       certListIntersectsPinset);
+    nsresult rv = EvalCert(currentCert, fingerprints, dynamicFingerprints,
+                           certListIntersectsPinset);
     if (NS_FAILED(rv)) {
       return rv;
     }
@@ -145,29 +137,6 @@ EvalChainWithHashType(const CERTCertList* certList, SECOidTag hashType,
 }
 
 /**
- * Given a pinset and certlist, return true if one of the certificates on
- * the list matches a fingerprint in the pinset, false otherwise.
- */
-static nsresult
-EvalChainWithPinset(const CERTCertList* certList,
-                    const StaticPinset* pinset,
-            /*out*/ bool& certListIntersectsPinset)
-{
-  certListIntersectsPinset = false;
-  // SHA256 is more trustworthy, try that first.
-  nsresult rv = EvalChainWithHashType(certList, SEC_OID_SHA256, pinset,
-                                      nullptr, certListIntersectsPinset);
-  if (NS_FAILED(rv)) {
-    return rv;
-  }
-  if (certListIntersectsPinset) {
-    return NS_OK;
-  }
-  return EvalChainWithHashType(certList, SEC_OID_SHA1, pinset, nullptr,
-                               certListIntersectsPinset);
-}
-
-/**
   Comparator for the is public key pinned host.
 */
 static int
@@ -184,8 +153,7 @@ PublicKeyPinningService::ChainMatchesPinset(const CERTCertList* certList,
                                             const nsTArray<nsCString>& aSHA256keys,
                                     /*out*/ bool& chainMatchesPinset)
 {
-  return EvalChainWithHashType(certList, SEC_OID_SHA256, nullptr, &aSHA256keys,
-                               chainMatchesPinset);
+  return EvalChain(certList, nullptr, &aSHA256keys, chainMatchesPinset);
 }
 
 // Returns via one of the output parameters the most relevant pinning
@@ -206,10 +174,9 @@ FindPinningInformation(const char* hostname, mozilla::pkix::Time time,
   if (!sssService) {
     return NS_ERROR_FAILURE;
   }
-  SiteHPKPState dynamicEntry;
-  TransportSecurityPreload *foundEntry = nullptr;
-  char *evalHost = const_cast<char*>(hostname);
-  char *evalPart;
+  TransportSecurityPreload* foundEntry = nullptr;
+  char* evalHost = const_cast<char*>(hostname);
+  char* evalPart;
   // Notice how the (xx = strchr) prevents pins for unqualified domain names.
   while (!foundEntry && (evalPart = strchr(evalHost, '.'))) {
     MOZ_LOG(gPublicKeyPinningLog, LogLevel::Debug,
@@ -293,13 +260,12 @@ CheckPinsForHostname(const CERTCertList* certList, const char* hostname,
     return NS_OK;
   }
   if (dynamicFingerprints.Length() > 0) {
-    return EvalChainWithHashType(certList, SEC_OID_SHA256, nullptr,
-                                 &dynamicFingerprints, chainHasValidPins);
+    return EvalChain(certList, nullptr, &dynamicFingerprints, chainHasValidPins);
   }
   if (staticFingerprints) {
     bool enforceTestModeResult;
-    rv = EvalChainWithPinset(certList, staticFingerprints->pinset,
-                             enforceTestModeResult);
+    rv = EvalChain(certList, staticFingerprints->pinset, nullptr,
+                   enforceTestModeResult);
     if (NS_FAILED(rv)) {
       return rv;
     }
diff --git a/security/manager/ssl/PublicKeyPinningService.h b/security/manager/ssl/PublicKeyPinningService.h
index 9a02ab9..601f624 100644
--- a/security/manager/ssl/PublicKeyPinningService.h
+++ b/security/manager/ssl/PublicKeyPinningService.h
@@ -18,13 +18,11 @@ class PublicKeyPinningService
 {
 public:
   /**
-   * Returns true if the given (host, certList) passes pinning checks,
-   * false otherwise. If the host is pinned, return true if one of the keys in
-   * the given certificate chain matches the pin set specified by the
-   * hostname. If the hostname is null or empty evaluate against all the
-   * possible names for the EE cert (Common Name (CN) plus all DNS Name:
-   * subject Alt Name entries). The certList's head is the EE cert and the
-   * tail is the trust anchor.
+   * 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
    */
@@ -35,9 +33,9 @@ public:
                             /*out*/ bool& chainHasValidPins,
                    /*optional out*/ PinningTelemetryInfo* pinningTelemetryInfo);
   /**
-   * Returns true if there is any intersection between the certificate list
-   * and the pins specified in the aSHA256key array. Values passed in are
-   * assumed to be in base64 encoded form
+   * 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 CERTCertList* certList,
                                      const nsTArray<nsCString>& aSHA256keys,





More information about the tbb-commits mailing list