Guard against overflow in BigEndianReader and BigEndianWriter.

wav_audio_handler_fuzzer found a fun bug in BigEndianReader. By calling
BigEndianReader::ReadPiece with a very large length, we can bypass the
internal safety check.

BigEndianReader::ReadPiece will abort the operation and return false if
|ptr_ + len > end_|. When |len| is a large value, |ptr_ + len| may
overflow, spuriously passing the bounds check.

All of the BigEndianReader and BigEndianWriter functions that take
length values use this vulnerable bounds check.

This CL replaces the vulnerable pointer arithmetic with a safe
alternative, and adds unit tests to detect regressions. After this CL,
it should be safe to hand untrusted lengths to BigEndianReader and
BigEndianWriter functions.

Bug: 1114803
Change-Id: Ibf1192298dfe37a1aebcf6afaad080d1b5765135
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2347368
Commit-Queue: Dan McArdle <dmcardle@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#796614}

base/big_endian.cc

@@ -12,17 +12,19 @@
 namespace base {
 
 BigEndianReader::BigEndianReader(const char* buf, size_t len)
-    : ptr_(buf), end_(ptr_ + len) {}
+    : ptr_(buf), end_(ptr_ + len) {
+  CHECK_LE(ptr_, end_);
+}
 
 bool BigEndianReader::Skip(size_t len) {
-  if (ptr_ + len > end_)
+  if (len > remaining())
     return false;
   ptr_ += len;
   return true;
 }
 
 bool BigEndianReader::ReadBytes(void* out, size_t len) {
-  if (ptr_ + len > end_)
+  if (len > remaining())
     return false;
   memcpy(out, ptr_, len);
   ptr_ += len;
@@ -30,7 +32,7 @@ bool BigEndianReader::ReadBytes(void* out, size_t len) {
 }
 
 bool BigEndianReader::ReadPiece(base::StringPiece* out, size_t len) {
-  if (ptr_ + len > end_)
+  if (len > remaining())
     return false;
   *out = base::StringPiece(ptr_, len);
   ptr_ += len;
@@ -39,7 +41,7 @@ bool BigEndianReader::ReadPiece(base::StringPiece* out, size_t len) {
 
 template<typename T>
 bool BigEndianReader::Read(T* value) {
-  if (ptr_ + sizeof(T) > end_)
+  if (sizeof(T) > remaining())
     return false;
   ReadBigEndian<T>(ptr_, value);
   ptr_ += sizeof(T);
@@ -86,17 +88,19 @@ bool BigEndianReader::ReadU16LengthPrefixed(base::StringPiece* out) {
 }
 
 BigEndianWriter::BigEndianWriter(char* buf, size_t len)
-    : ptr_(buf), end_(ptr_ + len) {}
+    : ptr_(buf), end_(ptr_ + len) {
+  CHECK_LE(ptr_, end_);
+}
 
 bool BigEndianWriter::Skip(size_t len) {
-  if (ptr_ + len > end_)
+  if (len > remaining())
     return false;
   ptr_ += len;
   return true;
 }
 
 bool BigEndianWriter::WriteBytes(const void* buf, size_t len) {
-  if (ptr_ + len > end_)
+  if (len > remaining())
     return false;
   memcpy(ptr_, buf, len);
   ptr_ += len;
@@ -105,7 +109,7 @@ bool BigEndianWriter::WriteBytes(const void* buf, size_t len) {
 
 template<typename T>
 bool BigEndianWriter::Write(T value) {
-  if (ptr_ + sizeof(T) > end_)
+  if (sizeof(T) > remaining())
     return false;
   WriteBigEndian<T>(ptr_, value);
   ptr_ += sizeof(T);

base/big_endian_unittest.cc

@@ -6,6 +6,8 @@
 
 #include <stdint.h>
 
+#include <limits>
+
 #include "base/strings/string_piece.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -128,6 +130,20 @@ TEST(BigEndianReaderTest, RespectsLength) {
   EXPECT_EQ(0u, reader.remaining());
 }
 
+TEST(BigEndianReaderTest, SafePointerMath) {
+  char data[] = "foo";
+  BigEndianReader reader(data, sizeof(data));
+  // The test should fail without ever dereferencing the |dummy_buf| pointer.
+  char* dummy_buf = reinterpret_cast<char*>(0xdeadbeef);
+  // Craft an extreme length value that would cause |reader.data() + len| to
+  // overflow.
+  size_t extreme_length = std::numeric_limits<size_t>::max() - 1;
+  base::StringPiece piece;
+  EXPECT_FALSE(reader.Skip(extreme_length));
+  EXPECT_FALSE(reader.ReadBytes(dummy_buf, extreme_length));
+  EXPECT_FALSE(reader.ReadPiece(&piece, extreme_length));
+}
+
 TEST(BigEndianWriterTest, WritesValues) {
   char expected[] = { 0, 0, 2, 3, 4, 5, 6, 7, 8, 9, 0xA, 0xB, 0xC, 0xD, 0xE,
                       0xF, 0x1A, 0x2B, 0x3C };
@@ -171,4 +187,16 @@ TEST(BigEndianWriterTest, RespectsLength) {
   EXPECT_EQ(0u, writer.remaining());
 }
 
+TEST(BigEndianWriterTest, SafePointerMath) {
+  char data[3];
+  BigEndianWriter writer(data, sizeof(data));
+  // The test should fail without ever dereferencing the |dummy_buf| pointer.
+  const char* dummy_buf = reinterpret_cast<const char*>(0xdeadbeef);
+  // Craft an extreme length value that would cause |reader.data() + len| to
+  // overflow.
+  size_t extreme_length = std::numeric_limits<size_t>::max() - 1;
+  EXPECT_FALSE(writer.Skip(extreme_length));
+  EXPECT_FALSE(writer.WriteBytes(dummy_buf, extreme_length));
+}
+
 }  // namespace base