Normalize hostnames before searching for HSTS/HPKP preloads

net/http/transport_security_state.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/http/transport_security_state.h"
 
 #if defined(USE_OPENSSL)
 #include <openssl/ecdsa.h>
 #include <openssl/ssl.h>
 #else  // !defined(USE_OPENSSL)
@@ skipping 280 matching lines @@
 
   if (delegate_)
     delegate_->StateIsDirty(this);
 }
 
 // static
 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
     // name is >255 bytes. However, search terms can have those properties.
     return std::string();
   }
 
   for (size_t i = 0; new_host[i]; i += new_host[i] + 1) {
     const unsigned label_length = static_cast<unsigned>(new_host[i]);
     if (!label_length)
@@ skipping 176 matching lines @@
 // In the dispatch table, the zero character represents the "end of string"
 // (which is the *beginning* of a hostname since we process it backwards). The
 // value in that case is special -- rather than an offset to another trie node,
 // it contains the HSTS information: whether subdomains are included, pinsets
 // etc. If an "end of string" matches a period in the hostname then the
 // information is remembered because, if no more specific node is found, then
 // that information applies to the hostname.
 //
 // 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));
   BitReader reader(kPreloadedHSTSData, kPreloadedHSTSBits);
   size_t bit_offset = kHSTSRootPosition;
   static const char kEndOfString = 0;
   static const char kEndOfTable = 127;
 
   *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).
   size_t hostname_offset = hostname.size();
 
   for (;;) {
     // Seek to the desired location.
     if (!reader.Seek(bit_offset)) {
       return false;
     }
@@ skipping 196 matching lines @@
                                      const base::Time& expiry,
                                      bool include_subdomains,
                                      const HashValueVector& hashes) {
   DCHECK(CalledOnValidThread());
   AddHPKPInternal(host, base::Time::Now(), expiry, include_subdomains, hashes);
 }
 
 // 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;
   }
 
   DCHECK(result.domain_id != DOMAIN_NOT_PINNED);
 
   UMA_HISTOGRAM_SPARSE_SLOWLY(
       "Net.PublicKeyPinFailureDomain", result.domain_id);
 }
 
@@ skipping 35 matching lines @@
   DCHECK(CalledOnValidThread());
 
   out->sts.upgrade_mode = DomainState::MODE_FORCE_HTTPS;
   out->sts.include_subdomains = false;
   out->pkp.include_subdomains = false;
 
   if (!IsBuildTimely())
     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);
   out->pkp.domain = out->sts.domain;
   out->sts.include_subdomains = result.sts_include_subdomains;
   out->sts.last_observed = base::GetBuildTime();
   out->sts.upgrade_mode =
       TransportSecurityState::DomainState::MODE_DEFAULT;
   if (result.force_https) {
     out->sts.upgrade_mode =
@@ skipping 168 matching lines @@
 TransportSecurityState::DomainState::STSState::~STSState() {
 }
 
 TransportSecurityState::DomainState::PKPState::PKPState() {
 }
 
 TransportSecurityState::DomainState::PKPState::~PKPState() {
 }
 
 }  // namespace