New issue
Advanced search Search tips

Issue 781766 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug-Security



Sign in to add a comment

Crash in media::SourceBufferRangeByPts::GetBufferIndexAt

Project Member Reported by ClusterFuzz, Nov 6 2017

Issue description

Detailed report: https://clusterfuzz.com/testcase?key=6050897984749568

Fuzzer: libFuzzer_mediasource_MP4_AACLC_AVC_pipeline_integration_fuzzer
Job Type: libfuzzer_chrome_ubsan
Platform Id: linux

Crash Type: UNKNOWN READ
Crash Address: 0x000f6b84d164
Crash State:
  media::SourceBufferRangeByPts::GetBufferIndexAt
  media::SourceBufferRangeByPts::TruncateAt
  media::SourceBufferStream<media::SourceBufferRangeByPts>::RemoveInternal
  
Sanitizer: undefined (UBSAN)

Recommended Security Severity: Medium

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=508791:508824

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6050897984749568

Issue filed automatically.

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md for more information.
 
Project Member

Comment 1 by sheriffbot@chromium.org, Nov 6 2017

Labels: M-64
Project Member

Comment 2 by sheriffbot@chromium.org, Nov 6 2017

Labels: ReleaseBlock-Stable
This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 3 by sheriffbot@chromium.org, Nov 6 2017

Labels: Pri-1

Comment 4 by vakh@chromium.org, Nov 6 2017

Components: Internals>Media>Source
Owner: wolenetz@chromium.org
Status: Assigned (was: Untriaged)
Hmm. I was unable to get this to repro locally using old reference instructions (https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md). However, I suspect the underlying root cause might be the same as  bug 771482 . Will not mark this as duplicate since no successful local repro yet.
@#5, it would help if I had attempted repro not master branch, not on my WIP fix-for-bug-771482-branch. I have successful repro now locally on master ToT branch.
Labels: -M-64 M-63
Like  bug 771482  (suspected duplicate), this needs fixing before M-63 goes stable.
Labels: -M-63 M-64
Labels: -M-64 M-63
Project Member

Comment 10 by sheriffbot@chromium.org, Nov 8 2017

Labels: -Security_Impact-Head Security_Impact-Beta
Labels: -ReleaseBlock-Stable -M-63 M-64
per mail with wolenetz@, this code won't be enabled by default or Finch before 64, so no need to merge to 63. Removing release blocker label.
Project Member

Comment 12 by sheriffbot@chromium.org, Nov 10 2017

Labels: ReleaseBlock-Stable
This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 13 by bugdroid1@chromium.org, Nov 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/469d91812fee7b2db7fd55d55bcb7663784c507d

commit 469d91812fee7b2db7fd55d55bcb7663784c507d
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Tue Nov 14 03:28:27 2017

MSE: If timestampOffset change jumps far enough forward, introduce gap

When buffering by either PTS or DTS, and in segments AppendMode, if the
web app sets timestampOffset such that the next coded frame for a track
emitted from the coded frame processing algorithm is far enough in the
future, a buffered range gap can be introduced.

This change makes similar behavior occur when in sequence AppendMode.
Prior to this change, the intent of sequence mode was assumed to be to
not introduce such gaps when jumping forwards due to a newly set
timestampOffset. However, such behavior is neither explicit nor required
in the MSE spec, and implementing it, especially when buffering
compliantly ByPts, is complex and unintuitive. For example, should
previously buffered frames in the "gap" be removed? Worse, the previous
implementation used an implicit expansion of our buffering adjacency
"fudge room" to coalesce such a gap in sequence mode when buffering by
DTS, using inter-frame DTS deltas.

The net of this change also fixes one source of SBRByPts range adjacency
CHECK failures. Subsequent changes will address other sources of
related failures in the referenced bugs, below.

BUG= 771482 ,  771480 ,  771473 ,  771481 ,  771539 ,  781766 ,  775967 
TEST=Updated FrameProcessorTests and added LargeTimestampOffsetJumpForward

Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: Iecc30c6ef6bf5c3f7433152cc62793806ecbd653
Reviewed-on: https://chromium-review.googlesource.com/757756
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516169}
[modify] https://crrev.com/469d91812fee7b2db7fd55d55bcb7663784c507d/media/filters/frame_processor.cc
[modify] https://crrev.com/469d91812fee7b2db7fd55d55bcb7663784c507d/media/filters/frame_processor.h
[modify] https://crrev.com/469d91812fee7b2db7fd55d55bcb7663784c507d/media/filters/frame_processor_unittest.cc

Project Member

Comment 14 by bugdroid1@chromium.org, Nov 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d5f07c6849faa9a88e6358eb001de5a75443d791

commit d5f07c6849faa9a88e6358eb001de5a75443d791
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Wed Nov 15 20:43:05 2017

MSE: When buffering ByPts, handle keyframe with PTS < previous keyframe

If FrameProcessor doesn't signal SourceBufferStream of a new coded frame
group when buffering ByPts and an otherwise continuous (in DTS) frame
sequence has keyframe PTS that does not increase, then code faults are
hit. Specifically,

A) the underlying SourceBufferRangeByPts could have its keyframe map
   become out of sequence with respect to the buffers it indexes,
   violating assumptions of its ordering, and

B) internal adjacency checks for the new frame with the previous append
   can fail.

This change adds such signalling to FrameProcessor to avoid these faults
when keyframe PTS sequence decreases in a CFG when buffering ByPts.
Note that "deep" SAP-Type-2 sequences can still trigger (B), tracked by
 bug 773115 .

TEST=New test cases' ProcessFrames calls triggered A or B previously:
   SegmentsModeNewByPts/FrameProcessorTest.OOOKeyframePts_1/0 (Hit A)
   SequenceModeNewByPts/FrameProcessorTest.OOOKeyframePts_1/0 (Hit A)*
   SegmentsModeNewByPts/FrameProcessorTest.OOOKeyframePts_2/0 (Hit B)
   SequenceModeNewByPts/FrameProcessorTest.OOOKeyframePts_2/0 (Hit B)
   * - this test also triggered (B) prior to landing prereq CL 469d9181

BUG= 771482 ,  771480 ,  771473 ,  771481 ,  771539 ,  781766 ,  775967 ,  773115 

Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I80902f3ce1c0a9262459df0a1092d6bb5cbc400f
Reviewed-on: https://chromium-review.googlesource.com/728860
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516814}
[modify] https://crrev.com/d5f07c6849faa9a88e6358eb001de5a75443d791/media/filters/frame_processor.cc
[modify] https://crrev.com/d5f07c6849faa9a88e6358eb001de5a75443d791/media/filters/frame_processor_unittest.cc
[modify] https://crrev.com/d5f07c6849faa9a88e6358eb001de5a75443d791/media/filters/source_buffer_range.cc

Project Member

Comment 15 by ClusterFuzz, Nov 16 2017

ClusterFuzz has detected this issue as fixed in range 516797:516837.

Detailed report: https://clusterfuzz.com/testcase?key=6050897984749568

Fuzzer: libFuzzer_mediasource_MP4_AACLC_AVC_pipeline_integration_fuzzer
Job Type: libfuzzer_chrome_ubsan
Platform Id: linux

Crash Type: UNKNOWN READ
Crash Address: 0x000f6b84d164
Crash State:
  media::SourceBufferRangeByPts::GetBufferIndexAt
  media::SourceBufferRangeByPts::TruncateAt
  media::SourceBufferStream<media::SourceBufferRangeByPts>::RemoveInternal
  
Sanitizer: undefined (UBSAN)

Recommended Security Severity: Medium

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=508791:508824
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=516797:516837

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6050897984749568

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
Project Member

Comment 16 by ClusterFuzz, Nov 16 2017

Labels: ClusterFuzz-Verified
Status: Verified (was: Assigned)
ClusterFuzz testcase 6050897984749568 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Project Member

Comment 17 by sheriffbot@chromium.org, Nov 16 2017

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member

Comment 18 by sheriffbot@chromium.org, Feb 22 2018

Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 19 by sheriffbot@chromium.org, Mar 27 2018

Labels: -Security_Impact-Beta -M-64 M-65 Security_Impact-Stable
Labels: -ReleaseBlock-Stable

Sign in to add a comment