Fix dictionary domain check problem.

net/base/sdch_manager.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/base/sdch_manager.h"
 
 #include "base/base64.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "crypto/sha2.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "net/url_request/url_request_http_job.h"
 
+namespace {
+
+// Canonicalize a URL from possibly containing a FQDN to the HDN format
+// specified in RFC 2965 so that trailing periods for (e.g.) domain matching
+// are not a concern.
+bool FQDNToHDN(std::string* host) {

[Ryan Sleevi, 2014/09/18 01:22:31]

1) Naming: "HDN" is not common
2) RFC 2965 is obsoleted by abarth's RFC 6265
3) HDN refers to a cookie-level concept that I don't think applies here (that
is, foo.com == foo.com., whereas for HDN, foo.com != foo.com. ) (c.f. 4.1.2.3 of
6265)

[Randy Smith (Not in Mondays), 2014/09/22 19:22:10]

Changed to "StripTrailingDot" and edited the explanatory text.  I'll note for
possible future discussion: I wouldn't mind having functions that refer directly
to the controlling terminology in the RFCs (which is where FQDNToHDN came from).
 However, based on our discussion, I don't think the relationship is precisely
specified in the RFCs, so I'll go with the concrete description of what I'm
doing.

+  if (host->at(host->size() - 1) != '.')

[Ryan Sleevi, 2014/09/18 01:22:31]

Why are you using at (which throws) vs [] (which doesn't?)

[Randy Smith (Not in Mondays), 2014/09/22 19:22:10]

Because I found (*host)[] awkward and wasn't aware of the exceptional behavior
:-}.  Done.

+    return false;
+
+  host->resize(host->size() - 1);
+  return true;
+}
+
+void FQDNToHDNURL(GURL* gurl) {
+  std::string host(gurl->host());
+
+  if (!FQDNToHDN(&host))
+    return;
+
+  GURL::Replacements replacements;
+  replacements.SetHostStr(host);
+  *gurl = gurl->ReplaceComponents(replacements);
+  return;
+}
+
+}  // namespace
+
 namespace net {
 
 //------------------------------------------------------------------------------
 // static
 
 // Adjust SDCH limits downwards for mobile.
 #if defined(OS_ANDROID) || defined(OS_IOS)
 // static
 const size_t SdchManager::kMaxDictionaryCount = 1;
 const size_t SdchManager::kMaxDictionarySize = 500 * 1000;
@@ skipping 372 matching lines @@
   }
 
   return true;
 }
 
 void SdchManager::GetVcdiffDictionary(
     const std::string& server_hash,
     const GURL& referring_url,
     scoped_refptr<Dictionary>* dictionary) {
   DCHECK(CalledOnValidThread());
+
+  GURL referring_url_hdn(referring_url);
+  FQDNToHDNURL(&referring_url_hdn);
+
   *dictionary = NULL;
   DictionaryMap::iterator it = dictionaries_.find(server_hash);
   if (it == dictionaries_.end()) {
     return;
   }
   scoped_refptr<Dictionary> matching_dictionary = it->second;
-  if (!IsInSupportedDomain(referring_url))
+  if (!IsInSupportedDomain(referring_url_hdn))
     return;
-  if (!matching_dictionary->CanUse(referring_url))
+  if (!matching_dictionary->CanUse(referring_url_hdn))

[Ryan Sleevi, 2014/09/18 01:22:30]

1) Why do these functions use GURLs, rather than just hosts?
2) Rather than stripping '.', why can you not canonicalize by appending '.' if
necessary?

The latter is what we do in X509Certificate (for RFC6125 hostname matching).

[Randy Smith (Not in Mondays), 2014/09/22 19:22:10]

I'm presuming you mean IsInSupportedDomain() and CanUse().  CanUse() checks path
& port as well as host.  IsInSupportedDomain() checks the scheme as well as the
host/domain.  IsInSupportedDomain() is problematic for other reasons, and I'd be
open to a refactor (though I'd want to do it in such a way as to not cause even
more problems for CL 423813002) but I'm basically happy with the abstraction
boundary for CanUse().
So I'm willing to do that for consistency sake, and will if you ask me again,
but I'd rather keep the code and spec in alignment.  I.e. if the spec prohibits
a trailing "." for dictionary domains, I'd rather keep the dictionary domain
free of the trailing ".", and strip it from the URL if it's there.

     return;
   *dictionary = matching_dictionary;
 }
 
 // TODO(jar): If we have evictions from the dictionaries_, then we need to
 // change this interface to return a list of reference counted Dictionary
 // instances that can be used if/when a server specifies one.
 void SdchManager::GetAvailDictionaryList(const GURL& target_url,
                                          std::string* list) {
   DCHECK(CalledOnValidThread());
+
+  GURL target_url_hdn(target_url);
+  FQDNToHDNURL(&target_url_hdn);
+
   int count = 0;
   for (DictionaryMap::iterator it = dictionaries_.begin();
        it != dictionaries_.end(); ++it) {
-    if (!IsInSupportedDomain(target_url))
+    if (!IsInSupportedDomain(target_url_hdn))
       continue;
-    if (!it->second->CanAdvertise(target_url))
+    if (!it->second->CanAdvertise(target_url_hdn))
       continue;
     ++count;
     if (!list->empty())
       list->append(",");
     list->append(it->second->client_hash());
   }
   // Watch to see if we have corrupt or numerous dictionaries.
   if (count > 0)
     UMA_HISTOGRAM_COUNTS("Sdch3.Advertisement_Count", count);
 }
@@ skipping 100 matching lines @@
         if (port >= 0)
           ports.insert(port);
       }
     }
 
     if (line_end >= header_end)
       break;
     line_start = line_end + 1;
   }
 
-  if (!IsInSupportedDomain(dictionary_url))
+  // Convert away from FQDN to the HDN format described in RFC 2695.
+  FQDNToHDN(&domain);
+  GURL dictionary_url_hdn(dictionary_url);
+  FQDNToHDNURL(&dictionary_url_hdn);
+
+  if (!IsInSupportedDomain(dictionary_url_hdn))
     return;
 
-  if (!Dictionary::CanSet(domain, path, ports, dictionary_url))
+  if (!Dictionary::CanSet(domain, path, ports, dictionary_url_hdn))
     return;
 
   // TODO(jar): Remove these hacks to preclude a DOS attack involving piles of
   // useless dictionaries.  We should probably have a cache eviction plan,
   // instead of just blocking additions.  For now, with the spec in flux, it
   // is probably not worth doing eviction handling.
   if (kMaxDictionarySize < dictionary_text.size()) {
     SdchErrorRecovery(DICTIONARY_IS_TOO_LARGE);
     return;
   }
   if (kMaxDictionaryCount <= dictionaries_.size()) {
     SdchErrorRecovery(DICTIONARY_COUNT_EXCEEDED);
     return;
   }
 
   UMA_HISTOGRAM_COUNTS("Sdch3.Dictionary size loaded", dictionary_text.size());
   DVLOG(1) << "Loaded dictionary with client hash " << client_hash
            << " and server hash " << server_hash;
   Dictionary* dictionary =
       new Dictionary(dictionary_text, header_end + 2, client_hash,
-                     dictionary_url, domain, path, expiration, ports);
+                     dictionary_url_hdn, domain, path, expiration, ports);
   dictionaries_[server_hash] = dictionary;
   return;
 }
 
 // static
 void SdchManager::UrlSafeBase64Encode(const std::string& input,
                                       std::string* output) {
   // Since this is only done during a dictionary load, and hashes are only 8
   // characters, we just do the simple fixup, rather than rewriting the encoder.
   base::Base64Encode(input, output);
   std::replace(output->begin(), output->end(), '+', '-');
   std::replace(output->begin(), output->end(), '/', '_');
 }
 
 }  // namespace net