Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Starred by 1 user
Status: Fixed
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security

Blocking:
issue 591845



Sign in to add a comment
Security: chrome_child!mov_read_keys - Heap corruption as a result of an off-by-1 zero allocation
Reported by p...@paulmehta.com, Sep 3 2016 Back to list
VULNERABILITY DETAILS

FFMPEG MP4 decoder contains an Off-by-1 validation results in an allocation of size 0, followed by corrupting an arbitrary number of pointers out of bounds on the heap, where each is pointing to controllable or uninitialized data.

Additional details in attached file: mov_read_keys-heap_corruption-pmehta.readme.txt

https://cs.chromium.org/chromium/src/third_party/ffmpeg/libavformat/mov.c?rcl=0&l=3145

static int mov_read_keys(MOVContext *c, AVIOContext *pb, MOVAtom atom)
{
    uint32_t count;
    uint32_t i;

    if (atom.size < 8)
        return 0;

    avio_skip(pb, 4);
    count = avio_rb32(pb);	// count = 0x3FFFFFFF
    if (count > UINT_MAX / sizeof(*c->meta_keys)) {  // <------------------------------------ This check should be    if(count >= UINT_MAX ...
        av_log(c->fc, AV_LOG_ERROR,
               "The 'keys' atom with the invalid key count: %d\n", count);
        return AVERROR_INVALIDDATA;
    }

    c->meta_keys_count = count + 1;  // 0x3FFFFFFF + 1 = 0x40000000
    c->meta_keys = av_mallocz(c->meta_keys_count * sizeof(*c->meta_keys)); // results in an allocation of size 0, followed by corrupting an arbitrary number of pointers out of bounds on the heap 

.text:1159A4DE                 push    edi             ; s
.text:1159A4DF                 call    _avio_rb32			<--------- read arbitrary DWORD
.text:1159A4E4                 mov     esi, eax
.text:1159A4E6                 add     esp, 10h
.text:1159A4E9                 mov     [ebp+var_4], esi
.text:1159A4EC                 cmp     esi, 3FFFFFFFh		<--------- Size check
.text:1159A4F2                 jbe     short loc_1159A511	<---------- JBE, should be JB
.text:1159A4F4                 mov     eax, [ebp+c]
.text:1159A4F7                 push    esi
.text:1159A4F8                 push    offset aTheKeysAtomWit ; "The 'keys' atom with the invalid key co"...
.text:1159A4FD                 push    10h             ; level
.text:1159A4FF                 push    dword ptr [eax+4] ; avcl
.text:1159A502                 call    _av_log
.text:1159A507                 add     esp, 10h
.text:1159A50A                 mov     eax, 0BEBBB1B7h
.text:1159A50F                 jmp     short loc_1159A590
.text:1159A511 ; ---------------------------------------------------------------------------
.text:1159A511
.text:1159A511 loc_1159A511:                           ; CODE XREF: mov_read_keys+3Cj
.text:1159A511                 push    ebx
.text:1159A512                 mov     ebx, [ebp+c]
.text:1159A515                 lea     eax, [esi+1]
.text:1159A518                 mov     [ebx+2Ch], eax
.text:1159A51B                 shl     eax, 2 			<-------------- Int wrap, allocate zero
.text:1159A51E                 push    eax             ; size
.text:1159A51F                 call    _av_mallocz		
.text:1159A524                 mov     [ebx+28h], eax



VERSION
Chrome Version: Version 53.0.2785.89 m stable (32-bit only)
Operating System: All, testing done on Windows

REPRODUCTION CASE
see attached file, mov_read_keys-heap_corruption-pmehta.mp4

FOR CRASHES, PLEASE INCLUDE THE FOLLOWING ADDITIONAL INFORMATION
Type of crash: tab
Crash State: Varies depending on exploitation techniques.

 
mov_read_keys-heap_corruption-pmehta.readme.txt
6.9 KB View Download
Comment 1 by p...@paulmehta.com, Sep 3 2016
Oops... the attached mp4 was crashing the browser... lol. While amusing, this this was unintentional!

I've re-attached the sample with a different extension.
mov_read_keys-heap_corruption-pmehta.mp4.bin
512 KB Download
Comment 2 by aarya@google.com, Sep 3 2016
 Issue 643947  has been merged into this issue.
Project Member Comment 3 by clusterf...@chromium.org, Sep 3 2016
ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://cluster-fuzz.appspot.com/testcase?key=6505664681869312
Comment 4 by aarya@google.com, Sep 3 2016
Cc: dalecur...@chromium.org
Components: Internals>Media
Labels: Pri-1
Owner: xhw...@chromium.org
Status: Assigned
Thanks Paul for the report.
Project Member Comment 5 by clusterf...@chromium.org, Sep 3 2016
Labels: Security_Severity-High
Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6505664681869312

Job Type: windows_asan_chrome
Platform Id: windows

Crash Type: Heap-buffer-overflow WRITE 4
Crash Address: 0x0da11df0
Crash State:
  mov_read_keys
  mov_read_default
  mov_read_meta
  
Recommended Security Severity: High

Regressed: https://cluster-fuzz.appspot.com/revisions?job=windows_asan_chrome&range=415934:416413

Minimized Testcase (512.08 Kb): https://cluster-fuzz.appspot.com/download/AMIfv95VK4o2x1zGBuu9r1ybHj0BxqBroXg6ydAPe9lVj5AFOU_FQuq-9D1nXmJi3hiP-lmKBlXDXQXzprq7ewyTHxd7dpMVw4H8b56SRjletDEznibA5NpeLefeVi5sLTOE_S80snpcCnTbAOx4Ghh6tvJWBqCv6mfCz0wP79Bzwzoi8e8jUZ0?testcase_id=6505664681869312

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

A recommended severity was added to this bug. Please change the severity if it is inaccurate.

Comment 6 by aarya@google.com, Sep 3 2016
Cc: xhw...@chromium.org
Owner: servolk@chromium.org
Author: servolk
Project: chromium
Changelist: https://chromium.googlesource.com/chromium/src/+/4fba1f0c2c06184687345d202f0afd08ae8dde2a
Time: Fri Sep 02 02:04:50 2016
File ffmpeg_demuxer.cc is changed in this cl (and is part of stack frame #4, "media::FFmpegDemuxer::Initialize")
Minimum distance from crash line to modified line: 19. (file: ffmpeg_demuxer.cc, crashed on: 883, modified: 902).
So this is actually a vulnerability in ffmpeg (ffmpeg/libavformat/mov.c) and not in Chromium code. Reassigning to wolenetz@ who is about to roll ffmpeg.
Cc: servolk@chromium.org
Owner: wolenetz@chromium.org
Btw, I have just checked FFmpeg upstream and it looks to me like the issue is still not fixed there. Perhaps we should send a patch to FFmpeg? I can do that, but I'm wondering if we should disclose how it was found and give them access to this bug? Or just send the fix without any explanations?
FYI I've sent a patch to ffmpeg-devel:
http://ffmpeg.org/pipermail/ffmpeg-devel/2016-September/199089.html
The patch has landed in ffmpeg, I guess, we should get it with the next ffmpeg roll.
Blocking: 591845
Labels: Security_Impact-Stable
Labels: OS-All
Project Member Comment 15 by sheriffbot@chromium.org, Sep 9 2016
Labels: M-53
The patch looks good to me. Is there anything I can do to help out, or is this starting to wrap up?
I'll look into cherry-picking this into Chromium ASAP.
ffmpeg chromium git CR: https://chromium-review.googlesource.com/#/c/383956

Once that lands, I'll roll DEPS in ToT ASAP for it to bake and then begin merging back. Thank you, servolk@ for putting together the patch and sending it upstream already.
Project Member Comment 19 by bugdroid1@chromium.org, Sep 9 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/third_party/ffmpeg/+/12183cc1619bd0ae001fc61aa5434b76e2ca5cea

commit 12183cc1619bd0ae001fc61aa5434b76e2ca5cea
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Fri Sep 09 23:22:47 2016

avformat/mov: Fix potential integer overflow in mov_read_keys

Actual allocation size is computed as (count + 1)*sizeof(meta_keys), so
we need to check that (count + 1) won't cause overflow.

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
(cherry picked from commit 347cb14b7cba7560e53f4434b419b9d8800253e7)

BUG= 643948 

Change-Id: I30d9d9dad8812e969ea52afcfcfbea6bcc0bf208
Reviewed-on: https://chromium-review.googlesource.com/383956
Reviewed-by: Xiaohan Wang <xhwang@chromium.org>

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

Project Member Comment 20 by bugdroid1@chromium.org, Sep 9 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/third_party/ffmpeg/+/12183cc1619bd0ae001fc61aa5434b76e2ca5cea

commit 12183cc1619bd0ae001fc61aa5434b76e2ca5cea
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Fri Sep 09 23:22:47 2016

avformat/mov: Fix potential integer overflow in mov_read_keys

Actual allocation size is computed as (count + 1)*sizeof(meta_keys), so
we need to check that (count + 1) won't cause overflow.

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
(cherry picked from commit 347cb14b7cba7560e53f4434b419b9d8800253e7)

BUG= 643948 

Change-Id: I30d9d9dad8812e969ea52afcfcfbea6bcc0bf208
Reviewed-on: https://chromium-review.googlesource.com/383956
Reviewed-by: Xiaohan Wang <xhwang@chromium.org>

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

Project Member Comment 21 by bugdroid1@chromium.org, Sep 10 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6e5925dcef736146fa6eed5f2d88c9820963704e

commit 6e5925dcef736146fa6eed5f2d88c9820963704e
Author: wolenetz <wolenetz@chromium.org>
Date: Sat Sep 10 02:08:54 2016

Roll src/third_party/ffmpeg/ 35740fc7b..12183cc16 (5 commits).

https://chromium.googlesource.com/chromium/third_party/ffmpeg.git/+log/35740fc7b72a..12183cc1619b

$ git log 35740fc7b..12183cc16 --date=short --no-merges --format='%ad %ae %s'
2016-09-09 wolenetz avformat/mov: Fix potential integer overflow in mov_read_keys
2016-09-01 chcunningham Detect basename collisions irrespective of file extension.
2016-08-30 chcunningham Adding generate_gn_unittests.py to PRESUBMIT
2016-08-30 chcunningham FFmpeg generate_gn.py: Fix tests and logic reduction algorithm.
2016-08-29 apatole Fix Chrome ARM64 linux compilation errors

TBR=chcunningham@chromium.org,servolk@chromium.org, xhwang@chromium.org
BUG= 643948 , 627567 , 535788 , 613452 

Review-Url: https://codereview.chromium.org/2330633002
Cr-Commit-Position: refs/heads/master@{#417800}

[modify] https://crrev.com/6e5925dcef736146fa6eed5f2d88c9820963704e/DEPS

Cc: gov...@chromium.org bustamante@chromium.org
Currently the expected fix is in trunk, awaiting CF confirmation that it's fixed.
CC+=bustamante@ and govind@ in anticipation of requesting merges to m54 and m53 once CF verifies trunk is fixed.
Cc: awhalley@chromium.org
Labels: M-54
+ awhalley@chromium.org, this change is not yet baked in Beta so it we can't take it for this week M53 Stable release correct? 

Adding M54 label based on comment #22.
Hi govind@ - correct, not for this week's M53 release.
Project Member Comment 25 by sheriffbot@chromium.org, Sep 27 2016
wolenetz: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

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
The testcase has been marked as non reproducible on ClusterFuzz. Should we mark this as Fixed?
Project Member Comment 27 by clusterf...@chromium.org, Oct 11 2016
ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://cluster-fuzz.appspot.com/testcase?key=6754734893694976
Project Member Comment 28 by sheriffbot@chromium.org, Oct 11 2016
wolenetz: Uh oh! This issue still open and hasn't been updated in the last 28 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

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
Project Member Comment 29 by sheriffbot@chromium.org, Oct 13 2016
Labels: -M-53
Labels: -M-54 M-55
Status: Fixed
Project Member Comment 31 by sheriffbot@chromium.org, Oct 18 2016
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member Comment 32 by sheriffbot@chromium.org, Oct 21 2016
Labels: Merge-Request-55
Comment 33 by dimu@chromium.org, Oct 21 2016
Labels: -Merge-Request-55 Merge-Review-55 Hotlist-Merge-Review
[Automated comment] DEPS changes referenced in bugdroid comments, needs manual review.
+awhalley@ (Secutiy TPM) for M55 review.
Labels: reward-topanel
Good bug to take for M55.  An alternative to the full DEPS roll would be to cherry pick:

2016-09-09 wolenetz avformat/mov: Fix potential integer overflow in mov_read_keys

into an M55 branch, if that might be lower risk.


Labels: -Merge-Review-55 Merge-Request-55
Comment 37 by dimu@chromium.org, Oct 24 2016
Labels: -Merge-Request-55 Merge-Review-55
[Automated comment] DEPS changes referenced in bugdroid comments, needs manual review.
Labels: -Merge-Review-55 Merge-Approved-55
Approving merge to M55 branch 2883 based on comment #35 & #36. Please merge ASAP. Thank you.
This actually landed in M55 (https://bugs.chromium.org/p/chromium/issues/detail?id=643948#c21 https://chromium.googlesource.com/chromium/src.git/+/6e5925dcef736146fa6eed5f2d88c9820963704e r417800), and was awaiting potential merge to M54 and M53, though didn't get approval for those.
Labels: -Merge-Approved-55 Merge-Request-54
Per #22 and repeated attempts to get CF to verify this as fixed (finally successful Oct 11 per #27), this was awaiting such confirmation for merge to M54 (and originally, M53 too).

bustamante@: CF, as of #27, confirms, so requesting merge to M54 now.
Comment 41 by dimu@chromium.org, Oct 26 2016
Labels: -Merge-Request-54 Merge-Review-54
[Automated comment] There appears to be on-going work (i.e. bugroid changes), needs manual review.
Comment 42 by dimu@chromium.org, Oct 26 2016
Labels: -Merge-Request-54 Merge-Review-54
[Automated comment] There appears to be on-going work (i.e. bugroid changes), needs manual review.
Labels: -Merge-Review-54 Merge-Approved-54
Per discussion with awhalley@, approved for merging into 54
Labels: -reward-topanel reward-unpaid reward-5500
The panel awarded $5,500 for this - thanks for a great report!
Project Member Comment 46 by bugdroid1@chromium.org, Oct 28 2016
Labels: merge-merged-m54-cherry-picks
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/third_party/ffmpeg/+/cf2d534bf049984bf179d09488c5c86735ddbc1d

commit cf2d534bf049984bf179d09488c5c86735ddbc1d
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Fri Oct 28 01:38:43 2016

avformat/mov: Fix potential integer overflow in mov_read_keys

Actual allocation size is computed as (count + 1)*sizeof(meta_keys), so
we need to check that (count + 1) won't cause overflow.

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
(cherry picked from commit 347cb14b7cba7560e53f4434b419b9d8800253e7)

BUG= 643948 

Change-Id: I30d9d9dad8812e969ea52afcfcfbea6bcc0bf208
Reviewed-on: https://chromium-review.googlesource.com/383956
Reviewed-by: Xiaohan Wang <xhwang@chromium.org>
(cherry picked from commit 12183cc1619bd0ae001fc61aa5434b76e2ca5cea)

TBR=xhwang@chromium.org

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

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

The label "merge-merged-m54-cherry-picks" added in #46 doesn't mean this is merged to M54 yet. An M54 git-drover of a DEPS roll for ffmpeg to cf2d534b still needs to occur to complete this merge to M54.
Project Member Comment 48 by bugdroid1@chromium.org, Oct 28 2016
Labels: -merge-approved-54 merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0a318e4039f7af462baf77fe309f5e40a33ecda1

commit 0a318e4039f7af462baf77fe309f5e40a33ecda1
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Fri Oct 28 02:09:34 2016

M54: Roll FFmpeg 75976ae..cf2d534b

https://chromium.googlesource.com/chromium/third_party/ffmpeg/+log/75976ae..cf2d534b

cf2d534 avformat/mov: Fix potential integer overflow in mov_read_keys

BUG= 643948 
TBR=xhwang@chromium.org

Review URL: https://codereview.chromium.org/2453323005 .

Cr-Commit-Position: refs/branch-heads/2840@{#791}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/0a318e4039f7af462baf77fe309f5e40a33ecda1/DEPS

amineer@, thank you for updating the buildspec. (#48 had no effect on the release branch builds.)
Labels: -reward-unpaid reward-inprocess
Labels: Release-3-M54
Labels: CVE-2016-5199
Project Member Comment 54 by sheriffbot@chromium.org, Jan 24 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