CVE-2024-27856
Overview
Background
- insertChildrenBefore
- A DOM helper that detaches new children from their old parents and inserts them before a reference node.
- Mutation events / script re-entry
- removeChild and node insertion dispatch events and assertions that can run author script mid-operation.
- RefPtr vs raw Node*
- A RefPtr keeps the referenced node alive; a raw pointer can dangle if script drops the last reference during the operation.
Root Cause Analysis
This fixes a use-after-free in DOM child insertion when event listeners mutate the tree during the operation. ContainerNode::insertChildrenBeforeWithoutPreInsertionValidityCheck inserts a batch of new children before a reference node (nextChild). The routine first detaches each new child from its old parent (oldParent->removeChild), then inserts each before nextChild. Both phases can run author script: removeChild dispatches mutation events (the test uses DOMSubtreeModified), and executeNodeInsertionWithScriptAssertion likewise runs during insertion.
Pre-patch, nextChild was held as a RAW Node* across these script-running steps. Author code invoked from those events can move nextChild elsewhere, remove it, or otherwise drop the last reference, so the raw nextChild pointer dangles; the guard that checks nextChild->parentNode() != this still DEREFERENCES the pointer, and insertBeforeCommon(*nextChild, …) uses it — a use-after-free.
The fix promotes the reference to a RefPtr (RefPtr refChild = nextChild) and uses refChild throughout (including advancing it to child->nextSibling() when the removed child was the reference), so the reference node is kept alive across the mutation events and script callbacks.
The restored invariant is that the insertion reference node cannot be freed by author script run mid-insertion. The regression test, inside a DOMSubtreeModified handler fired by setAttribute, calls hrElement.before(hrElement) and normalize() to churn the tree during insertion.
Attack Path
- Insert children with a reference node Trigger a DOM operation that batch-inserts nodes before a reference node (nextChild) on a container.
- Run script during the mutation A mutation-event listener (e.g. DOMSubtreeModified) fires during removeChild/insertion and manipulates the tree.
- Free the reference node The listener moves/removes the reference node so its last reference is dropped while the routine still holds a raw pointer to it.
- Use-after-free The insertion dereferences the dangling reference node (parentNode() check / insertBeforeCommon), corrupting or crashing the WebContent process toward code execution.
Impact Assessment
Changed Functions
| Function | Change | Notes |
|---|---|---|
ContainerNode::insertChildrenBeforeWithoutPreInsertionValidityCheckSource/WebCore/dom/ContainerNode.cpp |
modified | Holds the reference node as a RefPtr (refChild) instead of a raw Node* so it survives mutation events and script callbacks run during removeChild/insertion; uses refChild for the parent check and insertBeforeCommon. |
Files Changed
LayoutTests/fast/dom/set-attribute-and-normalize-in-event-expected.txtLayoutTests/fast/dom/set-attribute-and-normalize-in-event.htmlSource/WebCore/dom/ContainerNode.cpp
Audit Directions
- Same file: raw nodes across scriptAudit ContainerNode insertion/removal helpers for raw Node* references (nextChild, refChild, oldParent) held across removeChild / executeNodeInsertionWithScriptAssertion / mutation events.
- Mutate-during-DOM-op patternGrep WebCore DOM for raw node pointers used after a call that dispatches events (dispatchSubtreeModified, ChildListMutationScope) without a protecting Ref/RefPtr.
Patch
diff --git a/LayoutTests/fast/dom/set-attribute-and-normalize-in-event-expected.txt b/LayoutTests/fast/dom/set-attribute-and-normalize-in-event-expected.txt
new file mode 100644
index 000000000000..74c1f9350acf
--- /dev/null
+++ b/LayoutTests/fast/dom/set-attribute-and-normalize-in-event-expected.txt
@@ -0,0 +1,3 @@
+CONSOLE MESSAGE: RangeError: Maximum call stack size exceeded.
+CONSOLE MESSAGE: RangeError: Maximum call stack size exceeded.
+
diff --git a/LayoutTests/fast/dom/set-attribute-and-normalize-in-event.html b/LayoutTests/fast/dom/set-attribute-and-normalize-in-event.html
new file mode 100644
index 000000000000..5ec7cdccafa5
--- /dev/null
+++ b/LayoutTests/fast/dom/set-attribute-and-normalize-in-event.html
@@ -0,0 +1,17 @@
+<script>
+ function runTest() {
+ if (window.testRunner)
+ window.testRunner.dumpAsText();
+
+ marqueeElement.addEventListener("DOMSubtreeModified", () => {
+ try { hrElement.before(hrElement); } catch (e) { }
+ marqueeElement.normalize();
+ });
+
+ marqueeElement.setAttribute("a", "");
+ }
+</script>
+
+<body onload=runTest()>
+ <marquee id="marqueeElement">
+ <hr id="hrElement" width="1"></hr>
diff --git a/Source/WebCore/dom/ContainerNode.cpp b/Source/WebCore/dom/ContainerNode.cpp
index 30c7e9e66ba8..d3372f8d09d1 100644
--- a/Source/WebCore/dom/ContainerNode.cpp
+++ b/Source/WebCore/dom/ContainerNode.cpp
@@ -890,10 +890,11 @@ ExceptionOr<void> ContainerNode::appendChildWithoutPreInsertionValidityCheck(Nod
ExceptionOr<void> ContainerNode::insertChildrenBeforeWithoutPreInsertionValidityCheck(NodeVector&& newChildren, Node* nextChild)
{
+ RefPtr refChild = nextChild;
for (auto& child : newChildren) {
if (RefPtr oldParent = child->parentNode()) {
- if (nextChild == child.ptr())
- nextChild = child->nextSibling();
+ if (refChild.get() == child.ptr())
+ refChild = child->nextSibling();
if (auto result = oldParent->removeChild(child); result.hasException())
return result.releaseException();
}
@@ -910,14 +911,14 @@ ExceptionOr<void> ContainerNode::insertChildrenBeforeWithoutPreInsertionValidity
ChildListMutationScope mutation(*this);
for (auto& child : newChildren) {
- if (nextChild && nextChild->parentNode() != this) // Event listeners moved nextChild elsewhere.
+ if (refChild && refChild->parentNode() != this) // Event listeners moved nextChild elsewhere.
break;
if (child->parentNode()) // Event listeners inserted this child elsewhere.
break;
- executeNodeInsertionWithScriptAssertion(*this, child.get(), nextChild, ChildChange::Source::API, ReplacedAllChildren::No, [&] {
+ executeNodeInsertionWithScriptAssertion(*this, child.get(), refChild.get(), ChildChange::Source::API, ReplacedAllChildren::No, [&] {
child->setTreeScopeRecursively(treeScope());
- if (nextChild)
- insertBeforeCommon(*nextChild, child.get());
+ if (refChild)
+ insertBeforeCommon(*refChild, child.get());
else
appendChildCommon(child);
});