Simple Cache: validate lengths before allocations.

net/disk_cache/simple/simple_synchronous_entry.cc

 // Copyright (c) 2013 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/disk_cache/simple/simple_synchronous_entry.h"
 
 #include <algorithm>
 #include <cstring>
 #include <functional>
 #include <limits>
@@ skipping 647 matching lines @@
 }
 
 void SimpleSynchronousEntry::CheckEOFRecord(int index,
                                             const SimpleEntryStat& entry_stat,
                                             uint32_t expected_crc32,
                                             int* out_result) const {
   DCHECK(initialized_);
   uint32_t crc32;
   bool has_crc32;
   bool has_key_sha256;
-  int stream_size;
+  int32_t stream_size;
   *out_result = GetEOFRecordData(index, entry_stat, &has_crc32, &has_key_sha256,
                                  &crc32, &stream_size);
   if (*out_result != net::OK) {
     Doom();
     return;
   }
   if (has_crc32 && crc32 != expected_crc32) {
     DVLOG(1) << "EOF record had bad crc.";
     *out_result = net::ERR_CACHE_CHECKSUM_MISMATCH;
     RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_CRC_MISMATCH);
@@ skipping 491 matching lines @@
   // the actual size of stream 0.
   int total_data_size = GetDataSizeFromFileSize(key_.size(), file_size);
   out_entry_stat->set_data_size(0, 0);
   out_entry_stat->set_data_size(
       1,
       total_data_size - sizeof(net::SHA256HashValue) - sizeof(SimpleFileEOF));
 
   bool has_crc32;
   bool has_key_sha256;
   uint32_t read_crc32;
-  int stream_0_size;
+  int32_t stream_0_size;
   int ret_value_crc32 =
       GetEOFRecordData(0, *out_entry_stat, &has_crc32, &has_key_sha256,
                        &read_crc32, &stream_0_size);
   if (ret_value_crc32 != net::OK)
     return ret_value_crc32;
+
   // Calculate and set the real values for data size.
-  int stream_1_size = out_entry_stat->data_size(1) - stream_0_size;
+  int32_t stream_1_size = out_entry_stat->data_size(1);
   if (!has_key_sha256)
     stream_1_size += sizeof(net::SHA256HashValue);
-  if (stream_1_size < 0)
+  if (stream_0_size > stream_1_size)

[Julia Tuttle, 2016/06/27 20:39:10]

This conditional sounds nonsensical. It makes sense in the context of
"stream_1_size is actually the length of the whole thing and I'm about to
subtract stream_0_size from it", but it'd make more sense if you used another
name for stream_1_size and then did "int32_t stream_1_size = total_size -
stream_0_size" or something like that.

[gavinp, 2016/07/26 17:32:34]

Done.

     return net::ERR_FAILED;
+  stream_1_size -= stream_0_size;
   out_entry_stat->set_data_size(0, stream_0_size);
   out_entry_stat->set_data_size(1, stream_1_size);
 
   // Put stream 0 data in memory.
   *stream_0_data = new net::GrowableIOBuffer();
   (*stream_0_data)->SetCapacity(stream_0_size + sizeof(net::SHA256HashValue));
   int file_offset = out_entry_stat->GetOffsetInFile(key_.size(), 0, 0);
   int read_size = stream_0_size;
   if (has_key_sha256)
     read_size += sizeof(net::SHA256HashValue);
@@ skipping 55 matching lines @@
     RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_READ_FAILURE);
     return net::ERR_CACHE_CHECKSUM_READ_FAILURE;
   }
 
   if (eof_record.final_magic_number != kSimpleFinalMagicNumber) {
     RecordCheckEOFResult(cache_type_, CHECK_EOF_RESULT_MAGIC_NUMBER_MISMATCH);
     DVLOG(1) << "EOF record had bad magic number.";
     return net::ERR_CACHE_CHECKSUM_READ_FAILURE;
   }
 
+  if (!base::IsValueInRangeForNumericType<int>(eof_record.stream_size))

[Julia Tuttle, 2016/06/27 20:39:10]

int, or int32_t? (Also, do you want to check for negative here?)

[gavinp, 2016/07/26 17:32:34]

Great catch. Fixed the signature. Thanks.

Don't need to check for negative. eof_record.steam_size is an unsigned, so it's
UNPOSSIBLE.

+    return net::ERR_FAILED;
+
   *out_has_crc32 = (eof_record.flags & SimpleFileEOF::FLAG_HAS_CRC32) ==
                    SimpleFileEOF::FLAG_HAS_CRC32;
   *out_has_key_sha256 =
       (eof_record.flags & SimpleFileEOF::FLAG_HAS_KEY_SHA256) ==
       SimpleFileEOF::FLAG_HAS_KEY_SHA256;
   *out_crc32 = eof_record.data_crc32;
   *out_data_size = eof_record.stream_size;
   SIMPLE_CACHE_UMA(BOOLEAN, "SyncCheckEOFHasCrc", cache_type_, *out_has_crc32);
   return net::OK;
 }
@@ skipping 312 matching lines @@
   range.offset = offset;
   range.length = len;
   range.data_crc32 = data_crc32;
   range.file_offset = data_file_offset;
   sparse_ranges_.insert(std::make_pair(offset, range));
 
   return true;
 }
 
 }  // namespace disk_cache