Domain Reliability: More security review.

components/domain_reliability/scheduler.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/scheduler.h"
 
 #include <algorithm>
 
 #include "base/metrics/field_trial.h"
 #include "base/strings/string_number_conversions.h"
@@ skipping 19 matching lines @@
     return default_value;
 
   std::string group_name = base::FieldTrialList::FindFullName(field_trial_name);
   int value;
   if (!base::StringToInt(group_name, &value)) {
     LOG(ERROR) << "Expected integer for field trial " << field_trial_name
                << " group name, but got \"" << group_name << "\".";
     return default_value;
   }
 
   return value;

[Ryan Sleevi, 2014/04/26 02:37:39]

Why Int? Why not UInt?

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

Done.

 }
 
 }  // namespace
 
 namespace domain_reliability {
 
 // static
 DomainReliabilityScheduler::Params
 DomainReliabilityScheduler::Params::GetFromFieldTrialsOrDefaults() {
   DomainReliabilityScheduler::Params params;
@@ skipping 108 matching lines @@
   base::TimeDelta min_delay = std::max(min_by_deadline, min_by_backoff) - now;
   base::TimeDelta max_delay = std::max(max_by_deadline, min_by_backoff) - now;
 
   VLOG(1) << "Scheduling upload for between " << min_delay.InSeconds()
           << " and " << max_delay.InSeconds() << " seconds from now.";
 
   callback_.Run(min_delay, max_delay);
 }
 
 // TODO(ttuttle): Add min and max interval to config, use that instead.
+
 // TODO(ttuttle): Cap min and max intervals received from config.
 
 void DomainReliabilityScheduler::GetNextUploadTimeAndCollector(
     base::TimeTicks now,
     base::TimeTicks* upload_time_out,
     int* collector_index_out) {
   DCHECK(upload_time_out);
   DCHECK(collector_index_out);
 
   base::TimeTicks min_time;
   int min_index = kInvalidCollectorIndex;
 
   for (unsigned i = 0; i < collectors_.size(); ++i) {

[Ryan Sleevi, 2014/04/26 02:37:39]

s/unsigned/size_t/ - use the proper type.

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

Done.

     CollectorState* collector = &collectors_[i];
     // If a collector is usable, use the first one in the list.
     if (collector->failures == 0 || collector->next_upload <= now) {
       min_time = now;
       min_index = i;

[Ryan Sleevi, 2014/04/26 02:37:39]

SECURITY BUG: You just sunk my battleship :(

You coerce an unsigned to a signed here. So now we have a chance of getting a
negative number into *collector_index_out (for a large value of "i".

On line 99, you store this value into collector_index_.

If I call OnUploadComplete after this, you've now just walked backward in the
stack. And broken my heart. And my security, because you write to it in (line
119/126)

This is why you always leave a note... or always use the right integer types :)

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

Done.

       break;
     // If not, keep track of which will be usable soonest:
     } else if (min_index == kInvalidCollectorIndex ||
         collector->next_upload < min_time) {
       min_time = collector->next_upload;
       min_index = i;
     }
   }
 
   DCHECK_NE(kInvalidCollectorIndex, min_index);
   *upload_time_out = min_time;
   *collector_index_out = min_index;
 }
 
 base::TimeDelta DomainReliabilityScheduler::GetUploadRetryInterval(
     int failures) {
   if (failures == 0)
     return base::TimeDelta::FromSeconds(0);
   else {
     return params_.upload_retry_interval * (1 << std::min(failures - 1, 5));
   }
 }
 
 }  // namespace domain_reliability