[6] Data race in JSStyleSheet::visitAdditionalChildren during GC leading to use-after-free

Severity: Medium | Component: WebCore CSSOM ↔ JSC GC boundary | b1a42ef

Rated Medium because the diff serializes a GC-thread read of an owner pointer against main-thread frees of the backing WeakPtrImpl — a genuine cross-thread UAF — but escalation is bounded by the attacker's ability to time DOM teardown against a GC cycle, which the change does not demonstrate is reliable.

JSStyleSheet::visitAdditionalChildren is called by the JSC garbage collector on a GC thread. It calls addWebCoreOpaqueRoot(visitor, wrapped()) with root(StyleSheet*), which reads styleSheet->ownerNode() and styleSheet->ownerRule(). Both were WeakPtr members whose .get() returns a raw pointer, and the backing WeakPtrImpl can be freed by the main thread between the read and the subsequent dereference, causing a heap use-after-free. The same root(StyleSheet*) is the convergence point for JSCSSRule and JSCSSStyleDeclaration visitors, so all three GC visitor paths are affected. The fix adds a pure virtual opaqueRootForGCThread(), guards it with a new per-stylesheet lock, and changes the owner members from WeakPtr to CheckedPtr. A secondary GC-correctness fix keeps an @import child's parent stylesheet wrapper alive.

Source/WebCore/bindings/js/JSStyleSheetCustom.h

 inline WebCoreOpaqueRoot root(StyleSheet* styleSheet)
 {
-    if (SUPPRESS_UNCOUNTED_LOCAL CSSImportRule* ownerRule = styleSheet->ownerRule())
-        return root(ownerRule);
-    if (SUPPRESS_UNCOUNTED_LOCAL Node* ownerNode = styleSheet->ownerNode())
-        return root(ownerNode);
-    return WebCoreOpaqueRoot { styleSheet };
+    return styleSheet->opaqueRootForGCThread();
 }

Source/WebCore/css/CSSStyleSheet.cpp

 void CSSStyleSheet::clearOwnerNode()
 {
+    Locker locker { m_opaqueRootLockForGC };
     m_ownerNode = nullptr;
 }
+
+WebCoreOpaqueRoot CSSStyleSheet::opaqueRootForGCThread()
+{
+    Locker locker { m_opaqueRootLockForGC };
+    if (m_ownerNode)
+        return root(m_ownerNode.get());
+    if (SUPPRESS_UNCOUNTED_LOCAL SUPPRESS_UNCHECKED_LOCAL CSSImportRule* ownerRule = m_ownerRule.get()) {
+        if (auto* parentSheet = ownerRule->parentStyleSheet())
+            return parentSheet->opaqueRootForGCThread();
+    }
+    return WebCoreOpaqueRoot { this };
+}
+
+void CSSStyleSheet::clearOwnerRule()
+{
+    Locker locker { m_opaqueRootLockForGC };
+    m_ownerRule = nullptr;
+}

Source/WebCore/css/CSSStyleSheet.h

-    WeakPtr<Node, WeakPtrImplWithEventTargetData> m_ownerNode;
-    WeakPtr<CSSImportRule> m_ownerRule;
+    mutable Lock m_opaqueRootLockForGC;
+    CheckedPtr<Node> m_ownerNode;
+    CheckedPtr<CSSImportRule> m_ownerRule;

LayoutTests/fast/dom/StyleSheet/gc-import-rule-stylesheet.html

+window.childSheet = parentSheet.cssRules[0].styleSheet;
+parentSheet = null; style.remove(); style = null;
+gc();
+shouldBeEqualToString('childSheet.parentStyleSheet.foo', 'bar');

Patch Details

The patch replaces the inline root(StyleSheet*) chain — which walked styleSheet->ownerRule() then styleSheet->ownerNode() on the GC thread — with a single virtual dispatch to a new pure-virtual StyleSheet::opaqueRootForGCThread(), implemented in both CSSStyleSheet and XSLStyleSheet. Each implementation takes a new per-stylesheet lock (m_opaqueRootLockForGC) before reading the owner pointers, and the mutators (clearOwnerNode(), clearOwnerRule()) now also take that lock. The member types m_ownerNode/m_ownerRule change from WeakPtr to CheckedPtr<Node>/CheckedPtr<CSSImportRule>, and CSSImportRule gains CanMakeCheckedPtr. A secondary GC-correctness fix: for an @import child (which has m_ownerRule but no m_ownerNode), opaqueRootForGCThread() recurses into ownerRule->parentStyleSheet()->opaqueRootForGCThread() so the parent wrapper is grouped into the same opaque root and kept alive.

Unsynchronized cross-thread read of a WeakPtr-indirected owner pointer from the GC thread while the main thread can free the backing WeakPtrImpl.

Background

WebKit's concurrent GC marks live objects on dedicated GC threads while the main thread continues executing. DOM wrappers implement visitAdditionalChildren, called by the collector, to report edges to other objects; WebCoreOpaqueRoot/addWebCoreOpaqueRoot groups DOM objects under a common opaque root so a whole subtree is kept alive if any part is reachable. root(StyleSheet*) computes that root by walking to the stylesheet's owner. WeakPtr is a non-owning smart pointer whose .get() loads the current pointee address via a shared, heap-allocated WeakPtrImpl control block; CheckedPtr instead stores the pointer inline and, with CanMakeCheckedPtr, maintains an assertion counter on the pointee. Lock/Locker provide mutual exclusion. A CSSStyleSheet created for an @import rule has an m_ownerRule (the CSSImportRule) and no m_ownerNode; it is reachable from JS via the child sheet's parentStyleSheet property.

Analysis

This is a cross-thread data race leading to a heap use-after-free. Before the fix, JSStyleSheet::visitAdditionalChildren (and the convergent visitors for JSCSSRule and JSCSSStyleDeclaration) ran root(StyleSheet*) on a GC thread, calling styleSheet->ownerNode() / ownerRule(). These returned raw pointers from WeakPtr::get(), which dereferences a shared WeakPtrImpl control block to load the pointee address. The WeakPtrImpl backing store for the owner Node/CSSImportRule can be freed (or the weak reference cleared) by the main thread at the same instant the GC thread is loading through it, because there is no synchronization between the two threads.

The GC thread reads freed/torn WeakPtrImpl memory and may then dereference a stale pointer as a Node* or CSSImportRule*. (More precisely, a WeakPtr holds a Ref to its WeakPtrImpl, so the concrete race is on reassignment/clearing of the WeakPtr member itself; the diff shows the lock/CheckedPtr change but not the exact freeing sequence.) The fix serializes GC-thread reads and main-thread clears of the owner pointers under a per-stylesheet lock and stores the pointers as CheckedPtr directly in the stylesheet — no separately-freeable indirection — so the value is stable while the lock is held and its lifetime is asserted by the checked-pointer counter, whose owners (clearOwnerNode()/clearOwnerRule() in the element and import-rule destructors) run before the pointee is freed.

Exploitability is bounded by timing. An attacker who can time DOM/CSSOM teardown (removing a <style>/<link> owner, or destroying an @import rule) against a GC cycle could induce a UAF read of freed heap during marking, which under controlled heap conditions could be developed toward an information leak or further corruption in the renderer; that the timing is reliably winnable is a plausible but unproven claim not derivable from the diff.

This vulnerability weakens memory safety inside the WebContent process by allowing a GC marking thread to race main-thread CSSOM mutation. The security model assumes objects touched during concurrent marking are either immutable or synchronized; here the owner-pointer read on the GC thread had no synchronization against the main thread freeing the WeakPtrImpl, so the invariant that a pointer read during marking refers to live memory was violable.

This is a recurring hazard class at the WebCore↔JSC concurrent-GC boundary: visitAdditionalChildren/opaque-root helpers run on GC threads and must not touch main-thread-mutable state without synchronization. WeakPtr is especially dangerous because it looks like a plain pointer read but actually dereferences a separately-heap-allocated WeakPtrImpl that the owning thread can free — so the race is a genuine UAF, not just a torn scalar. The move to CheckedPtr plus an explicit lock is the correct shape: keep the pointer value inline and serialize the read/clear.

Note: The specific visitor entry points, the precise WeakPtrImpl-freeing sequence, the timing-based exploitability, and the asserted CheckedPtr lifetime invariant across all owner destructors are inferred from the commit message and code patterns rather than shown in the diff. The lock/CheckedPtr change and the opaque-root recursion are directly supported by the patch.

Audit directions

GC-thread code reading main-thread-mutable WeakPtr members without synchronization. Audit all visitAdditionalChildren, visitOutputConstraints, and root(...)/addWebCoreOpaqueRoot helpers in Source/WebCore/bindings/js for any call chain that invokes WeakPtr::get() on a member the main thread can clear. Start by grepping bindings/js for visitAdditionalChildren and cross-referencing each touched member's declared type for WeakPtr.
WeakPtr used as a lock-free cross-thread pointer. Grep WebCore for WeakPtr< members dereferenced from GC/marking or compositing/scrolling threads and verify each is either immutable during marking or guarded by a lock; treat .get() on such members as a concurrent load of the WeakPtrImpl control block, not a plain pointer read.
Verify the opaque-root grouping recursion. Examine whether cyclic or deeply nested @import chains could deadlock (same per-sheet lock reacquired) or overflow the stack during marking via ownerRule->parentStyleSheet()->opaqueRootForGCThread().
Confirm the CheckedPtr lifetime invariant. Check that HTMLStyleElement, HTMLLinkElement, SVGStyleElement, ProcessingInstruction all call clearOwnerNode() before the Node base destructor, and that CSSImportRule/StyleRuleImport call clearOwnerRule() before freeing, so no live CheckedPtr<Node>/CheckedPtr<CSSImportRule> outlives its pointee.