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

Issue 478745 link

Starred by 0 users

Issue metadata

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



Sign in to add a comment

Heap-use-after-free in blink::ContainerNode::addChildNodesToDeletionQueue

Project Member Reported by ClusterFuzz, Apr 20 2015

Issue description

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

Fuzzer: Inferno_layout_test_unmodified
Job Type: Windows_asan_chrome_no_sandbox

Crash Type: Heap-use-after-free WRITE 4
Crash Address: 0x03a44260
Crash State:
  blink::ContainerNode::addChildNodesToDeletionQueue
  blink::ContainerNode::removeDetachedChildrenInContainer
  blink::ContainerNode::removeDetachedChildren
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=windows_asan_chrome_no_sandbox&range=325751:325771

Minimized Testcase (0.31 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv9626TWbkXmA6TheKZYzWXrEqTuvnojTKzQB0Fro18ZDBYyF--1FSNv-zSaIxh1q4aA4in3LMMbv40hsD8CgbZBT7QiioDq75vyZwq0VxC285SDDxuw_6sKZtlwmKzIxfZpWn6Wzhtp5FUfH4b3IRZSem4EZgQ
<script type="foo">
		s.appendChild(p)
	</script>
	<b>
		<p>
			<script>
			 p = document.querySelector("p");
			 s = document.querySelector("script");
			 s.appendChild(p);
			 s.type = "";
			 </script>
		</b>
	</p>
	<script>
		p.parentNode.innerHTML = "";
		(gc = function() {
		})();
		location.reload();
	</script>



Filer: inferno
 
Project Member

Comment 1 by ClusterFuzz, Apr 20 2015

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

Fuzzer: Inferno_layout_test_unmodified
Job Type: Windows_syzyasan_chrome

Crash Type: Use-after-free READ 4
Crash Address: 0x4f074be3
Crash State:
  blink::ContainerNode::notifyNodeRemoved
  blink::ContainerNode::addChildNodesToDeletionQueue
  blink::ContainerNode::removeDetachedChildrenInContainer
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=windows_syzyasan_chrome&range=325796:325798

Minimized Testcase (0.33 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv97nbbDXOuXGyu1hH8M57kDV4sERHtSvgiyswsLIkmS0ki65T1mY3whNUgvtKYCsuajTq1KC9s2IBv-GRRAn60tDAlsOvRdMykFfs1Rzp1FIAnuwgCimtJ9T_cmFJdvdgwFtt8WOBZf95pbQeRgHSfGU816FPA
<!-- -->
<script type="foo">
		s.appendChild(p)
	</script>
	<b>
		<p>
			<script>
			 p = document.querySelector("p");
			 s = document.querySelector("script");
			 s.appendChild(p);
			 s.type = "";
			 </script>
		</b>
	</p>
	<script>
		p.parentNode.innerHTML = "";
		p = null;
		(gc = function() {
		})();
		location.reload();
	</script>


Filer: inferno
Cc: tkent@chromium.org morrita@chromium.org esprehn@chromium.org
Owner: caseq@chromium.org
Status: Assigned
The regression range does not look correct, can anyone please help to find culprit. this look pretty bad uaf.

Could this have regressed from https://chromium.googlesource.com/chromium/blink.git/+/78442e298e001c6668a229d902a80604e45ca115
Project Member

Comment 3 by ClusterFuzz, Apr 20 2015

Labels: Pri-1 M-44

Comment 4 by caseq@chromium.org, Apr 20 2015

Owner: amineer@chromium.org
I can't see bug 464552, but I do suspect it's a duplicate, the test case is really similar there and reverting https://chromium.googlesource.com/chromium/blink/+/4dd6623614cff05eb3c18cd96f8d6ed089b7965b%5E%21/#F2 solves the problem for me.

Cc: -morrita@chromium.org amineer@chromium.org timwillis@chromium.org lafo...@chromium.org
Owner: morrita@chromium.org
Alex, this is so painful. We can't just revert a security fix [after a security release] [which also had a layouttest to show vuln] like this. Please make sure to reopen original security bug or notifying someone in security team to do this. 
I don't think putting a ScriptForbiddenScope inside the parser like that is right, it seems like it's just papering over some bigger issue...
It's tricky. I need help from someone who understands how the parse should work here.
I'm not sure how this type of tree normalization should be reasoned in a safe way.

Project Member

Comment 8 by ClusterFuzz, Apr 20 2015

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

Comment 9 by ClusterFuzz, Apr 22 2015

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

Fuzzer: Inferno_layout_test_unmodified
Job Type: Linux_asan_content_shell_drt

Crash Type: Heap-use-after-free READ 4
Crash Address: 0x60b00007f1f8
Crash State:
  blink::ContainerNode::attach
  blink::Element::attach
  blink::ContainerNode::attach
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_content_shell_drt&range=288412:288859

Minimized Testcase (0.39 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv95FMOUSDEcmgbLcrTP7JbMUT88ZmJGqSp0-8ZhctG5YLbjzRpheL_-UgHwihilg-Om7atgLuKMOrvDo3kVsuUc-xROtrxYdGAjSxkZk_7eHv5zo002GyMmV1JU7M12kOncTgQacSrgvOkK8Plrf8yX3yVQ10g
<!-- -->
<script type="foo">
		s.appendChild(p)
	</script>
	<b>
		<p>
			<script>
			 p = document.querySelector("p");
			 s = document.querySelector("script");
			 s.appendChild(p);
			 s.type = "";
			 </script>
		</b>
	<script>
		p.parentNode.innerHTML = "";
		p = null;
		(gc = function() {
			for (var i = 0; i < 30000; ++i)
				var s = new String("AAAA" + Math.random());
		})();
</script>


Filer: inferno
Cc: abarth@chromium.org eseidel@chromium.org
Adam, Eric, Elliot - do you any suggestions for Hajime to try here. This is a pretty painful security bug, we have already released info on it (with test) [found by top external reporter Serg], now the fix is reverted and the layout test crashes with different stacks everytime. 
Cc: morrita@chromium.org
Owner: esprehn@chromium.org
I'm looking into this today. So far I can see that the end result of the script + parser is that the tree is corrupted. The <p> is a child of both the first <script> and the body at the same time. This means that when you remove the <p> from one location and it ends up still in the tree in another but free'd and then we UAF when walking the tree later.
Here's a test that shows it without just crashing (it'll crash on reload, but not right away):

<script id="firstScript" type="invalid">
    s.appendChild(p)
</script>
<b>
    <p id="paragraph">
        <script>
             p = document.getElementById("paragraph");
             s = document.getElementById("firstScript");
             s.appendChild(p);
             s.type = "text/javascript";
         </script>
    <!-- End tag omitted for implicit close -->
</b>
<script>
    p.remove(); // (1)
    var p = s.firstElementChild; // (2)
    if (p) {
        document.body.textContent = "FAIL, " + p.tagName + " has parentNode = " + p.parentNode;
    }
</script>

Outputs:

FAIL, P has parentNode = null

which demonstrates the issue where the <p> is still in the tree at (2) even though it's been removed at (1).

Comment 13 Deleted

Okay here's another test case that shows the deeper fundamental issue, this prints:

"""
Before type change
After type change
Script did run, p's parentNode = null
p's found: 1
"""

which means the parser ran the first script after the type attribute changed. Note that Firefox doesn't ever run a script when the type attribute changes. The bug here seems to be that the firstScript is being marked for execution again and then it runs in the middle of the adoption agency algorithm. We shouldn't be running that script at all.

I need to figure out why we mark it for execution again and then where the execution happens.

I don't think abarth@ or eseidel@ are working on any Blink bugs anymore, but they'd be the most qualified to fix this issue inside the parser.

<script>
function log(string) {
    document.getElementById("result").textContent += string + "\n";
}
</script>
<div id="result" style="white-space: pre"></div>
<script id="firstScript" type="invalid">
    log("Script did run, p's parentNode = " + p.parentNode);
</script>
<b>
    <p id="paragraph">
        <script>
             p = document.getElementById("paragraph");
             s = document.getElementById("firstScript");
             console.log("parent in before", p.parentNode);
             s.appendChild(p);
             log("Before type change");
             s.type = "";
             log("After type change");
         </script>
    <!-- End tag omitted for implicit close -->
</b>
<script>
    var ps = document.querySelectorAll("p");
    log("p's found: " + ps.length);
</script>

My guess: we're calling childrenChanged on firstScript, which is causing it to call prepareScript() on its loader, which examines the type.

You'd think this would not even call prepareScript, since the element was parser inserted and there's a check for that, but note that ScriptLoader resets the m_parserInserted bit to false the first time it's called for a parser-inserted script.
Minimal fix (if my guess above is correct) would be to set m_alreadyStarted to true for the "wrong type" bailout case in ScriptLoader::prepareScript.
Here's a proposed fix:

https://codereview.chromium.org/1117973003

The wrong type bailout case is part of the spec, I hesitate to mess with it now. The spec also says we should run the script like this, so Firefox is "wrong". This fix at least plugs the hole until we can figure out what to do about the spec.
Project Member

Comment 18 by bugdroid1@chromium.org, May 2 2015

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

------------------------------------------------------------------
r194835 | esprehn@chromium.org | 2015-05-02T01:30:08.680692Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/html/parser/HTMLConstructionSite.cpp?r1=194835&r2=194834&pathrev=194835
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/ContainerNode.cpp?r1=194835&r2=194834&pathrev=194835
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/parser/execute-script-during-adoption-agency-removal-expected.txt?r1=194835&r2=194834&pathrev=194835
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/ContainerNode.h?r1=194835&r2=194834&pathrev=194835
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/parser/execute-script-during-adoption-agency-removal.html?r1=194835&r2=194834&pathrev=194835

parserInsertBefore and parserRemoveChild should check newChild for a parent.

parserRemoveChild can run script in obscure cases involving the adoption
agency changing the children of a script element, this script can then
move the element the parser is trying to insert back into the page so that
parserInsertBefore would then insert the newChild in the tree again. This
means the child ends up being inserted twice which can result in a use
after free if one is removed and a GC happens.

To fix this we run the removal in a loop inside the insert methods until
the child really is removed. We probably want to file a spec bug about this
too.

BUG= 478745 

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

Comment 20 by ClusterFuzz, May 2 2015

ClusterFuzz has detected this issue as potentially fixed, but it appears to be flaky.

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

Fuzzer: Inferno_layout_test_unmodified
Job Type: Windows_syzyasan_chrome

Crash Type: Use-after-free READ 4
Crash Address: 0x4f074be3
Crash State:
  blink::ContainerNode::notifyNodeRemoved
  blink::ContainerNode::addChildNodesToDeletionQueue
  blink::ContainerNode::removeDetachedChildrenInContainer
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=windows_syzyasan_chrome&range=325796:325798

Minimized Testcase (0.33 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv97nbbDXOuXGyu1hH8M57kDV4sERHtSvgiyswsLIkmS0ki65T1mY3whNUgvtKYCsuajTq1KC9s2IBv-GRRAn60tDAlsOvRdMykFfs1Rzp1FIAnuwgCimtJ9T_cmFJdvdgwFtt8WOBZf95pbQeRgHSfGU816FPA
<!-- -->
<script type="foo">
		s.appendChild(p)
	</script>
	<b>
		<p>
			<script>
			 p = document.querySelector("p");
			 s = document.querySelector("script");
			 s.appendChild(p);
			 s.type = "";
			 </script>
		</b>
	</p>
	<script>
		p.parentNode.innerHTML = "";
		p = null;
		(gc = function() {
		})();
		location.reload();
	</script>

If you suspect that the result above is incorrect,try re-doing that job on the test case report page.
Project Member

Comment 21 by ClusterFuzz, May 2 2015

ClusterFuzz has detected this issue as fixed in range 328045:328051.

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

Fuzzer: Inferno_layout_test_unmodified
Job Type: Linux_asan_content_shell_drt

Crash Type: Heap-use-after-free READ 4
Crash Address: 0x60b00007f1f8
Crash State:
  blink::ContainerNode::attach
  blink::Element::attach
  blink::ContainerNode::attach
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_content_shell_drt&range=288412:288859
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_content_shell_drt&range=328045:328051

Minimized Testcase (0.39 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv95FMOUSDEcmgbLcrTP7JbMUT88ZmJGqSp0-8ZhctG5YLbjzRpheL_-UgHwihilg-Om7atgLuKMOrvDo3kVsuUc-xROtrxYdGAjSxkZk_7eHv5zo002GyMmV1JU7M12kOncTgQacSrgvOkK8Plrf8yX3yVQ10g
<!-- -->
<script type="foo">
		s.appendChild(p)
	</script>
	<b>
		<p>
			<script>
			 p = document.querySelector("p");
			 s = document.querySelector("script");
			 s.appendChild(p);
			 s.type = "";
			 </script>
		</b>
	<script>
		p.parentNode.innerHTML = "";
		p = null;
		(gc = function() {
			for (var i = 0; i < 30000; ++i)
				var s = new String("AAAA" + Math.random());
		})();
</script>

If you suspect that the result above is incorrect,try re-doing that job on the test case report page.
Project Member

Comment 22 by ClusterFuzz, May 2 2015

ClusterFuzz has detected this issue as potentially fixed, but it appears to be flaky.

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

Fuzzer: Inferno_layout_test_unmodified
Job Type: Windows_asan_chrome_no_sandbox

Crash Type: Heap-use-after-free WRITE 4
Crash Address: 0x03a44260
Crash State:
  blink::ContainerNode::addChildNodesToDeletionQueue
  blink::ContainerNode::removeDetachedChildrenInContainer
  blink::ContainerNode::removeDetachedChildren
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=windows_asan_chrome_no_sandbox&range=325751:325771

Minimized Testcase (0.31 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv9626TWbkXmA6TheKZYzWXrEqTuvnojTKzQB0Fro18ZDBYyF--1FSNv-zSaIxh1q4aA4in3LMMbv40hsD8CgbZBT7QiioDq75vyZwq0VxC285SDDxuw_6sKZtlwmKzIxfZpWn6Wzhtp5FUfH4b3IRZSem4EZgQ
<script type="foo">
		s.appendChild(p)
	</script>
	<b>
		<p>
			<script>
			 p = document.querySelector("p");
			 s = document.querySelector("script");
			 s.appendChild(p);
			 s.type = "";
			 </script>
		</b>
	</p>
	<script>
		p.parentNode.innerHTML = "";
		(gc = function() {
		})();
		location.reload();
	</script>

If you suspect that the result above is incorrect,try re-doing that job on the test case report page.
Why does CF think this is flaky fixed? I think CF might be confused.
Cc: mbarbe...@chromium.org
Elliot, It is fixed correctly, see c#21 from linux job types. I think there is some issue with windows job types, so ignore c#20, c#22. Marty, can you take a look at windows bots and redo to see what is causing this.
Project Member

Comment 25 by ClusterFuzz, May 4 2015

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify Merge-NA
Cc: ian.hick...@gmail.com
Adam asked me to look at this.

The minimised test case for the script execution issue is:

<script type="bogus">w('script 1')</script>
<b>
<p>
<script>
 w('start of script 2');
 var p = document.querySelector("p");
 var s = document.querySelector("script");
 s.appendChild(p); // causes script 1 to attempt to run, but it still doesn't have a supported type
 s.type = "";
 w('end of script 2');
</script>
</b> <!-- AAA pulls <p> out of <script> -->

Per the HTML spec, we should execute scripts when nodes are added to them, but not when nodes are removed. This markup causes the <p> element to be removed from the <script> element (by the AAA), but that shouldn't cause the script to execute. This appears to be correct and matches Firefox; it should be fixed in Chrome.

This is independent of the bug wherein the DOM ends up being non-coherent (which was the cause of the security bug).
 Issue 478577  has been merged into this issue.
Project Member

Comment 29 by ClusterFuzz, Aug 8 2015

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

Comment 30 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 31 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