New issue
Advanced search Search tips

Issue 750009 link

Starred by 2 users

Issue metadata

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



Sign in to add a comment

Heap-buffer-overflow in mov_read_trun

Project Member Reported by ClusterFuzz, Jul 28 2017

Issue description

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

Fuzzer: inferno_flicker
Job Type: windows_asan_chrome
Platform Id: windows

Crash Type: Heap-buffer-overflow WRITE {*}
Crash Address: 0x082113f4
Crash State:
  mov_read_trun
  mov_read_default
  mov_read_default
  
Sanitizer: address (ASAN)

Recommended Security Severity: High

Regressed: https://clusterfuzz.com/revisions?job=windows_asan_chrome&range=488128:488146

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


Issue filed automatically.

See https://github.com/google/clusterfuzz-tools for more information.
 
Project Member

Comment 1 by sheriffbot@chromium.org, Jul 28 2017

Labels: M-61
Project Member

Comment 2 by sheriffbot@chromium.org, Jul 28 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, Jul 28 2017

Labels: Pri-1
Project Member

Comment 4 by sheriffbot@chromium.org, Jul 29 2017

Labels: -Security_Impact-Head Security_Impact-Beta

Comment 5 by vakh@chromium.org, Jul 31 2017

Cc: wolenetz@chromium.org
Components: Internals>Media>FFmpeg
Owner: dalecur...@chromium.org
Status: Assigned (was: Untriaged)
My best guess is that this is due to https://chromium-review.googlesource.com/578656
Will take a look today.
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 31 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/third_party/ffmpeg/+/bcaa2585874573d486d94101e883cd2e5dfbca97

commit bcaa2585874573d486d94101e883cd2e5dfbca97
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Mon Jul 31 21:51:39 2017

[mov] Bail when invalid sample data is present.

ctts data in ffmpeg relies on the index entries array to be 1:1
with samples... yet sc->sample_count can be read directly from
the 'stsz' box and index entries are only generated if a chunk
count has been read from 'stco' box.

Ensure that if sc->sample_count > 0, sc->chunk_count is too.

BUG= 750009 
TEST=asan test case passes. passes fate suite.

Change-Id: Iafe53ca360fe07e07744134c80036631d8d4e9ae
Reviewed-on: https://chromium-review.googlesource.com/594714
Reviewed-by: Matthew Wolenetz <wolenetz@chromium.org>

[modify] https://crrev.com/bcaa2585874573d486d94101e883cd2e5dfbca97/libavformat/mov.c
[modify] https://crrev.com/bcaa2585874573d486d94101e883cd2e5dfbca97/chromium/patches/README

Project Member

Comment 8 by bugdroid1@chromium.org, Aug 1 2017

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

commit b654d193a3155d1bd3a51810202f28d0546671ed
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Tue Aug 01 22:56:33 2017

Roll src/third_party/ffmpeg/ a53b8db56..bcaa25858 (1 commit)

https://chromium.googlesource.com/chromium/third_party/ffmpeg.git/+log/a53b8db56039..bcaa25858745

$ git log a53b8db56..bcaa25858 --date=short --no-merges --format='%ad %ae %s'
2017-07-31 dalecurtis [mov] Bail when invalid sample data is present.

Created with:
  roll-dep src/third_party/ffmpeg

Bug:  750009 
Test: asan test case now passes
Tbr: wolenetz
Change-Id: I565568cdabf61533b5a0527ab73e9d2c8ba3fdc2
Reviewed-on: https://chromium-review.googlesource.com/595167
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Matthew Wolenetz <wolenetz@chromium.org>
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491142}
[modify] https://crrev.com/b654d193a3155d1bd3a51810202f28d0546671ed/DEPS

Fix only solved a subset of the issue; when testing I used the pipeline integration test fuzzer for speed, but the full chrome html+mp4 test case still fails. So expanding the workaround in https://chromium-review.googlesource.com/c/604488
Project Member

Comment 10 by bugdroid1@chromium.org, Aug 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/third_party/ffmpeg/+/dbbdb1680bd8e77ea72f8e5a79a09101bd7a9bdd

commit dbbdb1680bd8e77ea72f8e5a79a09101bd7a9bdd
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Mon Aug 07 21:00:55 2017

Expand fixes for bailing on invalid ctts data.

Original patch only fixed one case when the test clip is used
with the pipeline integration test fuzzer. Running the full html+mp4
test case in an ASAN chrome reveals a stronger fix is needed.

This was submitted upstream with the original patch, but still has
not been reviewed upstream.

BUG= 750009 
TEST=chrome asan no longer fails on test clip.

Change-Id: I6596841cedab4c3caa58cbe60a7c951e49fb28e0
Reviewed-on: https://chromium-review.googlesource.com/604488
Reviewed-by: Matthew Wolenetz <wolenetz@chromium.org>

[modify] https://crrev.com/dbbdb1680bd8e77ea72f8e5a79a09101bd7a9bdd/libavformat/mov.c

Project Member

Comment 11 by bugdroid1@chromium.org, Aug 8 2017

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

commit 569fe5e0f749baab2af2ac079e61cd60e8759d27
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Tue Aug 08 01:06:55 2017

Roll src/third_party/ffmpeg/ bcaa25858..dbbdb1680 (1 commit)

https://chromium.googlesource.com/chromium/third_party/ffmpeg.git/+log/bcaa25858745..dbbdb1680bd8

$ git log bcaa25858..dbbdb1680 --date=short --no-merges --format='%ad %ae %s'
2017-08-07 dalecurtis Expand fixes for bailing on invalid ctts data.

Created with:
  roll-dep src/third_party/ffmpeg

Bug:  750009 
Test: full html+mp4 asan test case now passes
Change-Id: I9d5b9f524598f04b25a6efde056998108e688236
Tbr: wolenetz
Reviewed-on: https://chromium-review.googlesource.com/604470
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Matthew Wolenetz <wolenetz@chromium.org>
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492485}
[modify] https://crrev.com/569fe5e0f749baab2af2ac079e61cd60e8759d27/DEPS

Project Member

Comment 12 by ClusterFuzz, Aug 8 2017

ClusterFuzz has detected this issue as fixed in range 492361:492522.

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

Fuzzer: inferno_flicker
Job Type: windows_asan_chrome
Platform Id: windows

Crash Type: Heap-buffer-overflow WRITE {*}
Crash Address: 0x082113f4
Crash State:
  mov_read_trun
  mov_read_default
  mov_read_default
  
Sanitizer: address (ASAN)

Recommended Security Severity: High

Regressed: https://clusterfuzz.com/revisions?job=windows_asan_chrome&range=488128:488146
Fixed: https://clusterfuzz.com/revisions?job=windows_asan_chrome&range=492361:492522

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


See https://github.com/google/clusterfuzz-tools 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 13 by ClusterFuzz, Aug 8 2017

Labels: ClusterFuzz-Verified
Status: Verified (was: Assigned)
ClusterFuzz testcase 4715337248669696 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 14 by sheriffbot@chromium.org, Aug 8 2017

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

Comment 15 by sheriffbot@chromium.org, Aug 10 2017

Labels: Merge-Request-61
Project Member

Comment 16 by sheriffbot@chromium.org, Aug 10 2017

Labels: -Merge-Request-61 Merge-Review-61 Hotlist-Merge-Review
This bug requires manual review: DEPS changes referenced in bugdroid comments.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: awhalley@chromium.org
+awhalley@ for M61 merge review.
govind@ - good for 61
Labels: -Merge-Review-61 Merge-Approved-61
Approving merge to M61 branch 3163 based on comment #18.
Project Member

Comment 20 by bugdroid1@chromium.org, Aug 12 2017

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chrome/tools/buildspec/+/1ccd4a4e442b34ee9a3854b351aed623435c4221

commit 1ccd4a4e442b34ee9a3854b351aed623435c4221
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Sat Aug 12 00:20:40 2017

Seems like this is already merged to M61 at #20. Is there anything pending? 
If not pls remove "Merge-Approved-61" label and apply "merge-merged-3163" label pls. Thank you.
Project Member

Comment 22 by sheriffbot@chromium.org, Aug 14 2017

Cc: gov...@chromium.org
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Approved-61 merge-merged-3163
Labels: -ReleaseBlock-Stable
Project Member

Comment 25 by sheriffbot@chromium.org, Nov 14 2017

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

Sign in to add a comment