[media] BoxReader: Cap |buf_size_| to |box_size_|.

Before this change, BoxReader could read syntax elements outside of a
box in cases where the MP4 was invalid. This isn't a security concern,
since the reads were still bounded by the buffer size, but it did make
it hard to interpret the later failures.

This CL shrinks |buf_size_| to match |box_size_| when the box size is
read. OOB reads will now result in Parse failures immediately.

Bug: 779321
Change-Id: I54be6ba3815d1502233b5e95d099ce9d9a54b3cc
Reviewed-on: https://chromium-review.googlesource.com/747080Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Commit-Queue: Dan Sanders <sandersd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513000}

media/formats/mp4/box_reader.cc

@@ -192,6 +192,7 @@ bool BoxReader::ScanChildren() {
   DCHECK(!scanned_);
   scanned_ = true;
 
+  DCHECK_LE(pos_, box_size_);
   while (pos_ < box_size_) {
     BoxReader child(&buf_[pos_], box_size_ - pos_, media_log_, is_EOS_);
     if (child.ReadHeader() != ParseResult::kOk)
@@ -199,8 +200,7 @@ bool BoxReader::ScanChildren() {
     children_.insert(std::pair<FourCC, BoxReader>(child.type(), child));
     pos_ += child.box_size();
   }
-
-  DCHECK(pos_ == box_size_);
+  DCHECK_EQ(pos_, box_size_);
   return true;
 }
 
@@ -278,6 +278,10 @@ ParseResult BoxReader::ReadHeader() {
   // header, which is where we want it.
   box_size_ = base::checked_cast<size_t>(box_size);
   box_size_known_ = true;
+
+  // We don't want future reads to go beyond the box.
+  buf_size_ = std::min(buf_size_, box_size_);
+
   return ParseResult::kOk;
 }
 

media/formats/mp4/box_reader.h

@@ -45,10 +45,9 @@ class MEDIA_EXPORT BufferReader {
   bool HasBytes(size_t count) {
     // As the size of a box is implementation limited to 2^31, fail if
     // attempting to check for too many bytes.
-    return (pos_ <= buf_size_ &&
-            count <
-                static_cast<uint64_t>(std::numeric_limits<int32_t>::max()) &&
-            buf_size_ - pos_ >= count);
+    const size_t impl_limit = 1 << 31;
+    return pos_ <= buf_size_ && count <= impl_limit &&
+           count <= buf_size_ - pos_;
   }
 
   // Read a value from the stream, perfoming endian correction, and advance the
@@ -84,7 +83,7 @@ class MEDIA_EXPORT BufferReader {
 
  protected:
   const uint8_t* buf_;
-  const size_t buf_size_;
+  size_t buf_size_;
   size_t pos_;
 
   template<typename T> bool Read(T* t) WARN_UNUSED_RESULT;
@@ -268,6 +267,7 @@ bool BoxReader::ReadAllChildrenInternal(std::vector<T>* children,
   // Must know our box size before attempting to parse child boxes.
   RCHECK(box_size_known_);
 
+  DCHECK_LE(pos_, box_size_);
   while (pos_ < box_size_) {
     BoxReader child_reader(&buf_[pos_], box_size_ - pos_, media_log_, is_EOS_);
 
@@ -280,7 +280,7 @@ bool BoxReader::ReadAllChildrenInternal(std::vector<T>* children,
     children->push_back(child);
     pos_ += child_reader.box_size();
   }
-
+  DCHECK_EQ(pos_, box_size_);
   return true;
 }
 

media/formats/mp4/box_reader_unittest.cc

@@ -464,5 +464,24 @@ TEST_F(BoxReaderTest, SgpdCount32bitOverflow) {
       kData, sizeof(kData), "Extreme SGPD count exceeds implementation limit.");
 }
 
+TEST_F(BoxReaderTest, OutsideOfBoxRead) {
+  static const uint8_t kData[] = {
+      0x00, 0x00, 0x00, 0x0c, 'f', 'r', 'e', 'e',  // header
+      0x01, 0x02, 0x03, 0x04,                      // box contents
+      0x05, 0x06, 0x07, 0x08,                      // buffer padding
+  };
+
+  std::unique_ptr<BoxReader> reader;
+  ParseResult result =
+      BoxReader::ReadTopLevelBox(kData, sizeof(kData), &media_log_, &reader);
+  EXPECT_EQ(result, ParseResult::kOk);
+  EXPECT_TRUE(reader);
+
+  uint32_t value;
+  EXPECT_TRUE(reader->Read4(&value));
+  EXPECT_EQ(value, 0x01020304u);
+  EXPECT_FALSE(reader->Read4(&value));
+}
+
 }  // namespace mp4
 }  // namespace media