MSE: Fix moar mp4 parsing security bugs.

MSE: Fix moar mp4 parsing security bugs.

Boxes with various sub-entries read the entry count from the user
provided mp4. Do not trust the counts. Check for size_t and vector
resize() overflow to avoid OOB writes in vector allocation.

Additionally, verify we have enough bytes to continue parsing before
allocating vectors to store parsed data.

Also evaluated other box_definition.cc vector resize() calls. Added
one additional check for SampleEncryptionEntry (probably overkill).

BUG=679645, 679646, 679647, 679653
TEST=Verified POCs no longer crash. New unit tests.

Review-Url: https://codereview.chromium.org/2643123002
Cr-Commit-Position: refs/heads/master@{#444935}
Committed: https://chromium.googlesource.com/chromium/src/+/d5e2e152b550e4fbfff9b08e7bdf7c9d4c937438

[chcunningham, 2017-01-19 22:49:59]

chcunningham@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[chcunningham, 2017-01-19 22:49:59]

Hey Dale, fixed the last batch. Refactored the unit tests.

https://codereview.chromium.org/2643123002/diff/1/media/formats/mp4/box_reade...
File media/formats/mp4/box_reader_unittest.cc (left):

https://codereview.chromium.org/2643123002/diff/1/media/formats/mp4/box_reade...
media/formats/mp4/box_reader_unittest.cc:279: // (does not cause an int32_t
overflow when checking that the bytes are
This actually was causing a 32 bit overflow (now tested separately). Changed the
count to just 0x0a to preserve testing the original path.

[chcunningham, 2017-01-19 22:50:13]

The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 22:51:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

lgtm, nice tests!

https://codereview.chromium.org/2643123002/diff/1/media/formats/mp4/box_defin...
File media/formats/mp4/box_definitions.cc (right):

https://codereview.chromium.org/2643123002/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:486: int bytes_per_edit = reader->version()
== 1 ? 20 : 12;
const size_t? since that's the type you're multiplying in?

https://codereview.chromium.org/2643123002/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:1141: const int bytes_per_field = 4;
Ditto.

https://codereview.chromium.org/2643123002/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:1211: const int bytes_per_entry = 8;
Ditto.

https://codereview.chromium.org/2643123002/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:1384: size_t sample_count =
reader->box_size() - reader->pos();
Presumably pos() can never be negative or > box_size() ?

https://codereview.chromium.org/2643123002/diff/1/media/formats/mp4/box_reade...
File media/formats/mp4/box_reader_unittest.cc (right):

https://codereview.chromium.org/2643123002/diff/1/media/formats/mp4/box_reade...
media/formats/mp4/box_reader_unittest.cc:445: LOG(ERROR) << __func__ << "size:"
<< sizeof(SampleDependsOn);
Drop?

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

Thanks!

https://codereview.chromium.org/2643123002/diff/1/media/formats/mp4/box_defin...
File media/formats/mp4/box_definitions.cc (right):

https://codereview.chromium.org/2643123002/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:486: int bytes_per_edit = reader->version()
== 1 ? 20 : 12;
On 2017/01/19 22:59:29, DaleCurtis wrote:
> const size_t? since that's the type you're multiplying in?

Done.

https://codereview.chromium.org/2643123002/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:1141: const int bytes_per_field = 4;
On 2017/01/19 22:59:30, DaleCurtis wrote:
> Ditto.

Done.

https://codereview.chromium.org/2643123002/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:1211: const int bytes_per_entry = 8;
On 2017/01/19 22:59:30, DaleCurtis wrote:
> Ditto.

Done.

https://codereview.chromium.org/2643123002/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:1384: size_t sample_count =
reader->box_size() - reader->pos();
On 2017/01/19 22:59:29, DaleCurtis wrote:
> Presumably pos() can never be negative or > box_size() ? 

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.

https://codereview.chromium.org/2643123002/diff/1/media/formats/mp4/box_reade...
File media/formats/mp4/box_reader_unittest.cc (right):

https://codereview.chromium.org/2643123002/diff/1/media/formats/mp4/box_reade...
media/formats/mp4/box_reader_unittest.cc:445: LOG(ERROR) << __func__ << "size:"
<< sizeof(SampleDependsOn);
On 2017/01/19 22:59:30, DaleCurtis wrote:
> Drop?

Done.

[chcunningham, 2017-01-19 23:55:40]

The CQ bit was checked by chcunningham@chromium.org

[chcunningham, 2017-01-19 23:55:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/2643123002/#ps20001
(title: "Feedback")

[commit-bot: I haz the power, 2017-01-19 23:56:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 01:48:30]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484870140675720,
"parent_rev": "fb0830575b5d15718194f802d48c80e6450eddde", "commit_rev":
"d5e2e152b550e4fbfff9b08e7bdf7c9d4c937438"}

[commit-bot: I haz the power, 2017-01-20 01:49:02]

Description was changed from

==========
MSE: Fix moar mp4 parsing security bugs.

Boxes with various sub-entries read the entry count from the user
provided mp4. Do not trust the counts. Check for size_t and vector
resize() overflow to avoid OOB writes in vector allocation.

Additionally, verify we have enough bytes to continue parsing before
allocating vectors to store parsed data.

Also evaluated other box_definition.cc vector resize() calls. Added
one additional check for SampleEncryptionEntry (probably overkill).

BUG=679645,679646,679647,679653
TEST=Verified POCs no longer crash. New unit tests.
==========

to

==========
MSE: Fix moar mp4 parsing security bugs.

Boxes with various sub-entries read the entry count from the user
provided mp4. Do not trust the counts. Check for size_t and vector
resize() overflow to avoid OOB writes in vector allocation.

Additionally, verify we have enough bytes to continue parsing before
allocating vectors to store parsed data.

Also evaluated other box_definition.cc vector resize() calls. Added
one additional check for SampleEncryptionEntry (probably overkill).

BUG=679645,679646,679647,679653
TEST=Verified POCs no longer crash. New unit tests.

Review-Url: https://codereview.chromium.org/2643123002
Cr-Commit-Position: refs/heads/master@{#444935}
Committed:
https://chromium.googlesource.com/chromium/src/+/d5e2e152b550e4fbfff9b08e7bdf...
==========

[commit-bot: I haz the power, 2017-01-20 01:49:03]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/d5e2e152b550e4fbfff9b08e7bdf...