Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(872)

Issue 2643573003: MSE: Fix Mp4 TRUN parsing overflow (Closed)

Created:
3 years, 11 months ago by chcunningham
Modified:
3 years, 11 months ago
Reviewers:
DaleCurtis
CC:
chromium-reviews, feature-media-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

MSE: Fix Mp4 TRUN parsing overflow TrackFragmentRun::Parse sample_count can take any value between 0x0 and 0xffffffff. We must check for size_t overflow when multiplying sample_count by "fields". We should also avoid attempting to resize vectors beyond their max_size() (potential OOB depending on stl library impl). BUG=679640 , TEST=unit test, manual verification of POC. Review-Url: https://codereview.chromium.org/2643573003 Cr-Commit-Position: refs/heads/master@{#444524} Committed: https://chromium.googlesource.com/chromium/src/+/24f5635bb25006c6ac263c47e64c8b1cfa0b0f7a

Patch Set 1 #

Patch Set 2 : Proper unit test for TRUN sample_count overflow #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+65 lines, -31 lines) Patch
M media/formats/mp4/box_definitions.cc View 1 2 chunks +21 lines, -5 lines 3 comments Download
M media/formats/mp4/box_reader_unittest.cc View 1 3 chunks +44 lines, -26 lines 1 comment Download

Dependent Patchsets:

Messages

Total messages: 20 (9 generated)
chcunningham
Hey, here is the first of six such fixes. Planning on dividing reviews since there's ...
3 years, 11 months ago (2017-01-18 21:13:32 UTC) #4
DaleCurtis
lgtm % question https://codereview.chromium.org/2643573003/diff/20001/media/formats/mp4/box_definitions.cc File media/formats/mp4/box_definitions.cc (right): https://codereview.chromium.org/2643573003/diff/20001/media/formats/mp4/box_definitions.cc#newcode1133 media/formats/mp4/box_definitions.cc:1133: RCHECK(reader->HasBytes(bytes_needed.ValueOrDie())); Isn't this unreachable if IsValid() ...
3 years, 11 months ago (2017-01-18 21:21:32 UTC) #6
DaleCurtis
Did you scan media/formats for other instances of .resize() on untrusted data?
3 years, 11 months ago (2017-01-18 21:21:55 UTC) #7
chcunningham
Thanks! On 2017/01/18 21:21:55, DaleCurtis wrote: > Did you scan media/formats for other instances of ...
3 years, 11 months ago (2017-01-18 21:52:06 UTC) #8
DaleCurtis
https://codereview.chromium.org/2643573003/diff/20001/media/formats/mp4/box_definitions.cc File media/formats/mp4/box_definitions.cc (right): https://codereview.chromium.org/2643573003/diff/20001/media/formats/mp4/box_definitions.cc#newcode1133 media/formats/mp4/box_definitions.cc:1133: RCHECK(reader->HasBytes(bytes_needed.ValueOrDie())); On 2017/01/18 at 21:52:06, chcunningham wrote: > On ...
3 years, 11 months ago (2017-01-18 21:53:05 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2643573003/20001
3 years, 11 months ago (2017-01-18 21:55:27 UTC) #13
commit-bot: I haz the power
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/24f5635bb25006c6ac263c47e64c8b1cfa0b0f7a
3 years, 11 months ago (2017-01-18 22:54:24 UTC) #16
chcunningham
I may have spotted another bug. When calling HasBytes for the TrackFragmentRun, we originally did ...
3 years, 11 months ago (2017-01-19 02:18:41 UTC) #17
chcunningham
On 2017/01/19 02:18:41, chcunningham wrote: > I may have spotted another bug. > > When ...
3 years, 11 months ago (2017-01-19 19:15:27 UTC) #18
DaleCurtis
Yeah in general I thought BitReader was designed such that we could omit HasBytes() type ...
3 years, 11 months ago (2017-01-19 19:18:51 UTC) #19
chcunningham
3 years, 11 months ago (2017-01-19 19:31:01 UTC) #20
Message was sent while issue was closed.
On 2017/01/19 19:18:51, DaleCurtis wrote:
> Yeah in general I thought BitReader was designed such that we could omit
> HasBytes() type checks since it will fail during the normal Read process.

Agree. Perhaps they're still nice to have as they help us bail just before
allocating a bunch of vectors that may never be used.

Powered by Google App Engine
This is Rietveld 408576698