Domain Reliability: More security review.

components/domain_reliability/config.cc

 // 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.
 
 #include "components/domain_reliability/config.h"
 
 #include "base/json/json_reader.h"
 #include "base/json/json_value_converter.h"
 #include "base/rand_util.h"
 #include "base/strings/string_util.h"
 
 namespace {
 
 bool ConvertURL(const base::StringPiece& string_piece, GURL* url) {
   *url = GURL(string_piece.as_string());
   return url->is_valid();
 }
 
 bool IsValidSampleRate(double p) { return p >= 0.0 && p <= 1.0; }
 
 }  // namespace
 
 namespace domain_reliability {
 
 DomainReliabilityConfig::Resource::Resource() {}
-
 DomainReliabilityConfig::Resource::~Resource() {}
 
 bool DomainReliabilityConfig::Resource::MatchesUrlString(
     const std::string& url_string) const {
   ScopedVector<std::string>::const_iterator it;
 
   for (it = url_patterns.begin(); it != url_patterns.end(); it++) {
     if (MatchPattern(url_string, **it))
       return true;
   }
 
   return false;
 }

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

You should have unittests that cover this - in particular the wildcard matching
issues I described.

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

I don't think they're security-relevant, but I have beefed up the unittest to
try some simple wildcard examples.

 
 bool DomainReliabilityConfig::Resource::DecideIfShouldReportRequest(
     bool success) const {
   double sample_rate = success ? success_sample_rate : failure_sample_rate;
   return base::RandDouble() < sample_rate;
 }

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

API design: It's worth noting that calling this multiple times for a Resource
may return non-deterministic results, which can run into TOCTTOU issues

Perhaps you should pre-calculate this for a Resource?

Also, this can return 'bad' results if !IsValid()

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

We shouldn't have TOCTOU issues; once the config is installed into a context,
it's const, so the probabilities can't change. (Also, it's not a
security-critical result, just a coin toss.)
The idea is that it's not pre-calculated; we want a random sample of the
requests.  If we precalculated it, we'd get either 100% or 0% of the beacons for
a resource.
What's a good way to deal with this? If the probabilities are just out of range,
it'll act as if they're clamped to the nearest endpoint. I could just DCHECK
that it's a valid range before using it.

 
 // static
 void DomainReliabilityConfig::Resource::RegisterJSONConverter(
     base::JSONValueConverter<DomainReliabilityConfig::Resource>* converter) {
   converter->RegisterStringField("resource_name", &Resource::name);
   converter->RegisterRepeatedString("url_patterns", &Resource::url_patterns);
   converter->RegisterDoubleField("success_sample_rate",
                                  &Resource::success_sample_rate);
   converter->RegisterDoubleField("failure_sample_rate",
                                  &Resource::failure_sample_rate);
 }
 
 bool DomainReliabilityConfig::Resource::IsValid() const {
   return !name.empty() && !url_patterns.empty() &&
       IsValidSampleRate(success_sample_rate) &&
       IsValidSampleRate(failure_sample_rate);
 }
 
 DomainReliabilityConfig::Collector::Collector() {}
-
 DomainReliabilityConfig::Collector::~Collector() {}
 
 // static
 void DomainReliabilityConfig::Collector::RegisterJSONConverter(
     base::JSONValueConverter<DomainReliabilityConfig::Collector>* converter) {
   converter->RegisterCustomField<GURL>("upload_url", &Collector::upload_url,
                                        &ConvertURL);
 }
 
 bool DomainReliabilityConfig::Collector::IsValid() const {
   return upload_url.is_valid();
 }
 
 DomainReliabilityConfig::DomainReliabilityConfig() : valid_until(0.0) {}
-
 DomainReliabilityConfig::~DomainReliabilityConfig() {}
 
 // static
 scoped_ptr<const DomainReliabilityConfig> DomainReliabilityConfig::FromJSON(
     const base::StringPiece& json) {
   scoped_ptr<base::Value> value(base::JSONReader::Read(json));
   if (!value)
     return scoped_ptr<const DomainReliabilityConfig>();
 
   DomainReliabilityConfig* config = new DomainReliabilityConfig();
   base::JSONValueConverter<DomainReliabilityConfig> converter;
   if (!converter.Convert(*value, config)) {
     return scoped_ptr<const DomainReliabilityConfig>();
   }

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

style: inconsistent bracing (compare 93/94 and 84/85... and rest of file,
really)

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

Done.

 
   if (!config->IsValid())
     return scoped_ptr<const DomainReliabilityConfig>();
 
   return scoped_ptr<const DomainReliabilityConfig>(config);
 }
 
 bool DomainReliabilityConfig::IsValid() const {
   if (valid_until == 0.0 || domain.empty() || version.empty() ||

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

What about negative values in valid_until ?

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

They are very expired.  I would not suggest providing one.  They are not
invalid, though.

(JSONValueConverter has no way to mark a field as required, so I initialize it
to 0.0, and if it's still 0.0 when I'm done, I assume it was not there.)

       resources.empty() || collectors.empty()) {
     return false;
   }
 
   for (size_t i = 0; i < resources.size(); ++i) {
     if (!resources[i]->IsValid())
       return false;
   }
 
   for (size_t i = 0; i < collectors.size(); ++i) {
     if (!collectors[i]->IsValid())
       return false;
   }
 
   return true;
 }
 
 bool DomainReliabilityConfig::IsExpired(base::Time now) const {
   base::Time valid_until_time = base::Time::FromDoubleT(valid_until);

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

Note that this can fail if IsValid() isn't checked before hand.

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

Added DCHECK.

   return now > valid_until_time;
 }
 
 int DomainReliabilityConfig::GetResourceIndexForUrl(const GURL& url) const {
   const std::string& url_string = url.spec();

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

Can this potentially allow sites to perform username matching rules on their
collecting URLs?

Does it mean that they can potentially collect usernames?

I can't help but feel like |url| may need more sanitization. This is where
having a comprehensive set of unittests can easily answer that question by
"adding tests".

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

I think by the time the URL hits here, it doesn't have one, but that's a good
idea. I'm passing it through the same filter that's used for the referrer
header.

 
   for (size_t i = 0; i < resources.size(); ++i) {
     if (resources[i]->MatchesUrlString(url_string))
       return static_cast<int>(i);
   }
 
   return -1;
 }
 
 // static
 void DomainReliabilityConfig::RegisterJSONConverter(
     base::JSONValueConverter<DomainReliabilityConfig>* converter) {
   converter->RegisterStringField("config_version",
                                  &DomainReliabilityConfig::version);
   converter->RegisterDoubleField("config_valid_until",
                                  &DomainReliabilityConfig::valid_until);
   converter->RegisterStringField("monitored_domain",
                                  &DomainReliabilityConfig::domain);
   converter->RegisterRepeatedMessage("monitored_resources",
                                      &DomainReliabilityConfig::resources);
   converter->RegisterRepeatedMessage("collectors",
                                      &DomainReliabilityConfig::collectors);
 }
 
 }  // namespace domain_reliability