Normalize hostnames before searching for HSTS/HPKP preloads

net/http/transport_security_state.cc

Index: net/http/transport_security_state.cc
diff --git a/net/http/transport_security_state.cc b/net/http/transport_security_state.cc
index 7147b48287f19f2bd9d1eaeb13e2beae24a5d340..ad48ef8236821cd40d9edad86691095382632366 100644
--- a/net/http/transport_security_state.cc
+++ b/net/http/transport_security_state.cc
@@ -298,7 +298,6 @@ std::string TransportSecurityState::CanonicalizeHost(const std::string& host) {
   // We cannot perform the operations as detailed in the spec here as |host|
   // has already undergone IDN processing before it reached us. Thus, we check
   // that there are no invalid characters in the host and lowercase the result.
-

[davidben, 2015/05/20 19:21:09]

Stray change?

[Ryan Sleevi, 2015/05/20 19:26:39]

Readability ;)

   std::string new_host;
   if (!DNSDomainFromDot(host, &new_host)) {
     // DNSDomainFromDot can fail if any label is > 63 bytes or if the whole
@@ -495,7 +494,7 @@ struct PreloadResult {
 //
 // Dispatch tables are always given in order, but the "end of string" (zero)
 // value always comes before an entry for '.'.
-bool DecodeHSTSPreloadRaw(const std::string& hostname,
+bool DecodeHSTSPreloadRaw(const std::string& search_hostname,
                           bool* out_found,
                           PreloadResult* out) {
   HuffmanDecoder huffman(kHSTSHuffmanTree, sizeof(kHSTSHuffmanTree));
@@ -506,9 +505,24 @@ bool DecodeHSTSPreloadRaw(const std::string& hostname,
 
   *out_found = false;
 
+  // Normalize any trailing '.' used for DNS suffix searches

[davidben, 2015/05/20 19:21:08]

Nit: period

+  std::string hostname = search_hostname;
+  size_t found = hostname.find_last_not_of('.');
+  if (found != std::string::npos) {
+    hostname.erase(found + 1);
+  } else {
+    hostname.clear();
+  }
+
+  // |hostname| has already undergone IDN conversion, so should be
+  // entirely A-Labels. The preload data is entirely normalized to
+  // lower case

[davidben, 2015/05/20 19:21:09]

Nit: period

+  base::StringToLowerASCII(&hostname);
+
   if (hostname.empty()) {
     return true;
   }
+
   // hostname_offset contains one more than the index of the current character
   // in the hostname that is being considered. It's one greater so that we can
   // represent the position just before the beginning (with zero).
@@ -725,14 +739,15 @@ void TransportSecurityState::AddHPKP(const std::string& host,
 // static
 bool TransportSecurityState::IsGooglePinnedProperty(const std::string& host) {
   PreloadResult result;
-  return DecodeHSTSPreload(host, &result) && result.has_pins &&
+  return !CanonicalizeHost(host).empty() && DecodeHSTSPreload(host, &result) &&

[davidben, 2015/05/20 19:21:09]

Would it be better to move the CanonicalizeHost check inside DecodeHSTSPreload?
Otherwise probably some new caller will forget to do it. And, as I understand,
that's closer to the old behavior?

+         result.has_pins &&
          kPinsets[result.pinset_id].accepted_pins == kGoogleAcceptableCerts;
 }
 
 // static
 void TransportSecurityState::ReportUMAOnPinFailure(const std::string& host) {
   PreloadResult result;
-  if (!DecodeHSTSPreload(host, &result) ||
+  if (CanonicalizeHost(host).empty() || !DecodeHSTSPreload(host, &result) ||
       !result.has_pins) {
     return;
   }
@@ -788,7 +803,7 @@ bool TransportSecurityState::GetStaticDomainState(const std::string& host,
     return false;
 
   PreloadResult result;
-  if (!DecodeHSTSPreload(host, &result))
+  if (CanonicalizeHost(host).empty() || !DecodeHSTSPreload(host, &result))
     return false;
 
   out->sts.domain = host.substr(result.hostname_offset);