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: Dec 2012
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug-Security

Sign in to add a comment

Issue 139168: Security: Creating a loop in the DOM tree (99% a DoS)

Reported by, Jul 26 2012

Issue description

Chrome Version: 20.0.1132.57 m stable
Operating System: all


bool ContainerNode::replaceChild(PassRefPtr<Node> newChild, Node* oldChild, ExceptionCode& ec, bool shouldLazyAttach)
1.  checkReplaceChild(newChild.get(), oldChild, ec);
    if (ec)
        return false;

2.  removeChild(oldChild, ec);
    if (ec)
        return false;
    // Add the new child(ren)
    for (NodeVector::const_iterator it = targets.begin(); it != targets.end(); ++it) {
        if (next)
3.          insertBeforeCommon(next.get(), child);
4.      updateTreeAfterInsertion(this, child, shouldLazyAttach);

1 - check hierarchy and bail if (but not only if) oldChild is a descendant of newChild
2 - by intercepting the removal event, we mess up the DOM a bit, so that oldChild->next() is
    a descendant of newChild
3 - this creates a loop in the DOM: newChild is accessible by next->prev() and next->parent()->...
4 - infinite loop

Program received signal SIGSEGV, Segmentation fault.
WebCore::Node::typeTag (this=<error reading variable: Cannot access memory at address 0x7fffc0617ff0>) at third_party/WebKit/Source/WebCore/dom/Node.h:703
703	    NodeTypeTag typeTag() const { return static_cast<NodeTypeTag>(m_nodeFlags & NodeTypeTagMask); }

This could be exploitable, if there was a way not to crash inside
updateTreeAfterInsertion(). I don't see how that could be done, so you decide.

Adding checkReplaceChild just before the for() would solve this problem.
1.0 KB View Download

Comment 1 by, Jul 28 2012

Status: WontFix
This is a renderer stack exhaustion. It's not a security issue because we isolate renderers in separate processes. This means that a site can crash its own tab, but does not affect the browser or other tabs.

Comment 2 by, Jul 29 2012

This isn't a stack exhaustion caused by, for example, a deeply nested html like "<p>"^n+"</p>"^n. If it's possible to intercept an event raised from updateTreeAfter* (before the stack exhaustion), or avoid crashing inside this procedure in any other way, then manipulating a looped DOM tree from JS could potentially lead to more interesting crashes. Classifying it as a DOS basing only on the stack signature is insufficient, IMO.

Comment 3 by, Jul 29 2012

Yeah, we need the opinion of a DOM expert here. cc:ed Dimitri. This sounds similar to some DOM issues noted by Mark Dowd in his audit a couple of years back.

Comment 4 by, Jul 29 2012

Ryosuke and Morrita-san have totally stolen the DOM expert title from me :)

Comment 5 by, Jul 30 2012


Comment 7 by, Jul 30 2012

Status: Available

Comment 8 by, Aug 5 2012

Labels: -Pri-0 -Area-Undefined Pri-2 Area-WebKit SecSeverity-Low Mstone-22 SecImpacts-Stable SecImpacts-Beta OS-All
Status: FixUnreleased

No one seemed to have a firm opinion on security impact. I don't see anything obvious, but will flag it low out of an abundance of caution.

Comment 9 by, Aug 8 2012

Still crashes -- collectChildrenAndRemoveFromOldParent can fire a mutation event.
866 bytes View Download

Comment 12 by, Aug 10 2012

Labels: Merge-Approved

Comment 13 by, Aug 10 2012

@morrita @rniwa: thanks for the two fixes! Now that you understand it further, did you have any thoughts on whether this tree inconsistency could lead to any bad conditions such as a use-after-free? I want to make sure we reward pawlkt under our reward program if that were the case.

Comment 14 by, Aug 10 2012

These bad topologies tend to result in script-contextless execution of scripts (i.e. cross origin bug).

Comment 15 by, Aug 16 2012

Labels: reward-topanel

Comment 16 by, Aug 20 2012

Labels: -reward-topanel reward-500 reward-unpaid
@pawlkt: we don't know of any bad impact from this bug / situation, but we'd like to award you a $500 Chromium Security Reward out of an abundance of caution.

Comment 17 by, Aug 20 2012


Comment 18 by, Aug 25 2012

Labels: -Restrict-View-SecurityTeam -Merge-Approved Restrict-View-SecurityNotify Merge-Merged
M22: -> (older CL already in M22)

Comment 19 by, Sep 19 2012

Labels: -reward-unpaid

Comment 20 by, Dec 20 2012

Status: Fixed

Comment 21 by, Mar 10 2013

Project Member
Labels: -Type-Security -Area-WebKit -SecSeverity-Low -Mstone-22 -SecImpacts-Stable -SecImpacts-Beta Cr-Content Security-Severity-Low Security-Impact-Stable Security-Impact-Beta M-22 Type-Bug-Security

Comment 22 by, Mar 21 2013

Labels: -Restrict-View-SecurityNotify

Comment 23 by, Mar 21 2013

Project Member
Labels: -Security-Severity-Low Security_Severity-Low

Comment 24 by, Mar 21 2013

Project Member
Labels: -Security-Impact-Stable Security_Impact-Stable

Comment 25 by, Mar 21 2013

Project Member
Labels: -Security-Impact-Beta Security_Impact-Beta

Comment 26 by, Apr 6 2013

Project Member
Labels: -Cr-Content Cr-Blink

Comment 27 by, Jun 14 2016

Project Member
Labels: -security_impact-beta

Comment 28 by, Oct 1 2016

Project Member
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit - Your friendly Sheriffbot

Comment 29 by, Oct 2 2016

Project Member
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit - Your friendly Sheriffbot

Comment 30 by, Oct 2 2016

Labels: allpublic

Sign in to add a comment