Bug 1715142 - introduce nsIPublicKeyPinningService and remove 'type' …

…parameter from nsISiteSecurityService r=rmf,necko-reviewers The public key pinning implementation is much less complex than the HSTS implementation, and only needs a small subset of the parameters of the latter. Furthermore, the information it relies on is static, and so is safe to access from content processes. This patch separates the two implementations, thus simplifying both of them and avoiding some unnecessary IPC calls in the process. Differential Revision: https://phabricator.services.mozilla.com/D117096
dothq · Jun 10, 2021 · f3c620e · f3c620e
1 parent 5052690
commit f3c620e
Show file tree

Hide file tree

Showing 46 changed files with 276 additions and 592 deletions.
diff --git a/browser/base/content/test/general/browser_star_hsts.js b/browser/base/content/test/general/browser_star_hsts.js
@@ -15,7 +15,6 @@ add_task(async function test_star_redirect() {
       Ci.nsISiteSecurityService
     );
     sss.resetState(
-      Ci.nsISiteSecurityService.HEADER_HSTS,
       NetUtil.newURI("http://example.com/"),
       0,
       Services.prefs.getBoolPref("privacy.partition.network_state")

diff --git a/devtools/shared/webconsole/network-helper.js b/devtools/shared/webconsole/network-helper.js
@@ -685,6 +685,9 @@ var NetworkHelper = {
         const sss = Cc["@mozilla.org/ssservice;1"].getService(
           Ci.nsISiteSecurityService
         );
+        const pkps = Cc[
+          "@mozilla.org/security/publickeypinningservice;1"
+        ].getService(Ci.nsIPublicKeyPinningService);
 
         // SiteSecurityService uses different storage if the channel is
         // private. Thus we must give isSecureURI correct flags or we
@@ -699,8 +702,8 @@ var NetworkHelper = {
           uri = Services.io.newURI("https://" + host);
         }
 
-        info.hsts = sss.isSecureURI(sss.HEADER_HSTS, uri, flags);
-        info.hpkp = sss.isSecureURI(sss.STATIC_PINNING, uri, flags);
+        info.hsts = sss.isSecureURI(uri, flags);
+        info.hpkp = pkps.hostHasPins(uri);
       } else {
         DevToolsUtils.reportException(
           "NetworkHelper.parseSecurityInfo",

diff --git a/devtools/shared/webconsole/test/chrome/test_network_security-hsts.html b/devtools/shared/webconsole/test/chrome/test_network_security-hsts.html
@@ -47,7 +47,7 @@
   // Reset HSTS state.
   const gSSService = Cc["@mozilla.org/ssservice;1"].getService(Ci.nsISiteSecurityService);
   const uri = Services.io.newURI(TEST_CASES[0].url);
-  gSSService.resetState(Ci.nsISiteSecurityService.HEADER_HSTS, uri, 0);
+  gSSService.resetState(uri, 0);
 
   SimpleTest.finish();
 }

diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
@@ -139,6 +139,7 @@
 #include "nsIPrompt.h"
 #include "nsIPromptCollection.h"
 #include "nsIPromptFactory.h"
+#include "nsIPublicKeyPinningService.h"
 #include "nsIReflowObserver.h"
 #include "nsIScriptChannel.h"
 #include "nsIScriptObjectPrincipal.h"
@@ -3714,21 +3715,18 @@ nsDocShell::DisplayLoadError(nsresult aError, nsIURI* aURI,
         nsCOMPtr<nsISiteSecurityService> sss =
             do_GetService(NS_SSSERVICE_CONTRACTID, &rv);
         NS_ENSURE_SUCCESS(rv, rv);
-        rv = sss->IsSecureURI(nsISiteSecurityService::HEADER_HSTS, aURI, flags,
-                              attrsForHSTS, nullptr, nullptr, &isStsHost);
-        NS_ENSURE_SUCCESS(rv, rv);
-        rv = sss->IsSecureURI(nsISiteSecurityService::STATIC_PINNING, aURI,
-                              flags, GetOriginAttributes(), nullptr, nullptr,
-                              &isPinnedHost);
+        rv = sss->IsSecureURI(aURI, flags, attrsForHSTS, nullptr, nullptr,
+                              &isStsHost);
         NS_ENSURE_SUCCESS(rv, rv);
       } else {
         mozilla::dom::ContentChild* cc =
             mozilla::dom::ContentChild::GetSingleton();
-        cc->SendIsSecureURI(nsISiteSecurityService::HEADER_HSTS, aURI, flags,
-                            attrsForHSTS, &isStsHost);
-        cc->SendIsSecureURI(nsISiteSecurityService::STATIC_PINNING, aURI, flags,
-                            GetOriginAttributes(), &isPinnedHost);
+        cc->SendIsSecureURI(aURI, flags, attrsForHSTS, &isStsHost);
       }
+      nsCOMPtr<nsIPublicKeyPinningService> pkps =
+          do_GetService(NS_PKPSERVICE_CONTRACTID, &rv);
+      NS_ENSURE_SUCCESS(rv, rv);
+      rv = pkps->HostHasPins(aURI, &isPinnedHost);
 
       if (Preferences::GetBool("browser.xul.error_pages.expert_bad_cert",
                                false)) {

diff --git a/dom/base/Document.cpp b/dom/base/Document.cpp
@@ -341,6 +341,7 @@
 #include "nsIPermission.h"
 #include "nsIPrompt.h"
 #include "nsIPropertyBag2.h"
+#include "nsIPublicKeyPinningService.h"
 #include "nsIReferrerInfo.h"
 #include "nsIRefreshURI.h"
 #include "nsIRequest.h"
@@ -1890,23 +1891,22 @@ void Document::GetFailedCertSecurityInfo(FailedCertSecurityInfo& aInfo,
   if (XRE_IsContentProcess()) {
     ContentChild* cc = ContentChild::GetSingleton();
     MOZ_ASSERT(cc);
-    cc->SendIsSecureURI(nsISiteSecurityService::HEADER_HSTS, aURI, flags, attrs,
-                        &aInfo.mHasHSTS);
-    cc->SendIsSecureURI(nsISiteSecurityService::STATIC_PINNING, aURI, flags,
-                        attrs, &aInfo.mHasHPKP);
+    cc->SendIsSecureURI(aURI, flags, attrs, &aInfo.mHasHSTS);
   } else {
     nsCOMPtr<nsISiteSecurityService> sss =
         do_GetService(NS_SSSERVICE_CONTRACTID);
     if (NS_WARN_IF(!sss)) {
       return;
     }
-    Unused << NS_WARN_IF(NS_FAILED(
-        sss->IsSecureURI(nsISiteSecurityService::HEADER_HSTS, aURI, flags,
-                         attrs, nullptr, nullptr, &aInfo.mHasHSTS)));
-    Unused << NS_WARN_IF(NS_FAILED(
-        sss->IsSecureURI(nsISiteSecurityService::STATIC_PINNING, aURI, flags,
-                         attrs, nullptr, nullptr, &aInfo.mHasHPKP)));
+    Unused << NS_WARN_IF(NS_FAILED(sss->IsSecureURI(aURI, flags, attrs, nullptr,
+                                                    nullptr, &aInfo.mHasHSTS)));
   }
+  nsCOMPtr<nsIPublicKeyPinningService> pkps =
+      do_GetService(NS_PKPSERVICE_CONTRACTID);
+  if (NS_WARN_IF(!pkps)) {
+    return;
+  }
+  Unused << NS_WARN_IF(NS_FAILED(pkps->HostHasPins(aURI, &aInfo.mHasHPKP)));
 }
 
 bool Document::AllowDeprecatedTls() {

diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp
@@ -4399,7 +4399,7 @@ mozilla::ipc::IPCResult ContentParent::RecvSetURITitle(nsIURI* uri,
 }
 
 mozilla::ipc::IPCResult ContentParent::RecvIsSecureURI(
-    const uint32_t& aType, nsIURI* aURI, const uint32_t& aFlags,
+    nsIURI* aURI, const uint32_t& aFlags,
     const OriginAttributes& aOriginAttributes, bool* aIsSecureURI) {
   nsCOMPtr<nsISiteSecurityService> sss(do_GetService(NS_SSSERVICE_CONTRACTID));
   if (!sss) {
@@ -4408,8 +4408,8 @@ mozilla::ipc::IPCResult ContentParent::RecvIsSecureURI(
   if (!aURI) {
     return IPC_FAIL_NO_REASON(this);
   }
-  nsresult rv = sss->IsSecureURI(aType, aURI, aFlags, aOriginAttributes,
-                                 nullptr, nullptr, aIsSecureURI);
+  nsresult rv = sss->IsSecureURI(aURI, aFlags, aOriginAttributes, nullptr,
+                                 nullptr, aIsSecureURI);
   if (NS_FAILED(rv)) {
     return IPC_FAIL_NO_REASON(this);
   }

diff --git a/dom/ipc/ContentParent.h b/dom/ipc/ContentParent.h
@@ -941,7 +941,7 @@ class ContentParent final
       const uint32_t& chromeFlags);
 
   mozilla::ipc::IPCResult RecvIsSecureURI(
-      const uint32_t& aType, nsIURI* aURI, const uint32_t& aFlags,
+      nsIURI* aURI, const uint32_t& aFlags,
       const OriginAttributes& aOriginAttributes, bool* aIsSecureURI);
 
   mozilla::ipc::IPCResult RecvAccumulateMixedContentHSTS(

diff --git a/dom/ipc/PContent.ipdl b/dom/ipc/PContent.ipdl
@@ -998,7 +998,7 @@ parent:
 
     async InitCrashReporter(NativeThreadId tid);
 
-    sync IsSecureURI(uint32_t aType, nsIURI aURI, uint32_t aFlags,
+    sync IsSecureURI(nsIURI aURI, uint32_t aFlags,
                      OriginAttributes aOriginAttributes)
         returns (bool isSecureURI);
 

diff --git a/dom/security/nsMixedContentBlocker.cpp b/dom/security/nsMixedContentBlocker.cpp
@@ -979,8 +979,7 @@ void nsMixedContentBlocker::AccumulateMixedContentHSTS(
   if (NS_FAILED(rv)) {
     return;
   }
-  rv = sss->IsSecureURI(nsISiteSecurityService::HEADER_HSTS, aURI, 0,
-                        aOriginAttributes, nullptr, nullptr, &hsts);
+  rv = sss->IsSecureURI(aURI, 0, aOriginAttributes, nullptr, nullptr, &hsts);
   if (NS_FAILED(rv)) {
     return;
   }

diff --git a/netwerk/base/nsNetUtil.cpp b/netwerk/base/nsNetUtil.cpp
@@ -2964,9 +2964,9 @@ nsresult NS_ShouldSecureUpgrade(
                resultCallback{std::move(aResultCallback)}]() mutable {
                 uint32_t hstsSource = 0;
                 bool isStsHost = false;
-                nsresult rv = service->IsSecureURI(
-                    nsISiteSecurityService::HEADER_HSTS, uri, flags,
-                    originAttributes, nullptr, &hstsSource, &isStsHost);
+                nsresult rv =
+                    service->IsSecureURI(uri, flags, originAttributes, nullptr,
+                                         &hstsSource, &isStsHost);
 
                 // Successfully get the result from |IsSecureURI| implies that
                 // the storage is ready to read.
@@ -2985,9 +2985,8 @@ nsresult NS_ShouldSecureUpgrade(
       return rv;
     }
 
-    nsresult rv =
-        sss->IsSecureURI(nsISiteSecurityService::HEADER_HSTS, aURI, flags,
-                         aOriginAttributes, nullptr, &hstsSource, &isStsHost);
+    nsresult rv = sss->IsSecureURI(aURI, flags, aOriginAttributes, nullptr,
+                                   &hstsSource, &isStsHost);
 
     // if the SSS check fails, it's likely because this load is on a
     // malformed URI or something else in the setup is wrong, so any error

diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp
@@ -1785,8 +1785,7 @@ nsresult nsHttpChannel::ProcessHSTSHeader(nsITransportSecurityInfo* aSecInfo,
 
   uint32_t failureResult;
   uint32_t headerSource = nsISiteSecurityService::SOURCE_ORGANIC_REQUEST;
-  rv = sss->ProcessHeader(nsISiteSecurityService::HEADER_HSTS, mURI,
-                          securityHeader, aSecInfo, aFlags, headerSource,
+  rv = sss->ProcessHeader(mURI, securityHeader, aSecInfo, aFlags, headerSource,
                           originAttributes, nullptr, nullptr, &failureResult);
   if (NS_FAILED(rv)) {
     nsAutoString consoleErrorCategory(u"Invalid HSTS Headers"_ns);

diff --git a/netwerk/protocol/http/nsHttpHandler.cpp b/netwerk/protocol/http/nsHttpHandler.cpp
@@ -2436,9 +2436,8 @@ nsresult nsHttpHandler::SpeculativeConnectInternal(
       aURI, originAttributes);
 
   nsCOMPtr<nsIURI> clone;
-  if (NS_SUCCEEDED(sss->IsSecureURI(nsISiteSecurityService::HEADER_HSTS, aURI,
-                                    flags, originAttributes, nullptr, nullptr,
-                                    &isStsHost)) &&
+  if (NS_SUCCEEDED(sss->IsSecureURI(aURI, flags, originAttributes, nullptr,
+                                    nullptr, &isStsHost)) &&
       isStsHost) {
     if (NS_SUCCEEDED(NS_GetSecureUpgradedURI(aURI, getter_AddRefs(clone)))) {
       aURI = clone.get();

diff --git a/security/manager/ssl/PublicKeyPinningService.cpp b/security/manager/ssl/PublicKeyPinningService.cpp
@@ -15,6 +15,7 @@
 #include "mozilla/Telemetry.h"
 #include "nsDependentString.h"
 #include "nsServiceManagerUtils.h"
+#include "nsSiteSecurityService.h"
 #include "mozpkix/pkixtypes.h"
 #include "mozpkix/pkixutil.h"
 #include "seccomon.h"
@@ -28,6 +29,8 @@ using namespace mozilla::psm;
 
 LazyLogModule gPublicKeyPinningLog("PublicKeyPinningService");
 
+NS_IMPL_ISUPPORTS(PublicKeyPinningService, nsIPublicKeyPinningService)
+
 enum class PinningMode : uint32_t {
   Disabled = 0,
   AllowUserCAMITM = 1,
@@ -348,24 +351,32 @@ nsresult PublicKeyPinningService::ChainHasValidPins(
                               pinningTelemetryInfo);
 }
 
-nsresult PublicKeyPinningService::HostHasPins(const char* hostname,
-                                              mozilla::pkix::Time time,
-                                              /*out*/ bool& hostHasPins) {
-  hostHasPins = false;
+NS_IMETHODIMP
+PublicKeyPinningService::HostHasPins(nsIURI* aURI, bool* hostHasPins) {
+  NS_ENSURE_ARG(aURI);
+  NS_ENSURE_ARG(hostHasPins);
+  *hostHasPins = false;
   PinningMode pinningMode(GetPinningMode());
   if (pinningMode == PinningMode::Disabled) {
     return NS_OK;
   }
-  nsAutoCString canonicalizedHostname(CanonicalizeHostname(hostname));
+  nsAutoCString hostname;
+  nsresult rv = nsSiteSecurityService::GetHost(aURI, hostname);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+  if (nsSiteSecurityService::HostIsIPAddress(hostname)) {
+    return NS_OK;
+  }
+
   const TransportSecurityPreload* staticFingerprints = nullptr;
-  nsresult rv = FindPinningInformation(canonicalizedHostname.get(), time,
-                                       staticFingerprints);
+  rv = FindPinningInformation(hostname.get(), Now(), staticFingerprints);
   if (NS_FAILED(rv)) {
     return rv;
   }
   if (staticFingerprints) {
-    hostHasPins = !staticFingerprints->mTestMode ||
-                  pinningMode == PinningMode::EnforceTestMode;
+    *hostHasPins = !staticFingerprints->mTestMode ||
+                   pinningMode == PinningMode::EnforceTestMode;
   }
   return NS_OK;
 }

diff --git a/security/manager/ssl/PublicKeyPinningService.h b/security/manager/ssl/PublicKeyPinningService.h
@@ -6,9 +6,7 @@
 #define PublicKeyPinningService_h
 
 #include "CertVerifier.h"
-#include "ScopedNSSTypes.h"
-#include "cert.h"
-#include "nsNSSCertificate.h"
+#include "nsIPublicKeyPinningService.h"
 #include "nsString.h"
 #include "nsTArray.h"
 #include "mozilla/Span.h"
@@ -17,8 +15,13 @@
 namespace mozilla {
 namespace psm {
 
-class PublicKeyPinningService {
+class PublicKeyPinningService final : public nsIPublicKeyPinningService {
  public:
+  PublicKeyPinningService() = default;
+
+  NS_DECL_THREADSAFE_ISUPPORTS
+  NS_DECL_NSIPUBLICKEYPINNINGSERVICE
+
   /**
    * Sets chainHasValidPins to true if the given (host, certList) passes pinning
    * checks, or to false otherwise. If the host is pinned, returns true via
@@ -34,20 +37,15 @@ class PublicKeyPinningService {
       /*out*/ bool& chainHasValidPins,
       /*optional out*/ PinningTelemetryInfo* pinningTelemetryInfo);
 
-  /**
-   * 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.
-   */
-  static nsresult HostHasPins(const char* hostname, mozilla::pkix::Time time,
-                              /*out*/ bool& hostHasPins);
-
   /**
    * Given a hostname of potentially mixed case with potentially multiple
    * trailing '.' (see bug 1118522), canonicalizes it to lowercase with no
    * trailing '.'.
    */
   static nsAutoCString CanonicalizeHostname(const char* hostname);
+
+ private:
+  ~PublicKeyPinningService() = default;
 };
 
 }  // namespace psm

diff --git a/security/manager/ssl/SSLServerCertVerification.cpp b/security/manager/ssl/SSLServerCertVerification.cpp
@@ -117,6 +117,7 @@
 #include "nsComponentManagerUtils.h"
 #include "nsContentUtils.h"
 #include "nsICertOverrideService.h"
+#include "nsIPublicKeyPinningService.h"
 #include "nsISiteSecurityService.h"
 #include "nsISocketProvider.h"
 #include "nsThreadPool.h"
@@ -428,18 +429,24 @@ static nsresult OverrideAllowedForHost(
     return rv;
   }
 
-  rv = sss->IsSecureURI(nsISiteSecurityService::HEADER_HSTS, uri,
-                        aProviderFlags, aOriginAttributes, nullptr, nullptr,
-                        &strictTransportSecurityEnabled);
+  rv = sss->IsSecureURI(uri, aProviderFlags, aOriginAttributes, nullptr,
+                        nullptr, &strictTransportSecurityEnabled);
   if (NS_FAILED(rv)) {
     MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
             ("[0x%" PRIx64 "] checking for HSTS failed", aPtrForLog));
     return rv;
   }
 
-  rv = sss->IsSecureURI(nsISiteSecurityService::STATIC_PINNING, uri,
-                        aProviderFlags, aOriginAttributes, nullptr, nullptr,
-                        &isStaticallyPinned);
+  nsCOMPtr<nsIPublicKeyPinningService> pkps =
+      do_GetService(NS_PKPSERVICE_CONTRACTID, &rv);
+  if (!pkps) {
+    MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
+            ("[0x%" PRIx64
+             "] Couldn't get nsIPublicKeyPinningService to check pinning",
+             aPtrForLog));
+    return NS_ERROR_FAILURE;
+  }
+  rv = pkps->HostHasPins(uri, &isStaticallyPinned);
   if (NS_FAILED(rv)) {
     MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
             ("[0x%" PRIx64 "] checking for static pin failed", aPtrForLog));

diff --git a/security/manager/ssl/components.conf b/security/manager/ssl/components.conf
@@ -147,6 +147,12 @@ Classes = [
         'headers': ['/security/manager/ssl/cert_storage/src/cert_storage.h'],
         'legacy_constructor': 'cert_storage_constructor',
     },
+    {
+        'cid': '{f64432b9-e8c6-41b4-b2da-8eb004344bba}',
+        'contract_ids': ['@mozilla.org/security/publickeypinningservice;1'],
+        'type': 'psm::PublicKeyPinningService',
+        'headers': ['/security/manager/ssl/PublicKeyPinningService.h'],
+    },
 ]
 
 if defined('MOZ_XUL'):

diff --git a/security/manager/ssl/moz.build b/security/manager/ssl/moz.build
@@ -36,6 +36,7 @@ XPIDL_SOURCES += [
     "nsIPKCS11ModuleDB.idl",
     "nsIPKCS11Slot.idl",
     "nsIProtectedAuthThread.idl",
+    "nsIPublicKeyPinningService.idl",
     "nsISecretDecoderRing.idl",
     "nsISecurityUITelemetry.idl",
     "nsISiteSecurityService.idl",