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

Issue 390174 link

Starred by 0 users

Issue metadata

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



Sign in to add a comment

Heap-use-after-free in WebCore::KURL::~KURL

Project Member Reported by ClusterFuzz, Jun 30 2014

Issue description

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

Fuzzer: Therealholden_worker
Job Type: Windows_asan_chrome

Crash Type: Heap-use-after-free READ 4
Crash Address: 0x00bbf320
Crash State:
  - crash stack -
  WebCore::KURL::~KURL
  WebCore::NewWebSocketChannelImpl::~NewWebSocketChannelImpl
  - free stack -
  WebCore::NewWebSocketChannelImpl::fail
  WebCore::NewWebSocketChannelImpl::didFail
  

Unminimized Testcase: https://cluster-fuzz.appspot.com/download/AMIfv95RxGq__MGtehWVJLLroZxzQbZTPYEp45G6Ch4lGNvIpoGxOFsF7XlOlfce1U8Bhd9LFwThgdLpk0MiZVp-osJSGafRppbT3axVF9-9Pt6RnfsvoEaPXSWLN4Yd-vEkmd7dFFs1DWQ9gaQa6Gy0GTEYZkInsA

Additional requirements: Requires Interaction Gestures
Filer: inferno@chromium.org
 
Cc: therealh...@gmail.com
Owner: yhirano@chromium.org
Status: Assigned
Project Member

Comment 2 by ClusterFuzz, Jun 30 2014

Labels: Pri-1

Comment 3 by rsesek@chromium.org, Jun 30 2014

Labels: Security_Impact-Head M-38
Project Member

Comment 4 by ClusterFuzz, Jun 30 2014

Labels: ReleaseBlock-Beta
This medium+ severity security issue is a regression on trunk.

Please fix this asap. If you are unable to look into this soon, please revert your change.

- Your friendly ClusterFuzz
yhirano@ does this impact stable, beta ?
NewWebSocketChannelImpl was enabled on 37.
On 36 it is not used.
Labels: -Security_Impact-Head -M-38 Security_Impact-Beta M-37
Thanks, fixing tags.
Project Member

Comment 8 by ClusterFuzz, Jul 1 2014

Labels: -ReleaseBlock-Beta ReleaseBlock-Stable
Cc: hirosh...@chromium.org
Project Member

Comment 10 by bugdroid1@chromium.org, Jul 2 2014

The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=177359

------------------------------------------------------------------
r177359 | yhirano@chromium.org | 2014-07-02T08:08:05.347970Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp?r1=177359&r2=177358&pathrev=177359

[WebSocket] Task creation should be separated from task posting.

Having a complex argument as a waitForMethodCompletion may keep alive
temporary objects which must be killed before posting a task to another
thread.

BUG= 390174 

Review URL: https://codereview.chromium.org/368453003
-----------------------------------------------------------------
Status: Fixed
Project Member

Comment 12 by bugdroid1@chromium.org, Jul 2 2014

Labels: Merge-TBD
Is there a merge required here?
Project Member

Comment 13 by ClusterFuzz, Jul 2 2014

Labels: -Restrict-View-SecurityTeam Merge-Triage 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
Cc: tyoshino@chromium.org
Labels: -Merge-TBD Merge-Requested
inferno@, sorry, I was wrong.
This is not a NewWebSocketChannelImpl bug and it affects M36.
Hence merging to M36 and M37 is needed.

+tyoshino as I will be OOO until Jul 9.

Labels: -M-37 M-36
Labels: Security_Impact-Stable
Cc: -tyoshino@chromium.org yhirano@chromium.org
Owner: tyoshino@chromium.org
Labels: -Merge-Triage
amineer@ - Merge-Requested for M37 (branch 2062).

tyoshino@ - Please merge once approved by amineer@
Cc: -yhirano@chromium.org tyoshino@chromium.org
Owner: yhirano@chromium.org
timwillis@: I requested merge for M36 (and I will request for M37 after that), is that OK?


Labels: -M-36 M-37
This needs to be qualified on 37 before we can consider it for 36.  Replacing 36 label for 37.  When verified, feel free to request merge into 36.
Project Member

Comment 22 by bugdroid1@chromium.org, Jul 10 2014

The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=177815

------------------------------------------------------------------
r177815 | hiroshige@chromium.org | 2014-07-10T10:39:58.094107Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/page/NetworkStateNotifier.cpp?r1=177815&r2=177814&pathrev=177815
   M http://src.chromium.org/viewvc/blink/trunk/Source/modules/webdatabase/SQLTransactionClient.cpp?r1=177815&r2=177814&pathrev=177815
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/ExecutionContext.cpp?r1=177815&r2=177814&pathrev=177815
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/CrossThreadTask.h?r1=177815&r2=177814&pathrev=177815
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/MessagePort.cpp?r1=177815&r2=177814&pathrev=177815
   M http://src.chromium.org/viewvc/blink/trunk/Source/modules/mediastream/MediaStreamTrackSourcesRequestImpl.cpp?r1=177815&r2=177814&pathrev=177815
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/ExecutionContext.h?r1=177815&r2=177814&pathrev=177815
   M http://src.chromium.org/viewvc/blink/trunk/Source/web/ServiceWorkerGlobalScopeProxy.cpp?r1=177815&r2=177814&pathrev=177815
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/Document.cpp?r1=177815&r2=177814&pathrev=177815
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/workers/WorkerObjectProxy.cpp?r1=177815&r2=177814&pathrev=177815
   M http://src.chromium.org/viewvc/blink/trunk/Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp?r1=177815&r2=177814&pathrev=177815
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/ExecutionContextTask.h?r1=177815&r2=177814&pathrev=177815
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/Document.h?r1=177815&r2=177814&pathrev=177815

Replace CallClosureTask::create(bind()) with createCrossThreadTask()

Background: 
The code pattern of 
postTask(CallClosureTask::create(bind(args))) 
is thread unsafe for posting tasks crossing threads, even if args are
deep-copied, due to temporary objects (created as the return value of bind()
and deep copy functions such as String::isolatedCopy()). 

Solution by this CL: 
Created createCrossThreadTask() and replaced all
CallClosureTask::create(bind()) with createCrossThreadTask().
createCrossThreadTask calls bind and does deep copy (if necessary) by
CrossThreadCopier inside createCrossThreadTask, like createCallbackTask.
This is safer for cross-thread task posting because all temporary objects are
created inside createCrossThreadTask (not in its caller), and thus destroyed
before returning from createCrossThreadTask (i.e. before calling postTask). 

Removed postTask() and postInspectorTask() that accepts Closure&
(i.e. return value of bind()).

BUG=390851
BUG= 390174 

Review URL: https://codereview.chromium.org/374583002
-----------------------------------------------------------------
which revisions are you requesting for a merge?
Labels: -Merge-Requested Merge-Approved
merge of blink r177359 approved for m37 beta branch 2062
Project Member

Comment 26 by bugdroid1@chromium.org, Jul 15 2014

Labels: -Merge-Approved merge-merged-2062
The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=178129

------------------------------------------------------------------
r178129 | yhirano@chromium.org | 2014-07-15T03:25:55.027655Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/branches/chromium/2062/Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp?r1=178129&r2=178128&pathrev=178129

Merge 177359 "[WebSocket] Task creation should be separated from..."

> [WebSocket] Task creation should be separated from task posting.
> 
> Having a complex argument as a waitForMethodCompletion may keep alive
> temporary objects which must be killed before posting a task to another
> thread.
> 
> BUG= 390174 
> 
> Review URL: https://codereview.chromium.org/368453003

TBR=yhirano@chromium.org, amineer@chromium.org

Review URL: https://codereview.chromium.org/391833004
-----------------------------------------------------------------
Labels: -M-37 M-36 Merge-Requested
Requesting merge for M36: http://src.chromium.org/viewvc/blink?view=revision&revision=177359
Verified on Beta?
Labels: -Merge-Requested
Oh, I've just noticed that the latest beta 37.0.2062.20 doesn't include blink:178129. I should wait more...

Cc: timwillis@chromium.org matthewyuan@chromium.org
Labels: Merge-Requested
Beta verification is not part of the process for security bugs. If you want to add new things, please talk to me and Tim.
matthewyuan@ - to clarify, although beta verification is a "nice to have", sometimes we don't get that luxury with security bugs. I'll add this bug to the list of M36 release 1 candidates that I'll discuss tomorrow with inferno@ and get back to you.

Comment 32 by amin...@google.com, Jul 23 2014


Please note that all merge requests must have been on or rolled into trunk
for at least 24 hours to be considered for merging (to ensure full bot
coverage and give an opportunity for any necessary reverts to occur).

To help facilitate this request, if you could please answer the following:
--------------------------------------------------------------------------
1) Has this change been on trunk for at least 24 hours?

2) Has this change shipped to at least one canary release (where applicable)?

3) Has anyone verified that these changes resolve the issue and cause no new
   crashes?

4) Why is this necessary for this milestone?

Thanks!

(this message is auto-generated each time the merge-request label is
applied; if you have previously answered these questions kindly disregard)

1) Has this change been on trunk for at least 24 hours?
Yes.

2) Has this change shipped to at least one canary release (where applicable)?
Yes.

3) Has anyone verified that these changes resolve the issue and cause no new
   crashes?
We didn't succeed to reproduce the crash on our environment and hence we couldn't verify that the crash is resolved.
This is a thread-related crash and hard to reproduce.

4) Why is this necessary for this milestone?
This is a use-after-free bug although it rarely happens.

this got into the latest beta for 37, 37.0.2062.44, can you check to see if this caused any new stability issues on that branch?  I need to know in order to consider this change for merge.
Labels: -Merge-Requested Merge-Approved
Project Member

Comment 38 by bugdroid1@chromium.org, Jul 30 2014

Labels: -Merge-Approved merge-merged-1985
The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=179211

------------------------------------------------------------------
r179211 | yhirano@chromium.org | 2014-07-30T08:43:07.583992Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/branches/chromium/1985/Source/modules/websockets/WorkerThreadableWebSocketChannel.cpp?r1=179211&r2=179210&pathrev=179211

Merge 177359 "[WebSocket] Task creation should be separated from..."

> [WebSocket] Task creation should be separated from task posting.
>
> Having a complex argument as a waitForMethodCompletion may keep alive
> temporary objects which must be killed before posting a task to another
> thread.
>
> BUG= 390174 
>
> Review URL: https://codereview.chromium.org/368453003

R=tyoshino@chromium.org
TBR=matthewyuan@chromium.org, yhirano@chromium.org

Review URL: https://codereview.chromium.org/425223002
-----------------------------------------------------------------
Labels: -Security_Impact-Beta Release-1-M36
Labels: CVE-2014-3165
Labels: reward-unpaid reward-2000
Congrats Collin - $2000 for this report (UAF, but does not look like there is control between use and free).
Labels: -reward-topanel -reward-unpaid reward-inprocess
Labels: -reward-inprocess
Processing via our e-payment system can take a few weeks, but reward should be on its way to you. Thanks again for your help!
Project Member

Comment 44 by ClusterFuzz, Oct 8 2014

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

Comment 45 by tkent@chromium.org, Nov 26 2015

Labels: Cr-Blink-Network-WebSockets

Comment 46 by tkent@chromium.org, Nov 27 2015

Labels: -Cr-Blink-WebSockets
Project Member

Comment 47 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 48 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
Labels: CVE_description-submitted

Sign in to add a comment