PatchGap
WebKit · WebKit › WebCore DOM

CVE-2024-54534

UAF in WebKit (WebCore DOM)
Medium CVSS 9.8 webkit UAF 🔧 Commit mapped

Overview

Medium
Severity
9.8
CVSS
No
Exploited ITW
Fixed
Fix Status
DescriptionProcessing maliciously crafted web content may lead to memory corruption
ComponentWebKit
Sub-componentWebCore DOM
Bug ClassUAF
Tracker277967
Fix commitccef7b85cc9b (WebKit/WebKit)
CWECWE-787 (Out-of-bounds write)
CVSS vectorCVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H
CISA KEVNot listed
CreditedTashita Software Security
Disclosed2024-12-11

Background

HashCountedSet
A WTF set that stores each element once along with an integer count of how many times it was added, here used to track how many handlers a Node has registered.
WeakHashCountedSet / WeakPtr
A counted set whose keys are weak references that automatically become null when the referenced object is destroyed, preventing stale-pointer dereference.
EventTargetSet
The Document-level structure (m_touchEventTargets, m_wheelEventTargets) recording every Node that currently has a touch or wheel event handler.
isEmptyIgnoringNullReferences / computeSize
Weak-container accessors that account for entries whose weak reference has already been nulled, so callers see only live members rather than a raw slot count.
Use-after-free
A memory-safety bug where a pointer is dereferenced after the object it referenced has been freed, allowing reads/writes of reallocated attacker-controlled memory.

Root Cause Analysis

The patch replaces the type of EventTargetSet in Document.h from HashCountedSet<Node*> (a counted set of raw, non-owning Node pointers) with WeakHashCountedSet<Node, WeakPtrImplWithEventTargetData> (a counted set of weak references). Document keeps two of these sets, m_touchEventTargets and m_wheelEventTargets, which record every Node in the document that has registered a touch or wheel event handler. The invariant these sets must uphold is that every entry corresponds to a live Node: the set holds only non-owning references, so an entry must be removed before or exactly when the referenced Node is destroyed. With raw Node* keys that invariant was enforced only by explicit bookkeeping calls (didRemoveEventTargetNode / removeHandlerFromSet), and any path where a handler-bearing Node was torn down without correctly pruning these sets left a dangling raw pointer. Subsequent operations that dereference the keys — most clearly Document::absoluteRegionForEventTargets, which before the patch did RefPtr node = keyValuePair.key and called absoluteEventRegionForNode(*node), and the iteration loops in wheelEventHandlerCount/touchEventHandlerCount — would then read or ref-count freed memory, a use-after-free.

The fix makes the container itself hold weak references: a destroyed Node’s slot is automatically nulled, so stale entries can never be dereferenced as live objects. Consequently the API surface changes throughout Document.cpp: add/remove/removeAll/contains now take Node& instead of Node*, size() (which counted possibly-dead slots) is replaced by computeSize() and isEmpty() by isEmptyIgnoringNullReferences() so callers ask about live entries only, and absoluteRegionForEventTargets iterates by value taking a Ref node = keyValuePair.key directly (the weak set never yields a dead key). A new removeAll(const ValueType&)/removeAll(iterator) pair was added to WeakHashCountedSet because that method did not previously exist on the weak container.

The fix restores the live-reference invariant structurally rather than relying on every teardown path to remember to prune, which is why it is fundamentally a memory-safety (UAF) fix despite the LogicError label.

Key insight
Storing DOM Nodes as raw non-owning pointers in the touch/wheel EventTargetSet made memory safety depend on every teardown path perfectly pruning the set; the fix removes that fragile obligation by making the container hold auto-nulling weak references, turning a latent use-after-free into a structurally impossible one.

Attack Path

  1. Register handlers From attacker-controlled web content, add touch and/or wheel event listeners to one or more DOM nodes so those nodes are inserted into Document::m_touchEventTargets / m_wheelEventTargets.
  2. Trigger a teardown path that skips pruning Manipulate the DOM (node removal, frame/document detachment, or a reentrant teardown during willBeRemovedFromFrame) so a handler-bearing Node is destroyed without its raw-pointer entry being correctly removed from the counted set, leaving a dangling Node* key.
  3. Reoccupy the freed slot Use standard heap grooming (allocate objects of the same size class as the freed Node) so the dangling pointer now points at attacker-influenced memory.
  4. Force a dereference of the stale key Cause the document to walk the target set — e.g. a hit-test / scrolling-region computation calling absoluteRegionForEventTargets, or wheelEventHandlerCount()/touchEventHandlerCount(), or hasTouchEventHandlers() — which reads and, in the pre-patch absoluteRegionForEventTargets, ref-counts the freed Node as if live.
  5. Escalate Convert the resulting use-after-free read/write of a Node object into a stronger primitive (type-confused vtable/refcount manipulation) toward code execution in the WebContent process; this escalation is standard exploitation background, not shown by the diff.

Impact Assessment

The primitive is a use-after-free on a Node object reachable from the WebContent (renderer) process purely through DOM manipulation and event-handler registration, so it is remotely reachable from a malicious web page. Pre-patch, absoluteRegionForEventTargets even performed a ref-count operation (RefPtr node = key) on the stale entry, giving both a dereference and a refcount write on freed memory, which is a comparatively strong UAF that heap grooming can steer toward type confusion and, with additional work, arbitrary code execution. Apple describes the observable effect as memory corruption. The bug is confined to the sandboxed WebContent process; achieving anything beyond a renderer compromise requires a separate sandbox-escape chain.

Changed Functions

FunctionChangeNotes
switch
Source/WebCore/dom/Document.cpp
 modified
if
Source/WebCore/dom/Document.cpp
 modified
for
Source/WebCore/dom/Document.cpp
 modified

Files Changed

  • Source/WTF/wtf/WeakHashCountedSet.h
  • Source/WebCore/dom/Document.cpp
  • Source/WebCore/dom/Document.h

Audit Directions

  • Other raw-pointer counted/hash sets of DOM nodes in Document
    Grep Document.h/.cpp and related DOM classes for HashCountedSet<Node*>, HashSet<Node*>, Vector<Node*> and similar non-owning containers keyed on Node*/Element*/EventTarget*; each is a candidate for the same dangling-key UAF and should either be weak or provably pruned on teardown.
  • size()/isEmpty() used as liveness on weak or soon-to-be-weak containers
    Search for callers using ->size() or ->isEmpty() on event-target and observer sets where computeSize()/isEmptyIgnoringNullReferences() would be semantically required; a raw size() over a container with dead slots signals stale-entry logic bugs.
  • Iterators taking a reference to a key then dereferencing
    Look for loops of the form RefPtr node = pair.key; … node or auto& over node containers without a liveness guarantee (grep absoluteEventRegionForNode-style callers and other absoluteForEventTargets helpers), which is the exact pattern that dereferenced the freed key here.
  • removeAll / prune-on-destroy obligations across WebCore observer registries
    Audit registries where node destruction must call a manual removeAll/unregister (touch, wheel, IntersectionObserver, mutation, resize observers); enumerate teardown paths (detach, willBeRemovedFromFrame, reentrant document destruction) and confirm every one prunes, or migrate the container to a weak type.

Patch

view commit
diff --git a/Source/WTF/wtf/WeakHashCountedSet.h b/Source/WTF/wtf/WeakHashCountedSet.h
index a4af95bf87b5..6aa449841455 100644
--- a/Source/WTF/wtf/WeakHashCountedSet.h
+++ b/Source/WTF/wtf/WeakHashCountedSet.h
@@ -64,6 +64,11 @@ public:
     bool remove(const ValueType&);
     bool remove(iterator);
 
+    // Removes the value, regardless of its count.
+    // Returns true if a value was removed.
+    bool removeAll(const ValueType&);
+    bool removeAll(iterator);
+
     // Clears the whole set.
     void clear() { m_impl.clear(); }
 
@@ -111,6 +116,22 @@ inline bool WeakHashCountedSet<Value, WeakPtrImpl>::remove(iterator it)
     return true;
 }
 
+template<typename Value, typename WeakPtrImpl>
+inline bool WeakHashCountedSet<Value, WeakPtrImpl>::removeAll(const ValueType& value)
+{
+    return removeAll(find(value));
+}
+
+template<typename Value, typename WeakPtrImpl>
+inline bool WeakHashCountedSet<Value, WeakPtrImpl>::removeAll(iterator it)
+{
+    if (it == end())
+        return false;
+
+    m_impl.remove(it);
+    return true;
+}
+
 template<typename Value, typename WeakMapImpl>
 size_t containerSize(const WeakHashCountedSet<Value, WeakMapImpl>& container) { return container.computeSize(); }
 
diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp
index 81729be58ab5..4e901c60f3d1 100644
--- a/Source/WebCore/dom/Document.cpp
+++ b/Source/WebCore/dom/Document.cpp
@@ -3232,11 +3232,11 @@ void Document::willBeRemovedFromFrame()
     commonTeardown();
 
 #if ENABLE(TOUCH_EVENTS)
-    if (m_touchEventTargets && m_touchEventTargets->size() && parentDocument())
+    if (m_touchEventTargets && m_touchEventTargets->computeSize() && parentDocument())
         protectedParentDocument()->didRemoveEventTargetNode(*this);
 #endif
 
-    if (m_wheelEventTargets && m_wheelEventTargets->size() && parentDocument())
+    if (m_wheelEventTargets && m_wheelEventTargets->computeSize() && parentDocument())
         protectedParentDocument()->didRemoveEventTargetNode(*this);
 
     if (RefPtr mediaQueryMatcher = m_mediaQueryMatcher)
@@ -8363,7 +8363,7 @@ void Document::wheelEventHandlersChanged(Node* node)
     UNUSED_PARAM(node);
 #endif
 
-    bool haveHandlers = m_wheelEventTargets && !m_wheelEventTargets->isEmpty();
+    bool haveHandlers = m_wheelEventTargets && !m_wheelEventTargets->isEmptyIgnoringNullReferences();
     page->chrome().client().wheelEventHandlersChanged(haveHandlers);
 }
 
@@ -8372,7 +8372,7 @@ void Document::didAddWheelEventHandler(Node& node)
     if (!m_wheelEventTargets)
         m_wheelEventTargets = makeUnique<EventTargetSet>();
 
-    m_wheelEventTargets->add(&node);
+    m_wheelEventTargets->add(node);
     wheelEventHandlersChanged(&node);
 
     if (RefPtr frame = this->frame())
@@ -8383,9 +8383,9 @@ static bool removeHandlerFromSet(EventTargetSet& handlerSet, Node& node, EventHa
 {
     switch (removal) {
     case EventHandlerRemoval::One:
-        return handlerSet.remove(&node);
+        return handlerSet.remove(node);
     case EventHandlerRemoval::All:
-        return handlerSet.removeAll(&node);
+        return handlerSet.removeAll(node);
     }
     return false;
 }
@@ -8410,7 +8410,7 @@ unsigned Document::wheelEventHandlerCount() const
         return 0;
 
     unsigned count = 0;
-    for (auto& handler : *m_wheelEventTargets)
+    for (auto handler : *m_wheelEventTargets)
         count += handler.value;
 
     return count;
@@ -8422,7 +8422,7 @@ void Document::didAddTouchEventHandler(Node& handler)
     if (!m_touchEventTargets)
         m_touchEventTargets = makeUnique<EventTargetSet>();
 
-    m_touchEventTargets->add(&handler);
+    m_touchEventTargets->add(handler);
 
     if (RefPtr parent = parentDocument()) {
         parent->didAddTouchEventHandler(*this);
@@ -8453,15 +8453,15 @@ void Document::didRemoveEventTargetNode(Node& handler)
 {
 #if ENABLE(TOUCH_EVENTS)
     if (m_touchEventTargets) {
-        m_touchEventTargets->removeAll(&handler);
-        if ((&handler == this || m_touchEventTargets->isEmpty()) && parentDocument())
+        m_touchEventTargets->removeAll(handler);
+        if ((&handler == this || m_touchEventTargets->isEmptyIgnoringNullReferences()) && parentDocument())
             protectedParentDocument()->didRemoveEventTargetNode(*this);
     }
 #endif
 
     if (m_wheelEventTargets) {
-        m_wheelEventTargets->removeAll(&handler);
-        if ((&handler == this || m_wheelEventTargets->isEmpty()) && parentDocument())
+        m_wheelEventTargets->removeAll(handler);
+        if ((&handler == this || m_wheelEventTargets->isEmptyIgnoringNullReferences()) && parentDocument())
             protectedParentDocument()->didRemoveEventTargetNode(*this);
     }
 }
@@ -8473,7 +8473,7 @@ unsigned Document::touchEventHandlerCount() const
         return 0;
 
     unsigned count = 0;
-    for (auto& handler : *m_touchEventTargets)
+    for (auto handler : *m_touchEventTargets)
         count += handler.value;
 
     return count;
@@ -8545,12 +8545,11 @@ Document::RegionFixedPair Document::absoluteRegionForEventTargets(const EventTar
     Region targetRegion;
     bool insideFixedPosition = false;
 
-    for (auto& keyValuePair : *targets) {
-        if (RefPtr node = keyValuePair.key) {
-            auto targetRegionFixedPair = absoluteEventRegionForNode(*node);
-            targetRegion.unite(targetRegionFixedPair.first);
-            insideFixedPosition |= targetRegionFixedPair.second;
-        }
+    for (auto keyValuePair : *targets) {
+        Ref node = keyValuePair.key;
+        auto targetRegionFixedPair = absoluteEventRegionForNode(node);
+        targetRegion.unite(targetRegionFixedPair.first);
+        insideFixedPosition |= targetRegionFixedPair.second;
     }
 
     return RegionFixedPair(targetRegion, insideFixedPosition);
diff --git a/Source/WebCore/dom/Document.h b/Source/WebCore/dom/Document.h
index d4874fbbf3a3..3fe0bb244379 100644
--- a/Source/WebCore/dom/Document.h
+++ b/Source/WebCore/dom/Document.h
@@ -56,7 +56,6 @@
 #include <wtf/Deque.h>
 #include <wtf/FixedVector.h>
 #include <wtf/Forward.h>
-#include <wtf/HashCountedSet.h>
 #include <wtf/HashSet.h>
 #include <wtf/Logger.h>
 #include <wtf/ObjectIdentifier.h>
@@ -64,6 +63,7 @@
 #include <wtf/RobinHoodHashMap.h>
 #include <wtf/TriState.h>
 #include <wtf/UniqueRef.h>
+#include <wtf/WeakHashCountedSet.h>
 #include <wtf/WeakHashMap.h>
 #include <wtf/WeakHashSet.h>
 #include <wtf/WeakListHashSet.h>
@@ -356,7 +356,7 @@ enum class NodeListInvalidationType : uint8_t {
 const auto numNodeListInvalidationTypes = enumToUnderlyingType(NodeListInvalidationType::InvalidateOnAnyAttrChange) + 1;
 
 enum class EventHandlerRemoval : bool { One, All };
-using EventTargetSet = HashCountedSet<Node*>;
+using EventTargetSet = WeakHashCountedSet<Node, WeakPtrImplWithEventTargetData>;
 
 enum class DocumentCompatibilityMode : uint8_t {
     NoQuirksMode = 1,
@@ -1481,8 +1481,8 @@ public:
     WEBCORE_EXPORT unsigned styleRecalcCount() const;
 
 #if ENABLE(TOUCH_EVENTS)
-    bool hasTouchEventHandlers() const { return m_touchEventTargets.get() ? m_touchEventTargets->size() : false; }
-    bool touchEventTargetsContain(Node& node) const { return m_touchEventTargets ? m_touchEventTargets->contains(&node) : false; }
+    bool hasTouchEventHandlers() const { return m_touchEventTargets && m_touchEventTargets->computeSize(); }
+    bool touchEventTargetsContain(Node& node) const { return m_touchEventTargets && m_touchEventTargets->contains(node); }
 #else
     bool hasTouchEventHandlers() const { return false; }
     bool touchEventTargetsContain(Node&) const { return false; }
@@ -1516,7 +1516,7 @@ public:
 #endif
     }
 
-    bool hasWheelEventHandlers() const { return m_wheelEventTargets.get() ? m_wheelEventTargets->size() : false; }
+    bool hasWheelEventHandlers() const { return m_wheelEventTargets && m_wheelEventTargets->computeSize(); }
     const EventTargetSet* wheelEventTargets() const { return m_wheelEventTargets.get(); }
 
     using RegionFixedPair = std::pair<Region, bool>;
Loading diff…

Original Bug Report

The reporter's bug is still restricted on the tracker.
On This Page