bug 1065909 - canonicalize hostnames in nsSiteSecurityService and PublicKeyPinningService r=mmc
authorDavid Keeler <dkeeler@mozilla.com>
Fri, 09 Jan 2015 09:46:05 -0800
changeset 248891 f0cd9e9e87aa30a6c7f0d53c45a0417a6b4f281a
parent 248890 2487f8ac51dd31c1c39ac062a287f2287cb496f3
child 248892 7b8870e4821e481fcc58447f8b9e4c2421c6b5cd
push id4489
push userraliiev@mozilla.com
push dateMon, 23 Feb 2015 15:17:55 +0000
treeherdermozilla-beta@fd7c3dc24146 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmmc
bugs1065909
milestone37.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
bug 1065909 - canonicalize hostnames in nsSiteSecurityService and PublicKeyPinningService r=mmc
security/manager/boot/src/PublicKeyPinningService.cpp
security/manager/boot/src/PublicKeyPinningService.h
security/manager/boot/src/nsSiteSecurityService.cpp
security/manager/ssl/tests/unit/test_pinning.js
security/manager/ssl/tests/unit/test_sts_fqdn.js
security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp
security/manager/ssl/tests/unit/xpcshell.ini
--- a/security/manager/boot/src/PublicKeyPinningService.cpp
+++ b/security/manager/boot/src/PublicKeyPinningService.cpp
@@ -360,10 +360,23 @@ PublicKeyPinningService::ChainHasValidPi
                                            bool enforceTestMode)
 {
   if (!certList) {
     return false;
   }
   if (!hostname || hostname[0] == 0) {
     return CheckChainAgainstAllNames(certList, enforceTestMode, time);
   }
-  return CheckPinsForHostname(certList, hostname, enforceTestMode, time);
+  nsAutoCString canonicalizedHostname(CanonicalizeHostname(hostname));
+  return CheckPinsForHostname(certList, canonicalizedHostname.get(),
+                              enforceTestMode, time);
 }
+
+nsAutoCString
+PublicKeyPinningService::CanonicalizeHostname(const char* hostname)
+{
+  nsAutoCString canonicalizedHostname(hostname);
+  ToLowerCase(canonicalizedHostname);
+  while (canonicalizedHostname.Last() == '.') {
+    canonicalizedHostname.Truncate(canonicalizedHostname.Length() - 1);
+  }
+  return canonicalizedHostname;
+}
--- a/security/manager/boot/src/PublicKeyPinningService.h
+++ b/security/manager/boot/src/PublicKeyPinningService.h
@@ -33,13 +33,20 @@ public:
                                 bool enforceTestMode);
   /**
    * 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 bool ChainMatchesPinset(const CERTCertList* certList,
                                  const nsTArray<nsCString>& aSHA256keys);
+
+  /**
+   * 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);
 };
 
 }} // namespace mozilla::psm
 
 #endif // PublicKeyPinningServiceService_h
--- a/security/manager/boot/src/nsSiteSecurityService.cpp
+++ b/security/manager/boot/src/nsSiteSecurityService.cpp
@@ -265,25 +265,31 @@ nsSiteSecurityService::Init()
   if (!storageWillPersist) {
     NS_WARNING("site security information will not be persisted");
   }
 
   return NS_OK;
 }
 
 nsresult
-nsSiteSecurityService::GetHost(nsIURI *aURI, nsACString &aResult)
+nsSiteSecurityService::GetHost(nsIURI* aURI, nsACString& aResult)
 {
   nsCOMPtr<nsIURI> innerURI = NS_GetInnermostURI(aURI);
-  if (!innerURI) return NS_ERROR_FAILURE;
+  if (!innerURI) {
+    return NS_ERROR_FAILURE;
+  }
 
-  nsresult rv = innerURI->GetAsciiHost(aResult);
+  nsAutoCString host;
+  nsresult rv = innerURI->GetAsciiHost(host);
 
-  if (NS_FAILED(rv) || aResult.IsEmpty())
+  if (NS_FAILED(rv) || host.IsEmpty()) {
     return NS_ERROR_UNEXPECTED;
+  }
+
+  aResult.Assign(PublicKeyPinningService::CanonicalizeHostname(host.get()));
 
   return NS_OK;
 }
 
 static void
 SetStorageKey(nsAutoCString& storageKey, nsCString& hostname, uint32_t aType)
 {
   storageKey = hostname;
@@ -858,18 +864,17 @@ nsSiteSecurityService::IsSecureHost(uint
     *aResult = !PublicKeyPinningService::ChainHasValidPins(certList,
                                                            aHost,
                                                            mozilla::pkix::Now(),
                                                            false);
     return NS_OK;
   }
 
   // Holepunch chart.apis.google.com and subdomains.
-  nsAutoCString host(aHost);
-  ToLowerCase(host);
+  nsAutoCString host(PublicKeyPinningService::CanonicalizeHostname(aHost));
   if (host.EqualsLiteral("chart.apis.google.com") ||
       StringEndsWith(host, NS_LITERAL_CSTRING(".chart.apis.google.com"))) {
     return NS_OK;
   }
 
   const nsSTSPreload *preload = nullptr;
 
   // First check the exact host. This involves first checking for an entry in
@@ -979,17 +984,17 @@ nsSiteSecurityService::GetKeyPinsForHost
   NS_ENSURE_ARG(afound);
   NS_ENSURE_ARG(aHostname);
 
   SSSLOG(("Top of GetKeyPinsForHostname"));
   *afound = false;
   *aIncludeSubdomains = false;
   pinArray.Clear();
 
-  nsAutoCString host(aHostname);
+  nsAutoCString host(PublicKeyPinningService::CanonicalizeHostname(aHostname));
   nsAutoCString storageKey;
   SetStorageKey(storageKey, host, nsISiteSecurityService::HEADER_HPKP);
 
   SSSLOG(("storagekey '%s'\n", storageKey.get()));
   mozilla::DataStorageType storageType = mozilla::DataStorage_Persistent;
   nsCString value = mSiteStateStorage->Get(storageKey, storageType);
   // decode now
   SiteHPKPState foundEntry(value);
@@ -1032,17 +1037,18 @@ nsSiteSecurityService::SetKeyPins(const 
     if (!stringIsBase64EncodingOf256bitValue(pin)) {
       return NS_ERROR_INVALID_ARG;
     }
     sha256keys.AppendElement(pin);
   }
   SiteHPKPState dynamicEntry(expireTime, SecurityPropertySet,
                              aIncludeSubdomains, sha256keys);
   // we always store data in permanent storage (ie no flags)
-  return SetHPKPState(aHost, dynamicEntry, 0);
+  nsAutoCString host(PublicKeyPinningService::CanonicalizeHostname(aHost));
+  return SetHPKPState(host.get(), dynamicEntry, 0);
 }
 
 nsresult
 nsSiteSecurityService::SetHPKPState(const char* aHost, SiteHPKPState& entry,
                                     uint32_t aFlags)
 {
   SSSLOG(("Top of SetPKPState"));
   nsAutoCString host(aHost);
--- a/security/manager/ssl/tests/unit/test_pinning.js
+++ b/security/manager/ssl/tests/unit/test_pinning.js
@@ -42,16 +42,23 @@ function test_strict() {
   // precedence. This prevents overrides for such hosts.
   add_connection_test("unknownissuer.include-subdomains.pinning.example.com",
     getXPCOMStatusFromNSS(MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE));
 
   // Issued by otherCA, which is not in the pinset for pinning.example.com.
   add_connection_test("bad.include-subdomains.pinning.example.com",
     getXPCOMStatusFromNSS(MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE));
 
+  // Check that using a FQDN doesn't bypass pinning.
+  add_connection_test("bad.include-subdomains.pinning.example.com.",
+    getXPCOMStatusFromNSS(MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE));
+  // For some reason this is also navigable (see bug 1118522).
+  add_connection_test("bad.include-subdomains.pinning.example.com..",
+    getXPCOMStatusFromNSS(MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE));
+
   // These domains serve certs that match the pinset.
   add_connection_test("include-subdomains.pinning.example.com", Cr.NS_OK);
   add_connection_test("good.include-subdomains.pinning.example.com", Cr.NS_OK);
   add_connection_test("exclude-subdomains.pinning.example.com", Cr.NS_OK);
 
   // This domain serves a cert that doesn't match the pinset, but subdomains
   // are excluded.
   add_connection_test("sub.exclude-subdomains.pinning.example.com", Cr.NS_OK);
@@ -138,17 +145,17 @@ 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
-  do_check_eq(prod_histogram.counts[0], 4); // Failure count
+  do_check_eq(prod_histogram.counts[0], 6); // Failure count
   do_check_eq(prod_histogram.counts[1], 4); // Success count
   do_check_eq(test_histogram.counts[0], 2); // Failure count
   do_check_eq(test_histogram.counts[1], 0); // Success count
 
   let moz_prod_histogram = service.getHistogramById("CERT_PINNING_MOZ_RESULTS")
                              .snapshot();
   let moz_test_histogram =
     service.getHistogramById("CERT_PINNING_MOZ_TEST_RESULTS").snapshot();
new file mode 100644
--- /dev/null
+++ b/security/manager/ssl/tests/unit/test_sts_fqdn.js
@@ -0,0 +1,45 @@
+/* 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/.
+ */
+"use strict";
+
+function run_test() {
+  let SSService = Cc["@mozilla.org/ssservice;1"]
+                    .getService(Ci.nsISiteSecurityService);
+  do_check_false(SSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HSTS,
+                                        "example.com", 0));
+  do_check_false(SSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HSTS,
+                                        "example.com.", 0));
+  // These cases are only relevant as long as bug 1118522 hasn't been fixed.
+  do_check_false(SSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HSTS,
+                                        "example.com..", 0));
+
+  let uri = Services.io.newURI("https://example.com", null, null);
+  let sslStatus = new FakeSSLStatus();
+  SSService.processHeader(Ci.nsISiteSecurityService.HEADER_HSTS, uri,
+                          "max-age=1000;includeSubdomains", sslStatus, 0);
+  do_check_true(SSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HSTS,
+                                       "example.com", 0));
+  do_check_true(SSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HSTS,
+                                       "example.com.", 0));
+  do_check_true(SSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HSTS,
+                                       "example.com..", 0));
+
+  do_check_true(SSService.isSecureURI(Ci.nsISiteSecurityService.HEADER_HSTS,
+                                      uri, 0));
+  uri = Services.io.newURI("https://example.com.", null, null);
+  do_check_true(SSService.isSecureURI(Ci.nsISiteSecurityService.HEADER_HSTS,
+                                      uri, 0));
+  uri = Services.io.newURI("https://example.com..", null, null);
+  do_check_true(SSService.isSecureURI(Ci.nsISiteSecurityService.HEADER_HSTS,
+                                      uri, 0));
+
+  SSService.removeState(Ci.nsISiteSecurityService.HEADER_HSTS, uri, 0);
+  do_check_false(SSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HSTS,
+                                        "example.com", 0));
+  do_check_false(SSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HSTS,
+                                        "example.com.", 0));
+  do_check_false(SSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HSTS,
+                                        "example.com..", 0));
+}
--- a/security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp
+++ b/security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp
@@ -47,16 +47,18 @@ const BadCertHost sBadCertHosts[] =
   { "ca-used-as-end-entity-name-mismatch.example.com", "ca-used-as-end-entity" },
   // All of include-subdomains.pinning.example.com is pinned to End Entity
   // Test Cert with nick localhostAndExampleCom. Any other nick will only
   // pass pinning when security.cert_pinning.enforcement.level != strict and
   // otherCA is added as a user-specified trust anchor. See StaticHPKPins.h.
   { "include-subdomains.pinning.example.com", "localhostAndExampleCom" },
   { "good.include-subdomains.pinning.example.com", "localhostAndExampleCom" },
   { "bad.include-subdomains.pinning.example.com", "otherIssuerEE" },
+  { "bad.include-subdomains.pinning.example.com.", "otherIssuerEE" },
+  { "bad.include-subdomains.pinning.example.com..", "otherIssuerEE" },
   { "exclude-subdomains.pinning.example.com", "localhostAndExampleCom" },
   { "sub.exclude-subdomains.pinning.example.com", "otherIssuerEE" },
   { "test-mode.pinning.example.com", "otherIssuerEE" },
   { "unknownissuer.include-subdomains.pinning.example.com", "unknownissuer" },
   { "nsCertTypeNotCritical.example.com", "nsCertTypeNotCritical" },
   { "nsCertTypeCriticalWithExtKeyUsage.example.com", "nsCertTypeCriticalWithExtKeyUsage" },
   { "nsCertTypeCritical.example.com", "nsCertTypeCritical" },
   { "end-entity-issued-by-v1-cert.example.com", "eeIssuedByV1Cert" },
--- a/security/manager/ssl/tests/unit/xpcshell.ini
+++ b/security/manager/ssl/tests/unit/xpcshell.ini
@@ -23,16 +23,17 @@ support-files =
 [test_datasignatureverifier.js]
 [test_hash_algorithms.js]
 [test_hmac.js]
 
 [test_sts_preloadlist_perwindowpb.js]
 [test_sts_preloadlist_selfdestruct.js]
 [test_sts_holepunch.js]
 [test_sts_ipv4_ipv6.js]
+[test_sts_fqdn.js]
 
 [test_sss_eviction.js]
 [test_sss_readstate.js]
 [test_sss_readstate_empty.js]
 [test_sss_readstate_garbage.js]
 [test_sss_readstate_huge.js]
 [test_sss_savestate.js]