Bug 1153444 - Fix up Key Pinning Telemetry (r=keeler)
authorMark Goodwin <mgoodwin@mozilla.com>
Fri, 21 Aug 2015 15:14:08 +0100
changeset 258838 fc86e9f2d6ea34b486058211fe468f4ada67f144
parent 258837 91fe6c306c8aa08cf24ed9fe9d507d836d9dc785
child 258839 0f06efef9c6d626de9d1071c038f89ef9a9088d6
push id14787
push userryanvm@gmail.com
push dateSun, 23 Aug 2015 21:21:14 +0000
treeherderfx-team@4ccdd06e51d7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskeeler
bugs1153444
milestone43.0a1
Bug 1153444 - Fix up Key Pinning Telemetry (r=keeler)
security/certverifier/CertVerifier.cpp
security/certverifier/CertVerifier.h
security/certverifier/NSSCertDBTrustDomain.cpp
security/certverifier/NSSCertDBTrustDomain.h
security/manager/ssl/PublicKeyPinningService.cpp
security/manager/ssl/PublicKeyPinningService.h
security/manager/ssl/RootCertificateTelemetryUtils.cpp
security/manager/ssl/RootCertificateTelemetryUtils.h
security/manager/ssl/SSLServerCertVerification.cpp
security/manager/ssl/tests/unit/test_pinning.js
--- a/security/certverifier/CertVerifier.cpp
+++ b/security/certverifier/CertVerifier.cpp
@@ -119,17 +119,18 @@ SECStatus
 CertVerifier::VerifyCert(CERTCertificate* cert, SECCertificateUsage usage,
                          Time time, void* pinArg, const char* hostname,
                          const Flags flags,
             /*optional*/ const SECItem* stapledOCSPResponseSECItem,
         /*optional out*/ ScopedCERTCertList* builtChain,
         /*optional out*/ SECOidTag* evOidPolicy,
         /*optional out*/ OCSPStaplingStatus* ocspStaplingStatus,
         /*optional out*/ KeySizeStatus* keySizeStatus,
-        /*optional out*/ SignatureDigestStatus* sigDigestStatus)
+        /*optional out*/ SignatureDigestStatus* sigDigestStatus,
+        /*optional out*/ PinningTelemetryInfo* pinningTelemetryInfo)
 {
   MOZ_LOG(gCertVerifierLog, LogLevel::Debug, ("Top of VerifyCert\n"));
 
   PR_ASSERT(cert);
   PR_ASSERT(usage == certificateUsageSSLServer || !(flags & FLAG_MUST_BE_EV));
   PR_ASSERT(usage == certificateUsageSSLServer || !keySizeStatus);
   PR_ASSERT(usage == certificateUsageSSLServer || !sigDigestStatus);
 
@@ -207,17 +208,17 @@ CertVerifier::VerifyCert(CERTCertificate
     case certificateUsageSSLClient: {
       // XXX: We don't really have a trust bit for SSL client authentication so
       // just use trustEmail as it is the closest alternative.
       NSSCertDBTrustDomain trustDomain(trustEmail, defaultOCSPFetching,
                                        mOCSPCache, pinArg, ocspGETConfig,
                                        mCertShortLifetimeInDays,
                                        pinningDisabled, MIN_RSA_BITS_WEAK,
                                        ValidityCheckingMode::CheckingOff,
-                                       AcceptAllAlgorithms, nullptr,
+                                       AcceptAllAlgorithms, nullptr, nullptr,
                                        builtChain);
       rv = BuildCertChain(trustDomain, certDER, time,
                           EndEntityOrCA::MustBeEndEntity,
                           KeyUsage::digitalSignature,
                           KeyPurposeId::id_kp_clientAuth,
                           CertPolicyId::anyPolicy, stapledOCSPResponse);
       break;
     }
@@ -257,22 +258,28 @@ CertVerifier::VerifyCert(CERTCertificate
                                     : NSSCertDBTrustDomain::FetchOCSPForEV;
 
       CertPolicyId evPolicy;
       SECOidTag evPolicyOidTag;
       SECStatus srv = GetFirstEVPolicy(cert, evPolicy, evPolicyOidTag);
       for (size_t i=0;
            i < digestAlgorithmOptionsCount && rv != Success && srv == SECSuccess;
            i++) {
+        // Because of the try-strict and fallback approach, we have to clear any
+        // previously noted telemetry information
+        if (pinningTelemetryInfo) {
+          pinningTelemetryInfo->Reset();
+        }
         NSSCertDBTrustDomain
           trustDomain(trustSSL, evOCSPFetching,
                       mOCSPCache, pinArg, ocspGETConfig,
                       mCertShortLifetimeInDays, mPinningMode, MIN_RSA_BITS,
                       ValidityCheckingMode::CheckForEV,
-                      digestAlgorithmOptions[i], hostname, builtChain);
+                      digestAlgorithmOptions[i], pinningTelemetryInfo, hostname,
+                      builtChain);
         rv = BuildCertChainForOneKeyUsage(trustDomain, certDER, time,
                                           KeyUsage::digitalSignature,// (EC)DHE
                                           KeyUsage::keyEncipherment, // RSA
                                           KeyUsage::keyAgreement,    // (EC)DH
                                           KeyPurposeId::id_kp_serverAuth,
                                           evPolicy, stapledOCSPResponse,
                                           ocspStaplingStatus);
         if (rv == Success) {
@@ -310,22 +317,29 @@ CertVerifier::VerifyCert(CERTCertificate
       static_assert(MOZ_ARRAY_LENGTH(keySizeOptions) ==
                     MOZ_ARRAY_LENGTH(keySizeStatuses),
                     "keySize array lengths differ");
 
       size_t keySizeOptionsCount = MOZ_ARRAY_LENGTH(keySizeStatuses);
 
       for (size_t i=0; i<keySizeOptionsCount && rv != Success; i++) {
         for (size_t j=0; j<digestAlgorithmOptionsCount && rv != Success; j++) {
+
+          // invalidate any telemetry info relating to failed chains
+          if (pinningTelemetryInfo) {
+            pinningTelemetryInfo->Reset();
+          }
+
           NSSCertDBTrustDomain trustDomain(trustSSL, defaultOCSPFetching,
                                            mOCSPCache, pinArg, ocspGETConfig,
                                            mCertShortLifetimeInDays,
                                            mPinningMode, keySizeOptions[i],
                                            ValidityCheckingMode::CheckingOff,
                                            digestAlgorithmOptions[j],
+                                           pinningTelemetryInfo,
                                            hostname, builtChain);
           rv = BuildCertChainForOneKeyUsage(trustDomain, certDER, time,
                                             KeyUsage::digitalSignature,//(EC)DHE
                                             KeyUsage::keyEncipherment,//RSA
                                             KeyUsage::keyAgreement,//(EC)DH
                                             KeyPurposeId::id_kp_serverAuth,
                                             CertPolicyId::anyPolicy,
                                             stapledOCSPResponse,
@@ -356,32 +370,32 @@ CertVerifier::VerifyCert(CERTCertificate
     }
 
     case certificateUsageSSLCA: {
       NSSCertDBTrustDomain trustDomain(trustSSL, defaultOCSPFetching,
                                        mOCSPCache, pinArg, ocspGETConfig,
                                        mCertShortLifetimeInDays,
                                        pinningDisabled, MIN_RSA_BITS_WEAK,
                                        ValidityCheckingMode::CheckingOff,
-                                       AcceptAllAlgorithms, nullptr,
+                                       AcceptAllAlgorithms, nullptr, nullptr,
                                        builtChain);
       rv = BuildCertChain(trustDomain, certDER, time,
                           EndEntityOrCA::MustBeCA, KeyUsage::keyCertSign,
                           KeyPurposeId::id_kp_serverAuth,
                           CertPolicyId::anyPolicy, stapledOCSPResponse);
       break;
     }
 
     case certificateUsageEmailSigner: {
       NSSCertDBTrustDomain trustDomain(trustEmail, defaultOCSPFetching,
                                        mOCSPCache, pinArg, ocspGETConfig,
                                        mCertShortLifetimeInDays,
                                        pinningDisabled, MIN_RSA_BITS_WEAK,
                                        ValidityCheckingMode::CheckingOff,
-                                       AcceptAllAlgorithms, nullptr,
+                                       AcceptAllAlgorithms, nullptr, nullptr,
                                        builtChain);
       rv = BuildCertChain(trustDomain, certDER, time,
                           EndEntityOrCA::MustBeEndEntity,
                           KeyUsage::digitalSignature,
                           KeyPurposeId::id_kp_emailProtection,
                           CertPolicyId::anyPolicy, stapledOCSPResponse);
       if (rv == Result::ERROR_INADEQUATE_KEY_USAGE) {
         rv = BuildCertChain(trustDomain, certDER, time,
@@ -397,17 +411,17 @@ CertVerifier::VerifyCert(CERTCertificate
       // TODO: The higher level S/MIME processing should pass in which key
       // usage it is trying to verify for, and base its algorithm choices
       // based on the result of the verification(s).
       NSSCertDBTrustDomain trustDomain(trustEmail, defaultOCSPFetching,
                                        mOCSPCache, pinArg, ocspGETConfig,
                                        mCertShortLifetimeInDays,
                                        pinningDisabled, MIN_RSA_BITS_WEAK,
                                        ValidityCheckingMode::CheckingOff,
-                                       AcceptAllAlgorithms, nullptr,
+                                       AcceptAllAlgorithms, nullptr, nullptr,
                                        builtChain);
       rv = BuildCertChain(trustDomain, certDER, time,
                           EndEntityOrCA::MustBeEndEntity,
                           KeyUsage::keyEncipherment, // RSA
                           KeyPurposeId::id_kp_emailProtection,
                           CertPolicyId::anyPolicy, stapledOCSPResponse);
       if (rv == Result::ERROR_INADEQUATE_KEY_USAGE) {
         rv = BuildCertChain(trustDomain, certDER, time,
@@ -420,17 +434,17 @@ CertVerifier::VerifyCert(CERTCertificate
     }
 
     case certificateUsageObjectSigner: {
       NSSCertDBTrustDomain trustDomain(trustObjectSigning, defaultOCSPFetching,
                                        mOCSPCache, pinArg, ocspGETConfig,
                                        mCertShortLifetimeInDays,
                                        pinningDisabled, MIN_RSA_BITS_WEAK,
                                        ValidityCheckingMode::CheckingOff,
-                                       AcceptAllAlgorithms, nullptr,
+                                       AcceptAllAlgorithms, nullptr, nullptr,
                                        builtChain);
       rv = BuildCertChain(trustDomain, certDER, time,
                           EndEntityOrCA::MustBeEndEntity,
                           KeyUsage::digitalSignature,
                           KeyPurposeId::id_kp_codeSigning,
                           CertPolicyId::anyPolicy, stapledOCSPResponse);
       break;
     }
@@ -452,41 +466,42 @@ CertVerifier::VerifyCert(CERTCertificate
         keyUsage = KeyUsage::digitalSignature;
         eku = KeyPurposeId::id_kp_OCSPSigning;
       }
 
       NSSCertDBTrustDomain sslTrust(trustSSL, defaultOCSPFetching, mOCSPCache,
                                     pinArg, ocspGETConfig, mCertShortLifetimeInDays,
                                     pinningDisabled, MIN_RSA_BITS_WEAK,
                                     ValidityCheckingMode::CheckingOff,
-                                    AcceptAllAlgorithms, nullptr, builtChain);
+                                    AcceptAllAlgorithms, nullptr, nullptr,
+                                    builtChain);
       rv = BuildCertChain(sslTrust, certDER, time, endEntityOrCA,
                           keyUsage, eku, CertPolicyId::anyPolicy,
                           stapledOCSPResponse);
       if (rv == Result::ERROR_UNKNOWN_ISSUER) {
         NSSCertDBTrustDomain emailTrust(trustEmail, defaultOCSPFetching,
                                         mOCSPCache, pinArg, ocspGETConfig,
                                         mCertShortLifetimeInDays,
                                         pinningDisabled, MIN_RSA_BITS_WEAK,
                                         ValidityCheckingMode::CheckingOff,
-                                        AcceptAllAlgorithms, nullptr,
+                                        AcceptAllAlgorithms, nullptr, nullptr,
                                         builtChain);
         rv = BuildCertChain(emailTrust, certDER, time, endEntityOrCA,
                             keyUsage, eku, CertPolicyId::anyPolicy,
                             stapledOCSPResponse);
         if (rv == Result::ERROR_UNKNOWN_ISSUER) {
           NSSCertDBTrustDomain objectSigningTrust(trustObjectSigning,
                                                   defaultOCSPFetching, mOCSPCache,
                                                   pinArg, ocspGETConfig,
                                                   mCertShortLifetimeInDays,
                                                   pinningDisabled,
                                                   MIN_RSA_BITS_WEAK,
                                                   ValidityCheckingMode::CheckingOff,
                                                   AcceptAllAlgorithms,
-                                                  nullptr, builtChain);
+                                                  nullptr, nullptr, builtChain);
           rv = BuildCertChain(objectSigningTrust, certDER, time,
                               endEntityOrCA, keyUsage, eku,
                               CertPolicyId::anyPolicy, stapledOCSPResponse);
         }
       }
 
       break;
     }
@@ -510,17 +525,18 @@ CertVerifier::VerifySSLServerCert(CERTCe
                      /*optional*/ void* pinarg,
                                   const char* hostname,
                                   bool saveIntermediatesInPermanentDatabase,
                                   Flags flags,
                  /*optional out*/ ScopedCERTCertList* builtChain,
                  /*optional out*/ SECOidTag* evOidPolicy,
                  /*optional out*/ OCSPStaplingStatus* ocspStaplingStatus,
                  /*optional out*/ KeySizeStatus* keySizeStatus,
-                 /*optional out*/ SignatureDigestStatus* sigDigestStatus)
+                 /*optional out*/ SignatureDigestStatus* sigDigestStatus,
+                 /*optional out*/ PinningTelemetryInfo* pinningTelemetryInfo)
 {
   PR_ASSERT(peerCert);
   // XXX: PR_ASSERT(pinarg)
   PR_ASSERT(hostname);
   PR_ASSERT(hostname[0]);
 
   if (builtChain) {
     *builtChain = nullptr;
@@ -535,17 +551,18 @@ CertVerifier::VerifySSLServerCert(CERTCe
   }
 
   ScopedCERTCertList builtChainTemp;
   // CreateCertErrorRunnable assumes that CheckCertHostname is only called
   // if VerifyCert succeeded.
   SECStatus rv = VerifyCert(peerCert, certificateUsageSSLServer, time, pinarg,
                             hostname, flags, stapledOCSPResponse,
                             &builtChainTemp, evOidPolicy, ocspStaplingStatus,
-                            keySizeStatus, sigDigestStatus);
+                            keySizeStatus, sigDigestStatus,
+                            pinningTelemetryInfo);
   if (rv != SECSuccess) {
     return rv;
   }
 
   Input peerCertInput;
   Result result = peerCertInput.Init(peerCert->derCert.data,
                                      peerCert->derCert.len);
   if (result != Success) {
--- a/security/certverifier/CertVerifier.h
+++ b/security/certverifier/CertVerifier.h
@@ -2,16 +2,17 @@
 /* vim: set ts=8 sts=2 et sw=2 tw=80: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef mozilla_psm__CertVerifier_h
 #define mozilla_psm__CertVerifier_h
 
+#include "mozilla/Telemetry.h"
 #include "pkix/pkixtypes.h"
 #include "OCSPCache.h"
 #include "ScopedNSSTypes.h"
 
 namespace mozilla { namespace psm {
 
 // These values correspond to the CERT_CHAIN_KEY_SIZE_STATUS telemetry.
 enum class KeySizeStatus {
@@ -26,16 +27,30 @@ enum class SignatureDigestStatus {
   NeverChecked = 0,
   GoodAlgorithmsOnly = 1,
   WeakEECert = 2,
   WeakCACert = 3,
   WeakCAAndEE = 4,
   AlreadyBad = 5,
 };
 
+class PinningTelemetryInfo
+{
+public:
+  // Should we accumulate pinning telemetry for the result?
+  bool accumulateResult;
+  Telemetry::ID certPinningResultHistogram;
+  int32_t certPinningResultBucket;
+  // Should we accumulate telemetry for the root?
+  bool accumulateForRoot;
+  int32_t rootBucket;
+
+  void Reset() { accumulateForRoot = false; accumulateResult = false; }
+};
+
 class CertVerifier
 {
 public:
   typedef unsigned int Flags;
   // XXX: FLAG_LOCAL_ONLY is ignored in the classic verification case
   static const Flags FLAG_LOCAL_ONLY;
   // Don't perform fallback DV validation on EV validation failure.
   static const Flags FLAG_MUST_BE_EV;
@@ -57,31 +72,33 @@ public:
                        void* pinArg,
                        const char* hostname,
                        Flags flags = 0,
        /*optional in*/ const SECItem* stapledOCSPResponse = nullptr,
       /*optional out*/ ScopedCERTCertList* builtChain = nullptr,
       /*optional out*/ SECOidTag* evOidPolicy = nullptr,
       /*optional out*/ OCSPStaplingStatus* ocspStaplingStatus = nullptr,
       /*optional out*/ KeySizeStatus* keySizeStatus = nullptr,
-      /*optional out*/ SignatureDigestStatus* sigDigestStatus = nullptr);
+      /*optional out*/ SignatureDigestStatus* sigDigestStatus = nullptr,
+      /*optional out*/ PinningTelemetryInfo* pinningTelemetryInfo = nullptr);
 
   SECStatus VerifySSLServerCert(
                     CERTCertificate* peerCert,
        /*optional*/ const SECItem* stapledOCSPResponse,
                     mozilla::pkix::Time time,
        /*optional*/ void* pinarg,
                     const char* hostname,
                     bool saveIntermediatesInPermanentDatabase = false,
                     Flags flags = 0,
    /*optional out*/ ScopedCERTCertList* builtChain = nullptr,
    /*optional out*/ SECOidTag* evOidPolicy = nullptr,
    /*optional out*/ OCSPStaplingStatus* ocspStaplingStatus = nullptr,
    /*optional out*/ KeySizeStatus* keySizeStatus = nullptr,
-   /*optional out*/ SignatureDigestStatus* sigDigestStatus = nullptr);
+   /*optional out*/ SignatureDigestStatus* sigDigestStatus = nullptr,
+   /*optional out*/ PinningTelemetryInfo* pinningTelemetryInfo = nullptr);
 
   enum PinningMode {
     pinningDisabled = 0,
     pinningAllowUserCAMITM = 1,
     pinningStrict = 2,
     pinningEnforceTestMode = 3
   };
 
--- a/security/certverifier/NSSCertDBTrustDomain.cpp
+++ b/security/certverifier/NSSCertDBTrustDomain.cpp
@@ -46,28 +46,30 @@ NSSCertDBTrustDomain::NSSCertDBTrustDoma
                                            OCSPCache& ocspCache,
              /*optional but shouldn't be*/ void* pinArg,
                                            CertVerifier::OcspGetConfig ocspGETConfig,
                                            uint32_t certShortLifetimeInDays,
                                            CertVerifier::PinningMode pinningMode,
                                            unsigned int minRSABits,
                                            ValidityCheckingMode validityCheckingMode,
                                            SignatureDigestOption signatureDigestOption,
+                              /*optional*/ PinningTelemetryInfo* pinningTelemetryInfo,
                               /*optional*/ const char* hostname,
                               /*optional*/ ScopedCERTCertList* builtChain)
   : mCertDBTrustType(certDBTrustType)
   , mOCSPFetching(ocspFetching)
   , mOCSPCache(ocspCache)
   , mPinArg(pinArg)
   , mOCSPGetConfig(ocspGETConfig)
   , mCertShortLifetimeInDays(certShortLifetimeInDays)
   , mPinningMode(pinningMode)
   , mMinRSABits(minRSABits)
   , mValidityCheckingMode(validityCheckingMode)
   , mSignatureDigestOption(signatureDigestOption)
+  , mPinningTelemetryInfo(pinningTelemetryInfo)
   , mHostname(hostname)
   , mBuiltChain(builtChain)
   , mCertBlocklist(do_GetService(NS_CERTBLOCKLIST_CONTRACTID))
   , mOCSPStaplingStatus(CertVerifier::OCSP_STAPLING_NEVER_CHECKED)
 {
 }
 
 // If useRoots is true, we only use root certificates in the candidate list.
@@ -787,17 +789,18 @@ 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;
     nsresult nsrv = PublicKeyPinningService::ChainHasValidPins(
-      certList, mHostname, time, enforceTestMode, chainHasValidPins);
+      certList, mHostname, time, enforceTestMode, chainHasValidPins,
+      mPinningTelemetryInfo);
     if (NS_FAILED(nsrv)) {
       return Result::FATAL_ERROR_LIBRARY_FAILURE;
     }
     if (!chainHasValidPins) {
       return Result::ERROR_KEY_PINNING_FAILURE;
     }
   }
 
--- a/security/certverifier/NSSCertDBTrustDomain.h
+++ b/security/certverifier/NSSCertDBTrustDomain.h
@@ -65,16 +65,17 @@ public:
   NSSCertDBTrustDomain(SECTrustType certDBTrustType, OCSPFetching ocspFetching,
                        OCSPCache& ocspCache, void* pinArg,
                        CertVerifier::OcspGetConfig ocspGETConfig,
                        uint32_t certShortLifetimeInDays,
                        CertVerifier::PinningMode pinningMode,
                        unsigned int minRSABits,
                        ValidityCheckingMode validityCheckingMode,
                        SignatureDigestOption,
+          /*optional*/ PinningTelemetryInfo* pinningTelemetryInfo = nullptr,
           /*optional*/ const char* hostname = nullptr,
       /*optional out*/ ScopedCERTCertList* builtChain = nullptr);
 
   virtual Result FindIssuer(mozilla::pkix::Input encodedIssuerName,
                             IssuerChecker& checker,
                             mozilla::pkix::Time time) override;
 
   virtual Result GetCertTrust(mozilla::pkix::EndEntityOrCA endEntityOrCA,
@@ -149,16 +150,17 @@ private:
   OCSPCache& mOCSPCache; // non-owning!
   void* mPinArg; // non-owning!
   const CertVerifier::OcspGetConfig mOCSPGetConfig;
   const uint32_t mCertShortLifetimeInDays;
   CertVerifier::PinningMode mPinningMode;
   const unsigned int mMinRSABits;
   ValidityCheckingMode mValidityCheckingMode;
   SignatureDigestOption mSignatureDigestOption;
+  PinningTelemetryInfo* mPinningTelemetryInfo;
   const char* mHostname; // non-owning - only used for pinning checks
   ScopedCERTCertList* mBuiltChain; // non-owning
   nsCOMPtr<nsICertBlocklist> mCertBlocklist;
   CertVerifier::OCSPStaplingStatus mOCSPStaplingStatus;
 };
 
 } } // namespace mozilla::psm
 
--- a/security/manager/ssl/PublicKeyPinningService.cpp
+++ b/security/manager/ssl/PublicKeyPinningService.cpp
@@ -266,17 +266,18 @@ 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 CERTCertList* certList, const char* hostname,
                      bool enforceTestMode, mozilla::pkix::Time time,
-             /*out*/ bool& chainHasValidPins)
+             /*out*/ bool& chainHasValidPins,
+    /*optional out*/ PinningTelemetryInfo* pinningTelemetryInfo)
 {
   chainHasValidPins = false;
   if (!certList) {
     return NS_ERROR_INVALID_ARG;
   }
   if (!hostname || hostname[0] == 0) {
     return NS_ERROR_INVALID_ARG;
   }
@@ -311,32 +312,42 @@ CheckPinsForHostname(const CERTCertList*
         ? Telemetry::CERT_PINNING_MOZ_TEST_RESULTS
         : Telemetry::CERT_PINNING_TEST_RESULTS;
       if (!enforceTestMode) {
         chainHasValidPins = true;
       }
     }
     // We can collect per-host pinning violations for this host because it is
     // operationally critical to Firefox.
-    if (staticFingerprints->mId != kUnknownId) {
-      int32_t bucket = staticFingerprints->mId * 2 + (enforceTestModeResult ? 1 : 0);
-      histogram = staticFingerprints->mTestMode
-        ? Telemetry::CERT_PINNING_MOZ_TEST_RESULTS_BY_HOST
-        : Telemetry::CERT_PINNING_MOZ_RESULTS_BY_HOST;
-      Telemetry::Accumulate(histogram, bucket);
-    } else {
-      Telemetry::Accumulate(histogram, enforceTestModeResult ? 1 : 0);
+    if (pinningTelemetryInfo) {
+      if (staticFingerprints->mId != kUnknownId) {
+        int32_t bucket = staticFingerprints->mId * 2
+                         + (enforceTestModeResult ? 1 : 0);
+        histogram = staticFingerprints->mTestMode
+          ? Telemetry::CERT_PINNING_MOZ_TEST_RESULTS_BY_HOST
+          : Telemetry::CERT_PINNING_MOZ_RESULTS_BY_HOST;
+        pinningTelemetryInfo->certPinningResultBucket = bucket;
+      } else {
+        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);
     // Only log telemetry if the certificate list is non-empty.
     if (!CERT_LIST_END(rootNode, certList)) {
-      if (!enforceTestModeResult) {
-        AccumulateTelemetryForRootCA(Telemetry::CERT_PINNING_FAILURES_BY_CA, rootNode->cert);
+      if (!enforceTestModeResult && pinningTelemetryInfo) {
+        int32_t binNumber = RootCABinNumber(&rootNode->cert->derCert);
+        if (binNumber != ROOT_CERTIFICATE_UNKNOWN ) {
+          pinningTelemetryInfo->accumulateForRoot = true;
+          pinningTelemetryInfo->rootBucket = binNumber;
+        }
       }
     }
 
     MOZ_LOG(gPublicKeyPinningLog, LogLevel::Debug,
            ("pkpin: Pin check %s for %s host '%s' (mode=%s)\n",
             enforceTestModeResult ? "passed" : "failed",
             staticFingerprints->mIsMoz ? "mozilla" : "non-mozilla",
             hostname, staticFingerprints->mTestMode ? "test" : "production"));
@@ -345,28 +356,30 @@ CheckPinsForHostname(const CERTCertList*
   return NS_OK;
 }
 
 nsresult
 PublicKeyPinningService::ChainHasValidPins(const CERTCertList* certList,
                                            const char* hostname,
                                            mozilla::pkix::Time time,
                                            bool enforceTestMode,
-                                   /*out*/ bool& chainHasValidPins)
+                                   /*out*/ bool& chainHasValidPins,
+                          /*optional out*/ PinningTelemetryInfo* pinningTelemetryInfo)
 {
   chainHasValidPins = false;
   if (!certList) {
     return NS_ERROR_INVALID_ARG;
   }
   if (!hostname || hostname[0] == 0) {
     return NS_ERROR_INVALID_ARG;
   }
   nsAutoCString canonicalizedHostname(CanonicalizeHostname(hostname));
   return CheckPinsForHostname(certList, canonicalizedHostname.get(),
-                              enforceTestMode, time, chainHasValidPins);
+                              enforceTestMode, time, chainHasValidPins,
+                              pinningTelemetryInfo);
 }
 
 nsresult
 PublicKeyPinningService::HostHasPins(const char* hostname,
                                      mozilla::pkix::Time time,
                                      bool enforceTestMode,
                                      /*out*/ bool& hostHasPins)
 {
--- a/security/manager/ssl/PublicKeyPinningService.h
+++ b/security/manager/ssl/PublicKeyPinningService.h
@@ -1,16 +1,17 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef PublicKeyPinningService_h
 #define PublicKeyPinningService_h
 
 #include "cert.h"
+#include "CertVerifier.h"
 #include "nsString.h"
 #include "nsTArray.h"
 #include "pkix/Time.h"
 
 namespace mozilla {
 namespace psm {
 
 class PublicKeyPinningService
@@ -26,17 +27,18 @@ public:
    * 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 CERTCertList* certList,
                                     const char* hostname,
                                     mozilla::pkix::Time time,
                                     bool enforceTestMode,
-                            /*out*/ bool& chainHasValidPins);
+                            /*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
    */
   static nsresult ChainMatchesPinset(const CERTCertList* certList,
                                      const nsTArray<nsCString>& aSHA256keys,
                              /*out*/ bool& chainMatchesPinset);
--- a/security/manager/ssl/RootCertificateTelemetryUtils.cpp
+++ b/security/manager/ssl/RootCertificateTelemetryUtils.cpp
@@ -6,23 +6,17 @@
 
 #include "RootCertificateTelemetryUtils.h"
 
 #include "mozilla/Logging.h"
 #include "RootHashes.inc" // Note: Generated by genRootCAHashes.js
 #include "ScopedNSSTypes.h"
 #include "mozilla/ArrayUtils.h"
 
-// Note: New CAs will show up as UNKNOWN_ROOT until
-// RootHashes.inc is updated to include them. 0 is reserved by
-// genRootCAHashes.js for the unknowns.
-#define UNKNOWN_ROOT  0
-#define HASH_FAILURE -1
-
-namespace mozilla { namespace psm { 
+namespace mozilla { namespace psm {
 
 PRLogModuleInfo* gPublicKeyPinningTelemetryLog =
   PR_NewLogModule("PublicKeyPinningTelemetryService");
 
 // Used in the BinarySearch method, this does a memcmp between the pointer
 // provided to its construtor and whatever the binary search is looking for.
 //
 // This implementation assumes everything to be of HASH_LEN, so it should not
@@ -49,17 +43,17 @@ private:
 int32_t
 RootCABinNumber(const SECItem* cert)
 {
   Digest digest;
 
   // Compute SHA256 hash of the certificate
   nsresult rv = digest.DigestBuf(SEC_OID_SHA256, cert->data, cert->len);
   if (NS_WARN_IF(NS_FAILED(rv))) {
-    return HASH_FAILURE;
+    return ROOT_CERTIFICATE_HASH_FAILURE;
   }
 
   // Compare against list of stored hashes
   size_t idx;
 
   MOZ_LOG(gPublicKeyPinningTelemetryLog, LogLevel::Debug,
            ("pkpinTelem: First bytes %02hx %02hx %02hx %02hx\n",
             digest.get().data[0], digest.get().data[1], digest.get().data[2], digest.get().data[3]));
@@ -71,27 +65,27 @@ RootCABinNumber(const SECItem* cert)
 
     MOZ_LOG(gPublicKeyPinningTelemetryLog, LogLevel::Debug,
           ("pkpinTelem: Telemetry index was %lu, bin is %d\n",
            idx, ROOT_TABLE[idx].binNumber));
     return (int32_t) ROOT_TABLE[idx].binNumber;
   }
 
   // Didn't match.
-  return UNKNOWN_ROOT;
+  return ROOT_CERTIFICATE_UNKNOWN;
 }
 
 
 // Attempt to increment the appropriate bin in the provided Telemetry probe ID. If
 // there was a hash failure, we do nothing.
 void
 AccumulateTelemetryForRootCA(mozilla::Telemetry::ID probe, 
   const CERTCertificate* cert)
 {
   int32_t binId = RootCABinNumber(&cert->derCert);
 
-  if (binId != HASH_FAILURE) {
+  if (binId != ROOT_CERTIFICATE_HASH_FAILURE) {
     Accumulate(probe, binId);
   }
 }
 
 } // namespace psm
 } // namespace mozilla
--- a/security/manager/ssl/RootCertificateTelemetryUtils.h
+++ b/security/manager/ssl/RootCertificateTelemetryUtils.h
@@ -7,15 +7,24 @@
 #ifndef RootCertificateTelemetryUtils_h
 #define RootCertificateTelemetryUtils_h
 
 #include "mozilla/Telemetry.h"
 #include "certt.h"
 
 namespace mozilla { namespace psm {
 
+// Note: New CAs will show up as UNKNOWN_ROOT until
+// RootHashes.inc is updated to include them. 0 is reserved by
+// genRootCAHashes.js for the unknowns.
+#define ROOT_CERTIFICATE_UNKNOWN  0
+#define ROOT_CERTIFICATE_HASH_FAILURE -1
+
+int32_t
+RootCABinNumber(const SECItem* cert);
+
 void
 AccumulateTelemetryForRootCA(mozilla::Telemetry::ID probe, const CERTCertificate* cert);
 
 } // namespace psm
 } // namespace mozilla
 
 #endif // RootCertificateTelemetryUtils_h
--- a/security/manager/ssl/SSLServerCertVerification.cpp
+++ b/security/manager/ssl/SSLServerCertVerification.cpp
@@ -1173,23 +1173,25 @@ AuthCertificate(CertVerifier& certVerifi
     !(providerFlags & nsISocketProvider::NO_PERMANENT_STORAGE);
 
   SECOidTag evOidPolicy;
   ScopedCERTCertList certList;
   CertVerifier::OCSPStaplingStatus ocspStaplingStatus =
     CertVerifier::OCSP_STAPLING_NEVER_CHECKED;
   KeySizeStatus keySizeStatus = KeySizeStatus::NeverChecked;
   SignatureDigestStatus sigDigestStatus = SignatureDigestStatus::NeverChecked;
+  PinningTelemetryInfo pinningTelemetryInfo;
 
   rv = certVerifier.VerifySSLServerCert(cert, stapledOCSPResponse,
                                         time, infoObject,
                                         infoObject->GetHostNameRaw(),
                                         saveIntermediates, 0, &certList,
                                         &evOidPolicy, &ocspStaplingStatus,
-                                        &keySizeStatus, &sigDigestStatus);
+                                        &keySizeStatus, &sigDigestStatus,
+                                        &pinningTelemetryInfo);
   PRErrorCode savedErrorCode;
   if (rv != SECSuccess) {
     savedErrorCode = PR_GetError();
   }
 
   if (ocspStaplingStatus != CertVerifier::OCSP_STAPLING_NEVER_CHECKED) {
     Telemetry::Accumulate(Telemetry::SSL_OCSP_STAPLING, ocspStaplingStatus);
   }
@@ -1197,16 +1199,26 @@ AuthCertificate(CertVerifier& certVerifi
     Telemetry::Accumulate(Telemetry::CERT_CHAIN_KEY_SIZE_STATUS,
                           static_cast<uint32_t>(keySizeStatus));
   }
   if (sigDigestStatus != SignatureDigestStatus::NeverChecked) {
     Telemetry::Accumulate(Telemetry::CERT_CHAIN_SIGNATURE_DIGEST_STATUS,
                           static_cast<uint32_t>(sigDigestStatus));
   }
 
+  if (pinningTelemetryInfo.accumulateForRoot) {
+    Telemetry::Accumulate(Telemetry::CERT_PINNING_FAILURES_BY_CA,
+                          pinningTelemetryInfo.rootBucket);
+  }
+
+  if (pinningTelemetryInfo.accumulateResult) {
+    Telemetry::Accumulate(pinningTelemetryInfo.certPinningResultHistogram,
+                          pinningTelemetryInfo.certPinningResultBucket);
+  }
+
   // We want to remember the CA certs in the temp db, so that the application can find the
   // complete chain at any time it might need it.
   // But we keep only those CA certs in the temp db, that we didn't already know.
 
   RefPtr<nsSSLStatus> status(infoObject->SSLStatus());
   RefPtr<nsNSSCertificate> nsc;
 
   if (!status || !status->HasServerCert()) {
--- a/security/manager/ssl/tests/unit/test_pinning.js
+++ b/security/manager/ssl/tests/unit/test_pinning.js
@@ -209,21 +209,21 @@ function test_enforce_test_mode() {
 function check_pinning_telemetry() {
   let service = Cc["@mozilla.org/base/telemetry;1"].getService(Ci.nsITelemetry);
   let prod_histogram = service.getHistogramById("CERT_PINNING_RESULTS")
                          .snapshot();
   let test_histogram = service.getHistogramById("CERT_PINNING_TEST_RESULTS")
                          .snapshot();
   // Because all of our test domains are pinned to user-specified trust
   // anchors, effectively only strict mode and enforce test-mode get evaluated
-  equal(prod_histogram.counts[0], 16,
+  equal(prod_histogram.counts[0], 4,
         "Actual and expected prod (non-Mozilla) failure count should match");
   equal(prod_histogram.counts[1], 4,
         "Actual and expected prod (non-Mozilla) success count should match");
-  equal(test_histogram.counts[0], 5,
+  equal(test_histogram.counts[0], 2,
         "Actual and expected test (non-Mozilla) failure count should match");
   equal(test_histogram.counts[1], 0,
         "Actual and expected test (non-Mozilla) success count should match");
 
   let moz_prod_histogram = service.getHistogramById("CERT_PINNING_MOZ_RESULTS")
                              .snapshot();
   let moz_test_histogram =
     service.getHistogramById("CERT_PINNING_MOZ_TEST_RESULTS").snapshot();