Frame element doesn't always unload its child frame.

https://bugs.webkit.org/show_bug.cgi?id=94717

Patch by Sergey Glazunov <serg.glazunov@gmail.com> on 2012-09-04
Reviewed by Hajime Morita.

Source/WebCore:

It's possible for a frame element that has been removed from the document
to retain an active child frame. This inconsistent state may become a source
of security vulnerabilities.

The patch adds a global HashSet to store the nodes currently processed by
ChildFrameDisconnector. Insertion into these nodes' subtrees is not allowed until
the processing is complete.

Also, the ChildFrameDisconnector call in removeChild(ren) is now immediately
followed by the actual removal.

Test: fast/frames/out-of-document-iframe-has-child-frame.html

* dom/ContainerNode.cpp:
(WebCore::willRemoveChildren): Move the ChildFrameDisconnector call out of a loop.
(WebCore::ContainerNode::removeChild): Rearrange some event firing code.
(WebCore::ContainerNode::removeChildren): Ditto.
* dom/ContainerNodeAlgorithms.cpp:
(WebCore::ChildFrameDisconnector::collectDescendant): Pass a new parameter to collectDescendant(Node*).
* dom/ContainerNodeAlgorithms.h:
(WebCore::ChildFrameDisconnector::ChildFrameDisconnector):
(ChildFrameDisconnector): Maintain a list of nodes that have an active ChildFrameDisconnector.
(WebCore::ChildFrameDisconnector::~ChildFrameDisconnector):
(WebCore::ChildFrameDisconnector::rootNodes):
(WebCore::ChildFrameDisconnector::collectDescendant): Add ShouldIncludeRoot parameter.
(WebCore::ChildFrameDisconnector::nodeHasDisconnector):
(WebCore):
* dom/Node.cpp:
(WebCore::checkAcceptChild): Reject a parent node if it or one of its parents has an active ChildFrameDisconnector.
* html/HTMLFrameElementBase.cpp:
(WebCore::HTMLFrameElementBase::didNotifySubtreeInsertions): Check if an element is still in the document.

LayoutTests:

* fast/frames/out-of-document-iframe-has-child-frame-expected.txt: Added.
* fast/frames/out-of-document-iframe-has-child-frame.html: Added.
* fast/innerHTML/innerHTML-iframe-expected.txt:
* platform/chromium/fast/frames/out-of-document-iframe-has-child-frame-expected.txt: Added.
* platform/chromium/fast/innerHTML: Added.
* platform/chromium/fast/innerHTML/innerHTML-iframe-expected.txt: Added.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@127534 268f45cc-cd09-0410-ab3c-d52691b4dbfc
parent db08652d
2012-09-04 Sergey Glazunov <serg.glazunov@gmail.com>
Frame element doesn't always unload its child frame.
https://bugs.webkit.org/show_bug.cgi?id=94717
Reviewed by Hajime Morita.
* fast/frames/out-of-document-iframe-has-child-frame-expected.txt: Added.
* fast/frames/out-of-document-iframe-has-child-frame.html: Added.
* fast/innerHTML/innerHTML-iframe-expected.txt:
* platform/chromium/fast/frames/out-of-document-iframe-has-child-frame-expected.txt: Added.
* platform/chromium/fast/innerHTML: Added.
* platform/chromium/fast/innerHTML/innerHTML-iframe-expected.txt: Added.
2012-09-04 Kenichi Ishibashi <bashi@chromium.org>
[Chromium] Unreviewed test expectation update after r127457
CONSOLE MESSAGE: line 19: NO_MODIFICATION_ALLOWED_ERR: DOM Exception 7: An attempt was made to modify an object where modifications are not allowed.
This tests that several ways of making an iframe that isn't inserted into a document tree but has a child frame will fail.
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
PASS targetFrame1.contentWindow == undefined is true
PASS targetFrame2.contentWindow == undefined is true
PASS targetFrame3.contentWindow == undefined is true
PASS successfullyParsed is true
TEST COMPLETE
<html>
<head>
<script src="../js/resources/js-test-pre.js"></script>
</head>
<body>
<div id="main"/>
<script>
description("This tests that several ways of making an iframe that isn't inserted into a document tree"
+ " but has a child frame will fail.");
main = document.getElementById("main");
try {
container = main.appendChild(document.createElement("div"));
helperFrame = container.appendChild(document.createElement("iframe"));
targetFrame1 = document.createElement("iframe");
helperFrame.contentWindow.onunload = function() {
container.insertBefore(targetFrame1, helperFrame);
}
main.removeChild(container);
} catch (e) { }
shouldBeTrue("targetFrame1.contentWindow == undefined");
try {
container = main.appendChild(document.createElement("div"));
helperElement = container.appendChild(document.createElement("input"));
helperElement.focus();
targetFrame2 = document.createElement("iframe");
helperElement.onblur = function() {
container.appendChild(targetFrame2);
}
main.removeChild(container);
} catch (e) { }
shouldBeTrue("targetFrame2.contentWindow == undefined");
try {
container = document.createElement("div");
targetFrame3 = container.appendChild(document.createElement("iframe"));
helperFrame = targetFrame3.appendChild(document.createElement("iframe"));
helperFrame.src = "javascript:top.container.removeChild(top.targetFrame3)";
document.body.appendChild(container);
} catch (e) { }
shouldBeTrue("targetFrame3.contentWindow == undefined");
isSuccessfullyParsed();
</script>
</body>
</html>
CONSOLE MESSAGE: line 12: NO_MODIFICATION_ALLOWED_ERR: DOM Exception 7: An attempt was made to modify an object where modifications are not allowed.
PASS: body and iframe cleared without crashing.
CONSOLE MESSAGE: line 19: Uncaught Error: NO_MODIFICATION_ALLOWED_ERR: DOM Exception 7
This tests that several ways of making an iframe that isn't inserted into a document tree but has a child frame will fail.
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
PASS targetFrame1.contentWindow == undefined is true
PASS targetFrame2.contentWindow == undefined is true
PASS targetFrame3.contentWindow == undefined is true
PASS successfullyParsed is true
TEST COMPLETE
2012-09-04 Sergey Glazunov <serg.glazunov@gmail.com>
Frame element doesn't always unload its child frame.
https://bugs.webkit.org/show_bug.cgi?id=94717
Reviewed by Hajime Morita.
It's possible for a frame element that has been removed from the document
to retain an active child frame. This inconsistent state may become a source
of security vulnerabilities.
The patch adds a global HashSet to store the nodes currently processed by
ChildFrameDisconnector. Insertion into these nodes' subtrees is not allowed until
the processing is complete.
Also, the ChildFrameDisconnector call in removeChild(ren) is now immediately
followed by the actual removal.
Test: fast/frames/out-of-document-iframe-has-child-frame.html
* dom/ContainerNode.cpp:
(WebCore::willRemoveChildren): Move the ChildFrameDisconnector call out of a loop.
(WebCore::ContainerNode::removeChild): Rearrange some event firing code.
(WebCore::ContainerNode::removeChildren): Ditto.
* dom/ContainerNodeAlgorithms.cpp:
(WebCore::ChildFrameDisconnector::collectDescendant): Pass a new parameter to collectDescendant(Node*).
* dom/ContainerNodeAlgorithms.h:
(WebCore::ChildFrameDisconnector::ChildFrameDisconnector):
(ChildFrameDisconnector): Maintain a list of nodes that have an active ChildFrameDisconnector.
(WebCore::ChildFrameDisconnector::~ChildFrameDisconnector):
(WebCore::ChildFrameDisconnector::rootNodes):
(WebCore::ChildFrameDisconnector::collectDescendant): Add ShouldIncludeRoot parameter.
(WebCore::ChildFrameDisconnector::nodeHasDisconnector):
(WebCore):
* dom/Node.cpp:
(WebCore::checkAcceptChild): Reject a parent node if it or one of its parents has an active ChildFrameDisconnector.
* html/HTMLFrameElementBase.cpp:
(WebCore::HTMLFrameElementBase::didNotifySubtreeInsertions): Check if an element is still in the document.
2012-09-03 Sam Weinig <sam@webkit.org>
Part 1 of removing PlatformString.h, move remaining functions to new homes
......@@ -363,8 +363,9 @@ static void willRemoveChildren(ContainerNode* container)
// fire removed from document mutation events.
dispatchChildRemovalEvents(child);
ChildFrameDisconnector(child).disconnect();
}
ChildFrameDisconnector(container, ChildFrameDisconnector::DoNotIncludeRoot).disconnect();
}
void ContainerNode::disconnectDescendantFrames()
......@@ -395,13 +396,6 @@ bool ContainerNode::removeChild(Node* oldChild, ExceptionCode& ec)
}
RefPtr<Node> child = oldChild;
willRemoveChild(child.get());
// Mutation events might have moved this child into a different parent.
if (child->parentNode() != this) {
ec = NOT_FOUND_ERR;
return false;
}
document()->removeFocusedNodeOfSubtree(child.get());
......@@ -416,6 +410,14 @@ bool ContainerNode::removeChild(Node* oldChild, ExceptionCode& ec)
return false;
}
willRemoveChild(child.get());
// Mutation events might have moved this child into a different parent.
if (child->parentNode() != this) {
ec = NOT_FOUND_ERR;
return false;
}
Node* prev = child->previousSibling();
Node* next = child->nextSibling();
removeBetween(prev, next, child.get());
......@@ -481,10 +483,6 @@ void ContainerNode::removeChildren()
// The container node can be removed from event handlers.
RefPtr<ContainerNode> protect(this);
// Do any prep work needed before actually starting to detach
// and remove... e.g. stop loading frames, fire unload events.
willRemoveChildren(protect.get());
// exclude this node when looking for removed focusedNode since only children will be removed
document()->removeFocusedNodeOfSubtree(this, true);
......@@ -492,6 +490,10 @@ void ContainerNode::removeChildren()
document()->removeFullScreenElementOfSubtree(this, true);
#endif
// Do any prep work needed before actually starting to detach
// and remove... e.g. stop loading frames, fire unload events.
willRemoveChildren(protect.get());
forbidEventDispatch();
Vector<RefPtr<Node>, 10> removedChildren;
removedChildren.reserveInitialCapacity(childNodeCount());
......
......@@ -108,7 +108,7 @@ void ChildNodeRemovalNotifier::notifyDescendantRemovedFromTree(ContainerNode* no
void ChildFrameDisconnector::collectDescendant(ElementShadow* shadow)
{
for (ShadowRoot* root = shadow->youngestShadowRoot(); root; root = root->olderShadowRoot())
collectDescendant(root);
collectDescendant(root, IncludeRoot);
}
void ChildFrameDisconnector::Target::disconnect()
......
......@@ -268,13 +268,37 @@ inline void ChildNodeRemovalNotifier::notify(Node* node)
class ChildFrameDisconnector {
public:
explicit ChildFrameDisconnector(Node* root);
enum ShouldIncludeRoot {
DoNotIncludeRoot,
IncludeRoot
};
explicit ChildFrameDisconnector(Node* root, ShouldIncludeRoot shouldIncludeRoot = IncludeRoot)
: m_root(root)
{
collectDescendant(m_root, shouldIncludeRoot);
rootNodes().add(m_root);
}
~ChildFrameDisconnector()
{
rootNodes().remove(m_root);
}
void disconnect();
static bool nodeHasDisconnector(Node*);
private:
void collectDescendant(Node* root);
void collectDescendant(Node* root, ShouldIncludeRoot);
void collectDescendant(ElementShadow*);
static HashSet<Node*>& rootNodes()
{
DEFINE_STATIC_LOCAL(HashSet<Node*>, nodes, ());
return nodes;
}
class Target {
public:
Target(HTMLFrameOwnerElement* element)
......@@ -292,16 +316,13 @@ private:
};
Vector<Target, 10> m_list;
Node* m_root;
};
inline ChildFrameDisconnector::ChildFrameDisconnector(Node* root)
inline void ChildFrameDisconnector::collectDescendant(Node* root, ShouldIncludeRoot shouldIncludeRoot)
{
collectDescendant(root);
}
inline void ChildFrameDisconnector::collectDescendant(Node* root)
{
for (Node* node = root; node; node = node->traverseNextNode(root)) {
for (Node* node = shouldIncludeRoot == IncludeRoot ? root : root->firstChild(); node;
node = node->traverseNextNode(root)) {
if (!node->isElementNode())
continue;
Element* element = toElement(node);
......@@ -322,6 +343,20 @@ inline void ChildFrameDisconnector::disconnect()
}
}
inline bool ChildFrameDisconnector::nodeHasDisconnector(Node* node)
{
HashSet<Node*>& nodes = rootNodes();
if (nodes.isEmpty())
return false;
for (; node; node = node->parentNode())
if (nodes.contains(node))
return true;
return false;
}
} // namespace WebCore
#endif // ContainerNodeAlgorithms_h
......@@ -40,6 +40,7 @@
#include "CSSStyleSheet.h"
#include "ChildNodeList.h"
#include "ClassNodeList.h"
#include "ContainerNodeAlgorithms.h"
#include "ContextMenuController.h"
#include "DOMImplementation.h"
#include "DOMSettableTokenList.h"
......@@ -1199,6 +1200,11 @@ static void checkAcceptChild(Node* newParent, Node* newChild, ExceptionCode& ec)
ec = HIERARCHY_REQUEST_ERR;
return;
}
if (newParent->inDocument() && ChildFrameDisconnector::nodeHasDisconnector(newParent)) {
ec = NO_MODIFICATION_ALLOWED_ERR;
return;
}
}
void Node::checkReplaceChild(Node* newChild, Node* oldChild, ExceptionCode& ec)
......
......@@ -161,9 +161,10 @@ Node::InsertionNotificationRequest HTMLFrameElementBase::insertedInto(ContainerN
return InsertionDone;
}
void HTMLFrameElementBase::didNotifySubtreeInsertions(ContainerNode* insertionPoint)
void HTMLFrameElementBase::didNotifySubtreeInsertions(ContainerNode*)
{
ASSERT_UNUSED(insertionPoint, insertionPoint->inDocument());
if (!inDocument())
return;
// DocumentFragments don't kick of any loads.
if (!document()->frame())
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment