Bug 1065909 - Canonicalize hostnames in nsSiteSecurityService and PublicKeyPinningService. r=mmc, a=sledru
authorDavid Keeler <dkeeler@mozilla.com>
Tue, 13 Jan 2015 11:18:03 -0800
changeset 242866 82cce51fb174
parent 242865 17b6018c53f0
child 242867 5ac62d0df17e
push id4322
push userryanvm@gmail.com
push date2015-01-14 15:18 +0000
treeherdermozilla-beta@82cce51fb174 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmmc, sledru
bugs1065909
milestone36.0
Bug 1065909 - Canonicalize hostnames in nsSiteSecurityService and PublicKeyPinningService. r=mmc, a=sledru
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
@@ -266,25 +266,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;
@@ -859,18 +865,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
@@ -1013,17 +1018,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);
@@ -1066,17 +1071,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
@@ -25,16 +25,17 @@ support-files =
 
 [test_sts_preloadlist_perwindowpb.js]
 # Bug 978426: Test fails consistently only on B2G ARM
 skip-if = buildapp == "b2g" && processor == "arm"
 
 [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]