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 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2015
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug-Security



Sign in to add a comment
link

Issue 453279: Heap-use-after-free in blink::MutationObserverRegistration::unregister

Reported by saif.els...@gmail.com, Jan 29 2015

Issue description

VULNERABILITY DETAILS

chrome-heap-use-after-free-blinkMutationObserverRegistrationunregister that crashes on 

==20307==ERROR: AddressSanitizer: heap-use-after-free on address 0x60600015c928 at pc 0x0001125c6b36 bp 0x7fff53c359c0 sp 0x7fff53c359b8

VERSION
Chrome Version: 42.0.2276.0 dev
Operating System: Mac OS X 

REPRODUCTION CASE
Open Attached minimized test case POC.html

FOR CRASHES, PLEASE INCLUDE THE FOLLOWING ADDITIONAL INFORMATION
Type of crash: tab
Crash State: asanlog.txt
 
POC.html
1.5 KB View Download
asanlog.txt
18.3 KB View Download

Comment 1 by saif.els...@gmail.com, Jan 29 2015

the --js-flags=--expose-gc switch should be used sorry for not including it in original report

Comment 2 by ClusterFuzz, Jan 29 2015

Project Member
ClusterFuzz is analyzing your testcase. Chromium developers can follow the progress at https://cluster-fuzz.appspot.com/testcase?key=5750223883206656

Comment 3 by ClusterFuzz, Jan 29 2015

Project Member
Summary: Heap-use-after-free in blink::MutationObserverRegistration::unregister (was: Security: chrome-heap-use-after-free-blinkMutationObserverRegistrationunregister)
Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5750223883206656

Uploader: rickyz@chromium.org
Job Type: Mac_asan_chrome

Crash Type: Heap-use-after-free READ 8
Crash Address: 0x60c00019fdc8
Crash State:
  blink::MutationObserverRegistration::unregister
  blink::MutationObserver::disconnect
  blink::MutationObserverV8Internal::disconnectMethodCallback
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=mac_asan_chrome&range=313665:313666

Minimized Testcase (0.90 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv94k9y-oYe1S4m4cNMvRBvr8wHWpMk6dlX9hueQNvyq1z-RRJpFTV3y8uZYyZTaP5n3OBR0UlPl2MwedQjQj3zI35g33sTUJBqI2p9l_G7bIUSm_D74aB2qS9Af0-XKcXN9VHUgGBuQmPKfdtx7HUtlcKnlJYA
<script>
try{ HTML0=document.createElement("ABBR")} catch(e){}
try{ HTML1=document.createElement("TITLE")} catch(e){}
try{ HTML2=document.createElement("THEAD")} catch(e){}
try{ HTML0.appendChild(HTML2)} catch(e){}
try{ HTML3=document.createElement("PROGRESS")} catch(e){}
try{ HTML1.appendChild(HTML3)} catch(e){}
try{ HTML4=document.createElement("INS")} catch(e){}
try{ HTML1.appendChild(HTML4)} catch(e){}
try{ var observer0= new MutationObserver(function() {})} catch(e){}
try{ observer0.observe(HTML4,{childList:1, })} catch(e){}
try{ observer0.observe(HTML1,{childList:1, subtree:1, })} catch(e){}
try{ HTML3=HTML3.nextSibling} catch(e){}
try{ HTML4=HTML2.parentNode} catch(e){}
try{ HTML3.outerHTML=HTML4.innerHTML} catch(e){}
try{ delete createdElements['HTML4']} catch(e){}
try{ HTML3=HTML4.previousSibling} catch(e){}
try{ gc()} catch(e){}
try{ observer0.disconnect()} catch(e){}
window.location.reload()
</script>

Comment 4 by ClusterFuzz, Jan 29 2015

Project Member
Labels: Stability-Memory-AddressSanitizer Security_Impact-Head
Status: Available

Comment 5 by infe...@chromium.org, Jan 29 2015

Cc: zhaoze.z...@partner.samsung.com
Status: Assigned
Author: zhaoze.zhou@partner.samsung.com
Component: blink
Changelist: https://chromium.googlesource.com/chromium/blink.git/+/b0f21fa42c4367b07d7faaa8f269f31d5333c7fa
Time: Wed Jan 14 05:33:44 2015
The CL last changed line 160 of file MutationObserver.cpp, which is stack frame 1.

Comment 6 by infe...@chromium.org, Jan 29 2015

Cc: dcheng@chromium.org jchaffraix@chromium.org
Weird, it says zhaoze.zhou@partner.samsung.com email is wrong. +Cc reviewers for that cl.

Comment 7 by rickyz@chromium.org, Jan 29 2015

Cc: adamk@chromium.org
Labels: Security_Severity-High Pri-1 M-40
Nice find. I hand-minimized it down to the attached file.

It looks like you have:

holder -> target

And you observe holder (with subtree) and target. This creates two MutationObserverRegistration that are owned by the each node.

When target is removed from holder, observedSubtreeNodeWillDetach is called, which converts the node to a transient registration node, and the nulling/gc makes holder's MutationObserverRegistration the owner of target.

When the observer is disconnected, it makes a copy of its registration list and starts iterating through it (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/dom/MutationObserver.cpp&l=158). First, it unregisters the registration on parent, which deletes target along with its registration. Then it hits target, which  has now been deleted.

Would it be valid to add a similar check to https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/dom/Node.cpp&l=367 for non-transient observers?

Also, how correct is making a copy of the registration set and iterating through that? Would a while (!m_registrations.isEmpty()) loop be a correct fix for this?

Adding adamk@ who has looked at a similar bug in the past.
min.html
381 bytes View Download

Comment 8 by ClusterFuzz, Jan 29 2015

Project Member
Labels: -Security_Impact-Head Security_Impact-Stable

Comment 9 by saif.els...@gmail.com, Jan 30 2015

Thank you for the explanation it's really awesome of you I appreciate it

Comment 10 by dcheng@chromium.org, Jan 30 2015

Owner: rafaelw@chromium.org
https://chromium.googlesource.com/chromium/blink.git/+/8816db0d28f1dceff04721619e17bbae89b53f9c is a better culprit. https://chromium.googlesource.com/chromium/blink.git/+/b0f21fa42c4367b07d7faaa8f269f31d5333c7fa just uses a C++11 foreach loop, which should have no effect on the correctness of this code.

Comment 11 by infe...@chromium.org, Jan 30 2015

Cc: pdr@chromium.org

Comment 12 by adamk@chromium.org, Feb 2 2015

Cc: rafaelw@chromium.org
Owner: rickyz@chromium.org
The first solution that comes to mind is to make MutationRegistrations RefCounted and make the transientRegistry hold RefPtrs; it seems like that should make this test case stop crashing, and since transient registrations are cleared at the end of each microtask, it seems unlikely to lead to memory leaks. I'm not working on Blink full time these days (nor is rafaelw), but I'd be happy to review such a change.

As a side note, this seems related to other MutationObserver GC bugs, like issue 329103. target's JS wrapper shouldn't be collected by the GC, since there's a MutationRecord with a reference to it in observer's m_records. But I don't think that fixing 329103 would necessarily make the existing code safe.

Comment 13 by infe...@chromium.org, Feb 12 2015

Owner: rafaelw@chromium.org
Rafael, since you caused the regression, can you please take a look or suggest an owner. Ricky is on the security team, so wont be fixing this.

Comment 14 by rafaelw@chromium.org, Feb 12 2015

@inferno, it's unclear to me how I caused this regression (other than helping to create this feature originally), but as Adam points out, I don't work on blink anymore either.

If I'm really the only person available to do this, I will.

That said, objects like MutationObserver which have subtle GC behavior have long been problems for blink and it's been quite a long time since I've understood how any of it works. Me fixing it would require investing time in re-leaning the current system and it seems like that would be a better investment for someone who continues to work on blink.

@pdr or @adamk, can you think of a good candidate who still works on blink?

Comment 15 by adamk@chromium.org, Feb 12 2015

Cc: haraken@chromium.org
+haraken, who runs a team with "DOM" in its name...

Comment 16 by ClusterFuzz, Feb 20 2015

Project Member
Labels: -M-40 M-41

Comment 17 by ClusterFuzz, Feb 27 2015

Project Member
Labels: Nag
rafaelw@: Uh oh! This issue is still open and hasn't been updated in the last 14 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz

Comment 18 by ClusterFuzz, Mar 13 2015

Project Member
rafaelw@: Uh oh! This issue is still open and hasn't been updated in the last 28 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz

Comment 19 by pdr@chromium.org, Mar 13 2015

Owner: haraken@chromium.org
rafaelw no longer works on blink.

@haraken, would you be willing to triage this?

Comment 20 by adamk@chromium.org, Mar 13 2015

For those late to this bug, the combination of comments 7 and 12 should pretty well explain the work to be done here.

Comment 21 by adamk@chromium.org, Mar 13 2015

Cc: -zhaoze.z...@partner.samsung.com

Comment 22 by haraken@chromium.org, Mar 25 2015

Owner: adamk@chromium.org
I don't have a bandwidth to look into this issue. Let me assign this to Adam at the moment.

Comment 23 by adamk@chromium.org, Mar 25 2015

Owner: ----
Status: Available
As I don't work on Blink anymore, this shouldn't sit in my queue. Marking as available to reflect the real state of the world. Though as rafaelw said above, if there's really no one on Blink who has time for this one of us may get to it eventually.

Comment 24 by pdr@chromium.org, Mar 26 2015

Owner: infe...@chromium.org
Status: Untriaged
Assigning to @inferno to triage/investigate because this is high severity.

Comment 25 by infe...@chromium.org, Mar 26 2015

Owner: kouhei@chromium.org
Status: Assigned
Kouhei@, can you please take a look.

Comment 26 by kouhei@chromium.org, Mar 27 2015

Status: Started
I'll try to work on this for 2hrs, but I'm currently flooded with other urgent tasks/bugs so feel free to take this one.

Comment 27 by kouhei@chromium.org, Mar 27 2015

Never mind. Managed to write a fix with in 2hrs: https://codereview.chromium.org/1039523003/

Comment 28 by kouhei@chromium.org, Mar 31 2015

Waiting for clusterfuzz to pick up fix

Comment 29 by infe...@chromium.org, Mar 31 2015

Status: Fixed
This was a one-time crasher on CF (hit a few times), so it can't verify.

Comment 30 by ClusterFuzz, Mar 31 2015

Project Member
Labels: -Restrict-View-SecurityTeam M-42 M-43 Restrict-View-SecurityNotify Merge-Triage
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 fix is very close to the branch point. After the branch happens, please make sure to check if your fix is in.

- Your friendly ClusterFuzz

Comment 31 by bugdroid1@chromium.org, Mar 31 2015

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

------------------------------------------------------------------
r192655 | kouhei@chromium.org | 2015-03-27T07:16:44.890912Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/MutationObserver.cpp?r1=192655&r2=192654&pathrev=192655

MutationObserver: add a check that iterating registration still exists in original set

The MutationObserver registration may be unregistered from the original set
while iterating on the cloned set.
Add a check so that it would only call unregister() on active registrations.

BUG= 453279 
TEST=manually tested from fuzzer repro case :(

Review URL: https://codereview.chromium.org/1039523003
-----------------------------------------------------------------

Comment 32 by timwillis@google.com, Apr 8 2015

Labels: reward-topanel

Comment 33 by timwillis@google.com, Apr 8 2015

Labels: -M-41 -Nag -M-43 -Merge-Triage Merge-Requested
Merge-Requested to M42 - branch 2311 

(noting that this request is after the stable candidate qualification, so may not go out with first M42 unless there's a respin)

Comment 34 by timwillis@google.com, Apr 8 2015

Cc: timwillis@chromium.org

Comment 35 by laforge@google.com, Apr 8 2015

Labels: -Merge-Requested Merge-Review Hotlist-Merge-Review
[Automated comment] Less than 2 weeks to go before stable on M42, manual review required.

Comment 36 by amineer@chromium.org, Apr 8 2015

Labels: -Merge-Review Merge-Approved
merge approved for m42 branch 2311

Comment 37 by bugdroid1@chromium.org, Apr 9 2015

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

------------------------------------------------------------------
r193431 | haraken@chromium.org | 2015-04-09T08:53:19.949390Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/branches/chromium/2311/Source/core/dom/MutationObserver.cpp?r1=193431&r2=193430&pathrev=193431

Merge 192655 "MutationObserver: add a check that iterating regis..."

> MutationObserver: add a check that iterating registration still exists in original set
> 
> The MutationObserver registration may be unregistered from the original set
> while iterating on the cloned set.
> Add a check so that it would only call unregister() on active registrations.
> 
> BUG= 453279 
> TEST=manually tested from fuzzer repro case :(
> 
> Review URL: https://codereview.chromium.org/1039523003

TBR=kouhei@chromium.org

Review URL: https://codereview.chromium.org/1072773002
-----------------------------------------------------------------

Comment 38 by timwillis@google.com, Apr 27 2015

Labels: Release-1-M42

Comment 39 by timwillis@google.com, Apr 28 2015

Labels: CVE-2015-1243

Comment 40 by timwillis@google.com, Jun 10 2015

Labels: -reward-topanel reward-unpaid reward-3000
Congrats - $3000 for this report. We'll start payment shortly.

Comment 41 by timwillis@google.com, Jun 25 2015

Labels: -reward-unpaid reward-inprocess

Comment 42 by ClusterFuzz, Jul 7 2015

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

Comment 43 by timwillis@google.com, Jul 24 2015

Labels: -reward-inprocess
Processing via our e-payment system can take up to two weeks, but the reward should be on its way to you. Thanks again for your help!

(Note: sorry for the delay here - it turns out in the new payment system, these payments were waiting for a second approval from me).

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

Project Member
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

Comment 45 by sheriffbot@chromium.org, Oct 2 2016

Project Member
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

Comment 46 by mbarbe...@chromium.org, Oct 2 2016

Labels: allpublic

Comment 47 by awhalley@chromium.org, Apr 25 2018

Labels: CVE_description-submitted

Sign in to add a comment