New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 0 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2014
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security



Sign in to add a comment

Heap-use-after-free in content::BufferedDataSource::ReadCallback

Project Member Reported by ClusterFuzz, Sep 5 2014

Issue description

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5479201185464320

Fuzzer: Inferno_flicker
Job Type: Linux_asan_chrome_media

Crash Type: Heap-use-after-free WRITE {*}
Crash Address: 0x7f457cd55800
Crash State:
  content::BufferedDataSource::ReadCallback
  content::BufferedResourceLoader::DoneRead
  content::BufferedResourceLoader::Read
  

Unminimized Testcase: https://cluster-fuzz.appspot.com/download/AMIfv97l2DIqnBNU7Qe2Q3Y-f8zr4aGnXqTUeBtcc1Msol5d-47XrLL9laWKqoG7m_bzk6IVq5z0yfnaZ-8eU9deJbEaqieUtGVNJGMIh4-t1fh72id4At5vcZBf6gfVOfoBkCB9vHlBXU7_FmMF7NvpecNAaBeAix-Gf5Q26Tq_5euKFXlGJHM


Filer: inferno
 
Cc: scherkus@chromium.org
Labels: Cr-Internals-Media
Owner: dalecur...@chromium.org
Status: Assigned
Project Member

Comment 2 by ClusterFuzz, Sep 5 2014

Labels: Pri-1
Hmm, I think BlockingURLProtocol::Abort() is letting an outstanding Read operation live longer than it should.

1. append_packet_chunked() allocates the output data here via av_grow_packet():
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg/libavformat/utils.c&l=203

2. append_packet_chunked() issues an avio_read() shortly after which triggers FFmpegGlue::AVIOReadOperation() -> BlockingURLProtocol::Read() -> BufferedDataSource::Read(), after which the BlockingURLProtocol blocks until a callback signal is received during BufferedDataSource::ReadCallback().

3. The above seems to run through 1 or more iterations, but eventually the do {} while loop completes and the original pkt is "exhausted" (pkt->size == 0).  FFmpeg releases the read buffer at this time.

I suspect a BlockingURLProtocol::Abort() is happening during step 3, but is failing to abort any outstanding read operations correctly. I'll keep digging.  Any thoughts scherkus?
I suspect we need to Stop the DataSource before Aborting the URLProtocol in the FFmpegDemuxer:

https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/ffmpeg_demuxer.cc&l=560
definitely strange -- it'd be good to have a repro and to understand if it's the abort code path that's letting outstanding operations get UAF'd
Status: Started
My assumptions were correct. I've repro'd the issue locally and have put together a fix here: https://codereview.chromium.org/544843005/

It looks like this has existed for a long time so I'd guess this impacts all the way down to stable. The code has changed slightly since then, but the core issue seems to remain:

http://src.chromium.org/viewvc/chrome/branches/2062/src/media/filters/ffmpeg_demuxer.cc?revision=289410

Labels: Security_Impact-Stable
Project Member

Comment 8 by ClusterFuzz, Sep 6 2014

Labels: M-37
Project Member

Comment 9 by bugdroid1@chromium.org, Sep 9 2014

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

commit 1973f0b6f7497b1887397de9bb36dbff4faf5e39
Author: dalecurtis <dalecurtis@chromium.org>
Date: Tue Sep 09 20:03:19 2014

Avoid FFmpeg destroying active resources during demuxer stop.

The root cause is a combination of BufferedDataSource::Stop()
requiring a lock and BlockingURLProtocol::Read() returning
control to FFmpeg which may destory resources in use by an
ongoing BufferedDataSource::ReadCallback() call.

The lock in BDS::Stop() shared by BDS::ReadCallback() prevents
the stop signal from taking affect until after the read finishes,
however calling BUP::Abort() will immediately returns control to
FFmpeg which may destroy the resources in use by BDS::ReadCallback().

Sadly, given the racy nature and multi-class dependencies I've been
unable to craft a unit test for this, but I've manually verified this
fix works by arbitrarily delaying BDS::Stop() under an ASAN build.

BUG= 411318 
TEST=none

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

Cr-Commit-Position: refs/heads/master@{#293995}

[modify] https://chromium.googlesource.com/chromium/src.git/+/1973f0b6f7497b1887397de9bb36dbff4faf5e39/media/filters/ffmpeg_demuxer.cc

Status: Fixed
Project Member

Comment 11 by ClusterFuzz, Sep 10 2014

Labels: -Restrict-View-SecurityTeam Merge-Triage M-38 Restrict-View-SecurityNotify
Adding Merge-Triage label for tracking purposes.

Once your fix had sufficient bake time (on canary, dev as appropriate), please nominate your fix for merge by adding the Merge-Requested label.

When your merge is approved by the release manager, please start merging with higher milestone label first. Make sure to re-request merge for every milestone in the label list. You can get branch information on omahaproxy.appspot.com.

- Your friendly ClusterFuzz
Has ClusterFuzz been updated recently? I was hoping it would detect this as fixed before marking for merge.
ok updated the build now, should get notification comment in < 1 day.
Project Member

Comment 14 by ClusterFuzz, Sep 12 2014

ClusterFuzz has detected this issue as fixed in latest custom build.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5479201185464320

Fuzzer: Inferno_flicker
Job Type: Linux_asan_chrome_media

Crash Type: Heap-use-after-free WRITE {*}
Crash Address: 0x7f66e9cba800
Crash State:
  content::BufferedDataSource::ReadCallback
  content::BufferedResourceLoader::DoneRead
  content::BufferedResourceLoader::Read
  

Minimized Testcase (45019.62 Kb): https://cluster-fuzz.appspot.com/download/AMIfv94DSe4Po0AKHdbZF66zLh4Lnwsm8gqw4UwZNDYcrWAvso20JkQF7cpe7ufVT51sc1J7rDyR7y9_MfcWUDcEhG5usiFKiDKxWxt5-PE7H5WSSP115mSNlT1mKmPQUTosVzhxDMAJRPlKZx3EeW4CTHzVoFIRF8ZaJr1grynE_MLNBXrfmRw

If you suspect that the result above is incorrect, try re-doing that job on the testcase report page.

Labels: Merge-Requested
Labels: -Merge-Requested Merge-Approved
Approved for 38.
Project Member

Comment 17 by bugdroid1@chromium.org, Sep 13 2014

Labels: -Merge-Approved merge-merged-2125
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fa897c995fa7c7a209c389f4521d8a875c056c5a

commit fa897c995fa7c7a209c389f4521d8a875c056c5a
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Sat Sep 13 00:10:49 2014

Merge "Avoid FFmpeg destroying active resources during demuxer stop."

The root cause is a combination of BufferedDataSource::Stop()
requiring a lock and BlockingURLProtocol::Read() returning
control to FFmpeg which may destory resources in use by an
ongoing BufferedDataSource::ReadCallback() call.

The lock in BDS::Stop() shared by BDS::ReadCallback() prevents
the stop signal from taking affect until after the read finishes,
however calling BUP::Abort() will immediately returns control to
FFmpeg which may destroy the resources in use by BDS::ReadCallback().

Sadly, given the racy nature and multi-class dependencies I've been
unable to craft a unit test for this, but I've manually verified this
fix works by arbitrarily delaying BDS::Stop() under an ASAN build.

BUG= 411318 
TEST=none
TBR=dalecurtis,scherkus

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

Cr-Commit-Position: refs/heads/master@{#293995}
(cherry picked from commit 1973f0b6f7497b1887397de9bb36dbff4faf5e39)

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

Cr-Commit-Position: refs/branch-heads/2125@{#337}
Cr-Branched-From: b68026d94bda36dd106a3d91a098719f952a9477-refs/heads/master@{#290040}

[modify] https://chromium.googlesource.com/chromium/src.git/+/fa897c995fa7c7a209c389f4521d8a875c056c5a/media/filters/ffmpeg_demuxer.cc

Labels: -M-37 -Merge-Triage
Labels: Release-0-M38
Project Member

Comment 20 by ClusterFuzz, Dec 17 2014

Labels: -Restrict-View-SecurityNotify
Bulk update: removing view restriction from closed bugs.
Project Member

Comment 21 by sheriffbot@chromium.org, Oct 1 2016

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 22 by sheriffbot@chromium.org, Oct 2 2016

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
Labels: allpublic

Sign in to add a comment