Domain Reliability: More security review.

components/domain_reliability/config.h

 // Copyright 2014 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.
 
 #ifndef COMPONENTS_DOMAIN_RELIABILITY_CONFIG_H_
 #define COMPONENTS_DOMAIN_RELIABILITY_CONFIG_H_
 
 #include <string>
 #include <vector>
 
@@ skipping 15 matching lines @@
   // A particular resource named in the config -- includes a set of URL
   // patterns that the resource will match, along with sample rates for
   // successful and unsuccessful requests.
   class DOMAIN_RELIABILITY_EXPORT Resource {
    public:
     Resource();
     ~Resource();
 
     // Returns whether |url_string| matches at least one of the |url_patterns|
     // in this Resource.
     bool MatchesUrlString(const std::string& url_string) const;

[Ryan Sleevi, 2014/04/25 01:00:02]

Why do you pass this as "url_string", rather than a GURL, which gives somewhat
more precise control over matching?

In particular, I'm concerned whether you can confound the matching algorithm
(which is a rather simple MatchPattern) through the use of query arguments.

Consider a match pattern of http*://mydomain.com

Now, consider a URL of
  http://victim.com/?some_query_parameter=://mydomain.com

Yes, some_query_parameter SHOULD be URL encoded as %3A%2F%2Ftest when sent over
the network. However, depending on how the caller handles/converts the URL
string, they may have allowed attacker data through. Your implementation would
allow it, seeing it as

http(stuff matching wildcard)://mydomain.com, reporting the failure of
victim.com to mydomain.com.

[Deprecated (see juliatuttle), 2014/04/28 22:01:13]

The config is only consulted once we've checked monitored_domains -- that is a
very simple map lookup, and looks directly at the hostname of the URL.  (That'll
change to origin before we allow untrusted configs, but even now it wouldn't
allow shenanigans like the above.)

[Deprecated (see juliatuttle), 2014/04/28 22:01:13]

Hmm.  I was going to say I don't want to repeatedly call .spec(), but it looks
like that's a fairly low-cost operation. Lemme do that.

 
     // Returns whether a request (that was successful if |success| is true)
     // should be reported (with a full beacon). (The output is random; it
     // compares a random number to |success_sample_rate| or
     // |failure_sample_rate|.)
     bool DecideIfShouldReportRequest(bool success) const;
 
     // Registers with the JSONValueConverter so it will know how to convert the
     // JSON for a named resource into the struct.
     static void RegisterJSONConverter(
@@ skipping 39 matching lines @@
 
   DomainReliabilityConfig();
   ~DomainReliabilityConfig();
 
   // Uses the JSONValueConverter to parse the JSON for a config into a struct.
   static scoped_ptr<const DomainReliabilityConfig> FromJSON(
       const base::StringPiece& json);
 
   bool IsValid() const;
 
+  // Checks whether |now| is past the expiration time provided in the config.
   bool IsExpired(base::Time now) const;
 
   // Finds the index (in resources) of the first Resource that matches a
   // particular URL. Returns -1 if the URL is not matched by any Resources.
   int GetResourceIndexForUrl(const GURL& url) const;
 
   // Registers with the JSONValueConverter so it will know how to convert the
   // JSON for a config into the struct.
   static void RegisterJSONConverter(
       base::JSONValueConverter<DomainReliabilityConfig>* converter);
 
   std::string version;
   double valid_until;
   std::string domain;
   ScopedVector<Resource> resources;
   ScopedVector<Collector> collectors;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(DomainReliabilityConfig);
 };
 
 }  // namespace domain_reliability
 
 #endif  // COMPONENTS_DOMAIN_RELIABILITY_CONFIG_H_