MSE: Fix moar mp4 parsing security bugs.

media/formats/mp4/box_definitions.cc

Index: media/formats/mp4/box_definitions.cc
diff --git a/media/formats/mp4/box_definitions.cc b/media/formats/mp4/box_definitions.cc
index f541ee633760d0e736a733ab470fbbb30f6a47f6..58e501628ba06ee73d1f1c7d903d4fdf2acd5103 100644
--- a/media/formats/mp4/box_definitions.cc
+++ b/media/formats/mp4/box_definitions.cc
@@ -193,6 +193,7 @@ bool SampleEncryptionEntry::Parse(BufferReader* reader,
   uint16_t subsample_count;
   RCHECK(reader->Read2(&subsample_count));
   RCHECK(subsample_count > 0);
+  RCHECK(subsample_count <= subsamples.max_size());
   subsamples.resize(subsample_count);
   for (SubsampleEntry& subsample : subsamples) {
     uint16_t clear_bytes;
@@ -482,11 +483,16 @@ bool EditList::Parse(BoxReader* reader) {
   uint32_t count;
   RCHECK(reader->ReadFullBoxHeader() && reader->Read4(&count));
 
-  if (reader->version() == 1) {
-    RCHECK(reader->HasBytes(count * 20));
-  } else {
-    RCHECK(reader->HasBytes(count * 12));
-  }
+  int bytes_per_edit = reader->version() == 1 ? 20 : 12;

[DaleCurtis, 2017/01/19 22:59:29]

const size_t? since that's the type you're multiplying in?

[chcunningham, 2017/01/19 23:55:31]

Done.

+
+  // Cast |count| to size_t before multiplying to support maximum platform size.
+  base::CheckedNumeric<size_t> bytes_needed =
+      base::CheckMul(bytes_per_edit, static_cast<size_t>(count));
+  RCHECK_MEDIA_LOGGED(bytes_needed.IsValid(), reader->media_log(),
+                      "Extreme ELST count exceeds implementation limit.");
+  RCHECK(reader->HasBytes(bytes_needed.ValueOrDie()));
+
+  RCHECK(count <= edits.max_size());
   edits.resize(count);
 
   for (std::vector<EditListEntry>::iterator edit = edits.begin();
@@ -1132,13 +1138,15 @@ bool TrackFragmentRun::Parse(BoxReader* reader) {
 
   int fields = sample_duration_present + sample_size_present +
       sample_flags_present + sample_composition_time_offsets_present;
+  const int bytes_per_field = 4;

[DaleCurtis, 2017/01/19 22:59:30]

Ditto.

[chcunningham, 2017/01/19 23:55:31]

Done.

 
   // Cast |sample_count| to size_t before multiplying to support maximum
   // platform size.
-  base::CheckedNumeric<size_t> bytes_needed =
-      base::CheckMul(fields, static_cast<size_t>(sample_count));
-  RCHECK_MEDIA_LOGGED(bytes_needed.IsValid(), reader->media_log(),
-                      "Extreme TRUN sample count exceeds system address space");
+  base::CheckedNumeric<size_t> bytes_needed = base::CheckMul(
+      fields, bytes_per_field, static_cast<size_t>(sample_count));
+  RCHECK_MEDIA_LOGGED(
+      bytes_needed.IsValid(), reader->media_log(),
+      "Extreme TRUN sample count exceeds implementation limit.");
   RCHECK(reader->HasBytes(bytes_needed.ValueOrDie()));
 
   if (sample_duration_present) {
@@ -1199,6 +1207,16 @@ bool SampleToGroup::Parse(BoxReader* reader) {
 
   uint32_t count;
   RCHECK(reader->Read4(&count));
+
+  const int bytes_per_entry = 8;

[DaleCurtis, 2017/01/19 22:59:30]

Ditto.

[chcunningham, 2017/01/19 23:55:31]

Done.

+  // Cast |count| to size_t before multiplying to support maximum platform size.
+  base::CheckedNumeric<size_t> bytes_needed =
+      base::CheckMul(bytes_per_entry, static_cast<size_t>(count));
+  RCHECK_MEDIA_LOGGED(bytes_needed.IsValid(), reader->media_log(),
+                      "Extreme SBGP count exceeds implementation limit.");
+  RCHECK(reader->HasBytes(bytes_needed.ValueOrDie()));
+
+  RCHECK(count <= entries.max_size());
   entries.resize(count);
   for (uint32_t i = 0; i < count; ++i) {
     RCHECK(reader->Read4(&entries[i].sample_count) &&
@@ -1279,6 +1297,19 @@ bool SampleGroupDescription::Parse(BoxReader* reader) {
 
   uint32_t count;
   RCHECK(reader->Read4(&count));
+
+  // Check that we have at least two bytes for each entry before allocating a
+  // potentially huge entries vector. In reality each entry will require a
+  // variable number of bytes in excess of 2.
+  const int bytes_per_entry = 2;
+  // Cast |count| to size_t before multiplying to support maximum platform size.
+  base::CheckedNumeric<size_t> bytes_needed =
+      base::CheckMul(bytes_per_entry, static_cast<size_t>(count));
+  RCHECK_MEDIA_LOGGED(bytes_needed.IsValid(), reader->media_log(),
+                      "Extreme SGPD count exceeds implementation limit.");
+  RCHECK(reader->HasBytes(bytes_needed.ValueOrDie()));
+
+  RCHECK(count <= entries.max_size());
   entries.resize(count);
   for (uint32_t i = 0; i < count; ++i) {
     if (version == 1) {
@@ -1350,9 +1381,10 @@ bool IndependentAndDisposableSamples::Parse(BoxReader* reader) {
   RCHECK(reader->version() == 0);
   RCHECK(reader->flags() == 0);
 
-  int sample_count = reader->box_size() - reader->pos();
+  size_t sample_count = reader->box_size() - reader->pos();

[DaleCurtis, 2017/01/19 22:59:29]

Presumably pos() can never be negative or > box_size() ? 

[chcunningham, 2017/01/19 23:55:31]

It can never be negative (type is size_t), and *should* never be > box_size().
It could be that some bug exists though which allows it to pass box_size(). But
I think the max_size() check I added would still save us from OOB troubles.

+  RCHECK(sample_count <= sample_depends_on_.max_size());
   sample_depends_on_.resize(sample_count);
-  for (int i = 0; i < sample_count; ++i) {
+  for (size_t i = 0; i < sample_count; ++i) {
     uint8_t sample_info;
     RCHECK(reader->Read1(&sample_info));