|
|
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. |
DescriptionMSE: 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
Dependent Patchsets: Messages
Total messages: 20 (9 generated)
The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
chcunningham@chromium.org changed reviewers: + dalecurtis@chromium.org
Hey, here is the first of six such fixes. Planning on dividing reviews since there's a fair bit of nuance here. https://codereview.chromium.org/2643573003/diff/20001/media/formats/mp4/box_r... File media/formats/mp4/box_reader_unittest.cc (left): https://codereview.chromium.org/2643573003/diff/20001/media/formats/mp4/box_r... media/formats/mp4/box_reader_unittest.cc:304: TEST_F(BoxReaderTest, ReadAllChildrenWithInvalidChild) { This test was busted. On 32-bit, we overflow when computing the needed number of bytes to parse all the samples. The catch is, the overflow wrapped all the way around several times, ending up close to int32 max and way exceeding the number of bytes actually available in kData. Parsing would then fail when the box reader indicated it did not have the needed (incorrect) number of bytes. This meant the test passed in the same way on both 32 and 64 bit, covering up the 32bit overflow.
Description was changed from ========== MSE: Fix Mp4 parsing security bugs. 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 tests, manual verification of POCs. ========== to ========== 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. ==========
lgtm % question https://codereview.chromium.org/2643573003/diff/20001/media/formats/mp4/box_d... File media/formats/mp4/box_definitions.cc (right): https://codereview.chromium.org/2643573003/diff/20001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.cc:1133: RCHECK(reader->HasBytes(bytes_needed.ValueOrDie())); Isn't this unreachable if IsValid() fails above?
Did you scan media/formats for other instances of .resize() on untrusted data?
Thanks! On 2017/01/18 21:21:55, DaleCurtis wrote: > Did you scan media/formats for other instances of .resize() on untrusted data? Not yet. Will do that once I've patched up the 6 reported cases. https://codereview.chromium.org/2643573003/diff/20001/media/formats/mp4/box_d... File media/formats/mp4/box_definitions.cc (right): https://codereview.chromium.org/2643573003/diff/20001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.cc:1133: RCHECK(reader->HasBytes(bytes_needed.ValueOrDie())); On 2017/01/18 21:21:32, DaleCurtis wrote: > Isn't this unreachable if IsValid() fails above? Yes. Its only reachable when IsValid passes, in which case we still want to check HasBytes. The "Die" part of ValueOrDie() is not reachable, but I think its better than ValueOrDefault() (only two options AFAIK).
https://codereview.chromium.org/2643573003/diff/20001/media/formats/mp4/box_d... File media/formats/mp4/box_definitions.cc (right): https://codereview.chromium.org/2643573003/diff/20001/media/formats/mp4/box_d... media/formats/mp4/box_definitions.cc:1133: RCHECK(reader->HasBytes(bytes_needed.ValueOrDie())); On 2017/01/18 at 21:52:06, chcunningham wrote: > On 2017/01/18 21:21:32, DaleCurtis wrote: > > Isn't this unreachable if IsValid() fails above? > > Yes. > > Its only reachable when IsValid passes, in which case we still want to check HasBytes. > > The "Die" part of ValueOrDie() is not reachable, but I think its better than ValueOrDefault() (only two options AFAIK). Whoops, sorry I totally skipped over the function call when analyzing.
The CQ bit was checked by chcunningham@chromium.org
The CQ bit was unchecked by chcunningham@chromium.org
The CQ bit was checked by chcunningham@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484776486194970, "parent_rev": "96205c442a254bfedf737c97f836c7bfd1973d29", "commit_rev": "24f5635bb25006c6ac263c47e64c8b1cfa0b0f7a"}
Message was sent while issue was closed.
Description was changed from ========== 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. ========== to ========== 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/+/24f5635bb25006c6ac263c47e64c... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/24f5635bb25006c6ac263c47e64c...
Message was sent while issue was closed.
I may have spotted another bug. When calling HasBytes for the TrackFragmentRun, we originally did the following: ... int fields = sample_duration_present + sample_size_present + sample_flags_present + sample_composition_time_offsets_present; RCHECK(reader->HasBytes(fields * sample_count)); ... Even in the typical case where fields * sample_count does not overflow, I'm not sure this is the right number for HasBytes(). In the lines below, we iterate over each sample and read the present fields. For each field we read *4 bytes* - am I reading this right? if (sample_duration_present) RCHECK(reader->Read4(&sample_durations[i])); If so, the call to HasBytes() should really be HasBytes(fields * 4 * sample_count) (with needed overflow checking).
Message was sent while issue was closed.
On 2017/01/19 02:18:41, chcunningham wrote: > I may have spotted another bug. > > When calling HasBytes for the TrackFragmentRun, we originally did the following: > > ... > int fields = sample_duration_present + sample_size_present + > sample_flags_present + sample_composition_time_offsets_present; > > RCHECK(reader->HasBytes(fields * sample_count)); > ... > > Even in the typical case where fields * sample_count does not overflow, I'm not > sure this is the right number for HasBytes(). In the lines below, we iterate > over each sample and read the present fields. For each field we read *4 bytes* - > am I reading this right? > > if (sample_duration_present) > RCHECK(reader->Read4(&sample_durations[i])); > > If so, the call to HasBytes() should really be HasBytes(fields * 4 * > sample_count) (with needed overflow checking). Yeah - I just confirmed this is wrong. Its not a huge hole though. reader->ReadX will always internally check that X bytes are available, so it just fails a few lines down without going OOB. Will clean it up in a subsequent patch.
Message was sent while issue was closed.
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.
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. |