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 {
+
+// Spec collision warning: URLs may have domain names which have a
+// trailing "." (e.g. "http://this.is.a.domain.com./file.txt").  The
+// SDCH spec was written to parallel RFC 2965 (an old version of the
+// cookie spec, which didn't precisely address the issue of trailing "."s.
+// The newest version of the cookie spec, RFC 6265, specifically prohibits
+// trailing "."s from cookie domains.  So we prohibit trailing "."s in
+// dictionary domains, and strip any trailing "." from URL domains.
+bool StripTrailingDot(std::string* host) {
+  if (host->size() == 0)

[Ryan Sleevi, 2014/09/22 20:30:37]

host->empty()

(a nit I hold on to from the days of 'dumb' stl implementations, but also the
generic form of not requiring counting like other containers MAY)

[Randy Smith (Not in Mondays), 2014/09/23 19:52:58]

Done.

+    return false;
+
+  if ((*host)[host->size() - 1] != '.')

[Ryan Sleevi, 2014/09/22 20:30:36]

if (*host->rbegin() != '.')
  return false;

Laziness is a virtue :)

[Randy Smith (Not in Mondays), 2014/09/23 19:52:58]

Agreed :-}.  Done.

+    return false;
+
+  host->resize(host->size() - 1);
+  return true;
+}
+
+void StripTrailingDotURL(GURL* gurl) {

[Ryan Sleevi, 2014/09/22 20:30:36]

naming wise, this felt a little weird, although I understand how you got here.

Perhaps just something as "NormalizeToDictionaryURL"?

[Randy Smith (Not in Mondays), 2014/09/23 19:52:58]

I don't want "NormalizeToDictionary" because to me that pre-supposes the results
of our discussion about the spec framework this code is to be written in. 
Having said that, I agree with your next comment, which frees up
"StripTrailingDot", so I'll use that.

+  std::string host(gurl->host());
+
+  if (!StripTrailingDot(&host))
+    return;

[Ryan Sleevi, 2014/09/22 20:30:36]

Just inline this? You never use "StripTrailingDot" in this file

[Randy Smith (Not in Mondays), 2014/09/23 19:52:58]

Done.

+
+  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 96 matching lines @@
         registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES).empty()) {
     SdchErrorRecovery(DICTIONARY_SPECIFIES_TOP_LEVEL_DOMAIN);
     return false;  // domain was a TLD.
   }
   if (!Dictionary::DomainMatch(dictionary_url, domain)) {
     SdchErrorRecovery(DICTIONARY_DOMAIN_NOT_MATCHING_SOURCE_URL);
     return false;
   }
 
   std::string referrer_url_host = dictionary_url.host();
   size_t postfix_domain_index = referrer_url_host.rfind(domain);

[Ryan Sleevi, 2014/09/22 22:49:42]

BUG: Check your npos results! Bad things happen here if .rfind(domain) == npos
(don't assume line 160 is gonna save your bacon)

[Randy Smith (Not in Mondays), 2014/09/23 19:52:58]

Ignoring all comments on untouched code for this CL (applies to several
following comments as well).

   // See if it is indeed a postfix, or just an internal string.
   if (referrer_url_host.size() == postfix_domain_index + domain.size()) {

[Ryan Sleevi, 2014/09/22 22:49:42]

I find this logic really, really weird.

Why not just use EndsWith?

std::string dictionary_url_host = dictionary_url.host();
if (EndsWith(dictionary_url_host, domain, false) &&
    domain.size() != referrer_url_host.size() &&
    dictionary_url_host.find(".", 0, dictionary_url_host.size() - domain.size()
- 1) != std::string::npos) {
}

The first ensures that www.google.com ends with google.com. This is really weird
to check, since you just did this check on line 160

The next would be to check to make sure they both aren't google.com (since that
prefix will never have a leading .)

Then you check to see if the space [www.] has a dot (which it does), indicating
it's a violation, based on what you said on line 139-141

     // It is a postfix... so check to see if there's a dot in the prefix.
     size_t end_of_host_index = referrer_url_host.find_first_of('.');
     if (referrer_url_host.npos != end_of_host_index  &&
         end_of_host_index < postfix_domain_index) {
       SdchErrorRecovery(DICTIONARY_REFERER_URL_HAS_DOT_IN_PREFIX);
       return false;
     }
   }
 
   if (!ports.empty()
@@ skipping 253 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);
+  StripTrailingDotURL(&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/22 20:30:37]

OK, I'm fairly confused now why it's necessary to do this.

SdchManager::Dictionary::CanUse uses DomainMatch, which just calls

gurl.DomainIs(...)

and DomainIs handles the trailing . just fine (c.f
https://code.google.com/p/chromium/codesearch#chromium/src/url/gurl.cc&rcl=14...
)

[Randy Smith (Not in Mondays), 2014/09/22 20:43:51]

This isn't necessary; it's the checks in AddSdchDictionary that are necessary. 
The failure that produced this CL was the referring URL for an added dictionary
having a "." at the end and also being a subdomain of the domain it was trying
to set an encoding dictionary for, which should have been bounced because you
aren't allowed to set dictionaries for parent domains (i.e. you can't set a
dictionary for google.com from www.hosted.google.com).  It's a pure suffix check
(as specified in the spec :-J), not done through DomainMatch, which failed when
the trailing "." was present.  

I put it here (and forgot to change the naming, sorry) because I was doing a
general change to strip trailing dots from referring URLs; I didn't want to have
code in one place that did the strip and in another that didn't.  If you'd like,
I can convert this to a comment to the effect that CanUse uses DomainMatch(),
which handles the trailing ".".  But I suspect what you want to focus on is what
the code in Dictionary::CanSet() and callers looks like, and then figure out
from that whether or not we want a change here.

     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);
+  StripTrailingDotURL(&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 28 matching lines @@
     allow_latency_experiment_.insert(url.host());
     return;
   }
   ExperimentSet::iterator it = allow_latency_experiment_.find(url.host());
   if (allow_latency_experiment_.end() == it)
     return;  // It was already erased, or never allowed.
   SdchErrorRecovery(LATENCY_TEST_DISALLOWED);
   allow_latency_experiment_.erase(it);
 }
 
 void SdchManager::AddSdchDictionary(const std::string& dictionary_text,

[Ryan Sleevi, 2014/09/22 20:55:26]

style nits: Perhaps for a future CL, but this should be on a line by itself, per
the style guide, or dictionary_url should be aligned to the open (

-    const GURL& dictionary_url) {
+    const GURL& dictionary_url_const) {

[Ryan Sleevi, 2014/09/22 20:55:26]

pedantry naming: dictionary_url here, and normalized_dictionary_url below
(which, yes, requires more code line changes)

I just find the _const an unnerving "Reverse Hungarian Notation" ;)

[Randy Smith (Not in Mondays), 2014/09/23 19:52:58]

Done.

   DCHECK(CalledOnValidThread());
   std::string client_hash;
   std::string server_hash;
   GenerateHash(dictionary_text, &client_hash, &server_hash);
   if (dictionaries_.find(server_hash) != dictionaries_.end()) {
     SdchErrorRecovery(DICTIONARY_ALREADY_LOADED);
     return;                             // Already loaded.
   }
 
   std::string domain, path;
   std::set<int> ports;
   base::Time expiration(base::Time::Now() + base::TimeDelta::FromDays(30));
 
   if (dictionary_text.empty()) {
     SdchErrorRecovery(DICTIONARY_HAS_NO_TEXT);
     return;                             // Missing header.
   }
 
   size_t header_end = dictionary_text.find("\n\n");
   if (std::string::npos == header_end) {
     SdchErrorRecovery(DICTIONARY_HAS_NO_HEADER);
     return;                             // Missing header.
   }
   size_t line_start = 0;  // Start of line being parsed.
   while (1) {
     size_t line_end = dictionary_text.find('\n', line_start);
     DCHECK(std::string::npos != line_end);
     DCHECK_LE(line_end, header_end);

[Ryan Sleevi, 2014/09/22 20:55:26]

This is really unnerving. Shouldn't these be non-CHECK, handled errors?

 
     size_t colon_index = dictionary_text.find(':', line_start);
     if (std::string::npos == colon_index) {
       SdchErrorRecovery(DICTIONARY_HEADER_LINE_MISSING_COLON);
       return;                         // Illegal line missing a colon.
     }
 
     if (colon_index > line_end)

[Ryan Sleevi, 2014/09/22 20:55:26]

if line_end == npos, this will always be false.

       break;
 
     size_t value_start = dictionary_text.find_first_not_of(" \t",
                                                            colon_index + 1);
     if (std::string::npos != value_start) {
       if (value_start >= line_end)
         break;
       std::string name(dictionary_text, line_start, colon_index - line_start);
       std::string value(dictionary_text, value_start, line_end - value_start);
       name = base::StringToLowerASCII(name);

[Ryan Sleevi, 2014/09/22 20:55:26]

Such wasteful copying :'(

name == could be LowerCaseEqualsASCII(), these could be base::StringPieces, cats
and dogs could live together in harmony...

       if (name == "domain") {
         domain = value;
       } else if (name == "path") {
         path = value;
       } else if (name == "format-version") {
         if (value != "1.0")
           return;
       } else if (name == "max-age") {
         int64 seconds;
         base::StringToInt64(value, &seconds);

[Ryan Sleevi, 2014/09/22 20:55:26]

Unchecked return value? Killing me smalls, KILLING ME :)

         expiration = base::Time::Now() + base::TimeDelta::FromSeconds(seconds);
       } else if (name == "port") {
         int port;
         base::StringToInt(value, &port);

[Ryan Sleevi, 2014/09/22 20:55:26]

This too!

         if (port >= 0)
           ports.insert(port);
       }
     }
 
     if (line_end >= header_end)
       break;
     line_start = line_end + 1;

[Ryan Sleevi, 2014/09/22 20:55:27]

if line_end == npos, this will wrap around to 0 (since npos is typically -1,
it's an unsigned int, so will overflow)

However, because of 590, we'll never hit that (since npos will always be >
header_end), but it does mean we'll have parsed a line that lacked a terminator,
and violated your assumed DCHECKs.

I'd feel rather awesome if this used base/strings/string_tokenizer in the
future, but I'll leave that to a future CL. But yeah, complex parsing in //net,
scurry :)

   }
 
+  // Forbid dictionary with trailing dots; the SDCH spec is explicitly
+  // patterned after an old cookie spec (RFC 2695) which is silent on
+  // the issue of trailing dots, and the newer cookie spec (RFC 6265)
+  // explicitly forbids them.
+  if (domain.size() != 0 && domain[domain.size() - 1] == '.') {
+    SdchErrorRecovery(DICTIONARY_DOMAIN_HAS_TRAILING_PERIOD);
+    return;

[Ryan Sleevi, 2014/09/22 20:55:27]

From your reply, it _seems_ that this is the ONLY necessary change, is that
correct? All other bits are handled by DomainIs()?

[Randy Smith (Not in Mondays), 2014/09/22 21:09:56]

I don't believe so; I think the stripping of the dot from the
dictionary_url{_const} below is the most important point.  Again, the failing
example in the bug (now immortalized in one of the tests) was the dictionary_url
having a trailing ".", and the domain for the SDCH dictionary *not* having that
".".

[Ryan Sleevi, 2014/09/22 22:49:42]

I tried to work through why this is. That is, I'm trying to tease from you what
the _necessary_ changes were, versus which you were trying to do for code
health/consistency. Altogether, this feels like a much larger change at present,
and I have trouble reasoning about the correctness of it.

That is, broadly speaking, I'm trying to understand where you're making domain
comparisons and why these are problematic. Rather than normalizing all the entry
points, we should be trying to reduce the comparisons altogether, and, whenever
possible, defer to GURL (which will do the right thing)

IsInSupportedDomain - ok, sure, the blacklist considers google.com !=
google.com. . I can't tell if that's a feature (subdomain.foo.com != foo.com) or
a bug.

CanSet -
  - GetDomainAndRegistry (handles trailing .; I think)
  - DomainMatch(dictionary_url, domain) - does the right thing (re: GURL)

So the only place that it matters is when you start matching referrer_url_host
and postfix_domain_index. I've commented more in that function.

+  }
+
+  // Strip trailing dot from the dictionary url before comparison.
+  GURL dictionary_url(dictionary_url_const);
+  StripTrailingDotURL(&dictionary_url);
+
   if (!IsInSupportedDomain(dictionary_url))
     return;
 
   if (!Dictionary::CanSet(domain, path, ports, dictionary_url))
     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.
@@ skipping 20 matching lines @@
 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