[3] HTMLDialogElement: invoker `value()` reference freed by a `beforetoggle` listener

HighWebCore HTML — HTMLDialogElementUAF

The dialog handed back a return value from memory the page just freed.

7075344

High. A two-hop accessor chain hands a callee an alias into DOM attribute storage, and the callee's own cancelable pre-event gives page script a synchronous window to free it. The free-to-use window is bounded by an attacker-controlled JS callback, which makes reclaim grooming straightforward rather than opportunistic.

WebKit's string types are refcounted, and a const String& parameter is a borrow rather than an owning copy — a deliberate efficiency choice that holds right up until the callee hands control back to page script. HTML's declarative command invokers let a <button> carrying command and commandfor attributes drive another element's state machine, with the button's value attribute supplying the return value for a dialog's close command. Dialogs and popovers fire a cancelable beforetoggle event before committing an open/closed transition, so the state-change routine has a synchronous re-entrancy point in its middle, and any borrowed DOM reference it is still holding must survive that point.

The angle: a page can free an attacker-sized, attacker-populated heap allocation at a precisely chosen moment inside close() and have the engine read it back into dialog.returnValue, which script can then read.

Fix the bug in HTMLDialogElement::handleCommandInternal by storing the String value of the invoker in a local variable.

Source/WebCore/html/HTMLDialogElement.cpp

LayoutTests/fast/html/dialog-close-from-button-crash.html

Patch Details

Both command branches in HTMLDialogElement::handleCommandInternal previously passed invoker.value().string() directly as the const String& argument to close() and requestClose(). HTMLButtonElement::value() is declared in HTMLButtonElement.h as const AtomString& NODELETE value() const, and AtomString::string() hands back the wrapped String by reference, so the callee's parameter was an alias into the button's live attribute storage rather than an owned copy. The patch introduces a local String value = invoker.value().string(); in each branch — materializing a String that takes its own reference on the underlying StringImpl — and passes that local instead. No other production code changed; the rest of the commit is the regression test and its expectation file, which removes the button's value attribute from a beforetoggle listener and then prints dialog.returnValue.

Binding a callee's reference parameter to caller-owned, script-mutable storage that a re-entrancy point inside the callee can free.

Background

String types and ownership. WebKit's strings are refcounted around a shared StringImpl buffer. AtomString wraps an interned AtomStringImpl living in a per-thread atom table, and AtomString::string() exposes the wrapped String by reference. Copying a String takes an additional reference; taking a const String& does not.

Element attribute storage. An element's attributes are held as Attribute records containing an AtomString value. HTMLButtonElement::value() returns const AtomString& — a reference directly into that storage — and removeAttribute() destroys the record, releasing its reference.

Command invokers. A <button> with command and commandfor attributes dispatches a declarative command to the target element on activation, routed through Element::handleCommandInternal, which HTMLDialogElement overrides as bool handleCommandInternal(HTMLButtonElement& invoker, const CommandType&) final. The button's value attribute supplies the return value for the close and request-close commands.

beforetoggle. A cancelable ToggleEvent that dialogs and popovers dispatch before committing an open/closed state transition, giving page script a synchronous hook inside the middle of the C++ state-change routine. HTMLDialogElement::show() and showModal() display this dispatch-then-recheck shape.

m_returnValue. A String member of HTMLDialogElement, set by setReturnValue(String&&) and exposed to script through const String& returnValue() const, which the IDL surfaces as the dialog.returnValue property.

Re-entrancy. Any point where native code dispatches a DOM event calls into JavaScript, which may synchronously mutate arbitrary C++-backed DOM state before returning.

Analysis

The root cause is a use-after-free: handleCommandInternal bound the callee's const String& parameter directly to storage owned by the invoker button's value attribute. Neither HTMLButtonElement::value() nor AtomString::string() copies or takes an owning reference — the chain is a two-hop alias into the element's attribute list. The missing invariant is that a reference into script-mutable DOM attribute storage must not outlive a call that can re-enter JavaScript.

  main thread, inside close():

  handleCommandInternal
    └─ close(invoker.value().string(), &invoker)   ← borrow, refcount unchanged
         │
         ├─ dispatch beforetoggle ───────────────► listener JS
         │                                            │
         │                        button.removeAttribute('value')
         │                          └─ Attribute destroyed
         │                               └─ last ref on AtomStringImpl dropped
         │                                    └─ unregistered from atom table, freed
         │   ┌────────────────────────────────────────┘
         ▼   ▼
       consume the string parameter
         └─ m_returnValue = <freed StringImpl>   ← UAF read + refcount write

close() dispatches a cancelable beforetoggle ToggleEvent before it commits the state change — the same pattern visible in show() and showModal() in the supplied source, and confirmed operationally by the regression test, whose beforetoggle listener fires during the close() invoked by button.click(). Inside that listener, script calls button.removeAttribute('value'). Destroying the Attribute releases its AtomString, and because the test builds the value at runtime ('PA' + 'SS') rather than using a literal that would already be interned and retained elsewhere, that release drops the last reference: the AtomStringImpl is unregistered from the atom table and freed. When close() resumes, the reference it still holds points at the freed StringImpl, and the assignment into m_returnValue both reads the freed header and increments its refcount. The precise line inside close() that consumes the parameter after the dispatch is inferred — the supplied source context for HTMLDialogElement.cpp is truncated before close() — but it must follow the dispatch for the reported crash to occur.

Reachability needs nothing exotic: an open <dialog>, a <button command=close commandfor=dialog value=...> inside it, a beforetoggle listener, and a scripted .click(). No user gesture or feature flag appears in the test. Following it step by step:

1. button.setAttribute('value', 'PA' + 'SS') interns a runtime-built value, so the resulting AtomStringImpl is referenced only by the button's attribute record rather than by a pre-existing literal atom.
2. button.click() reaches handleCommandInternal, which pre-fix bound close()'s const String& parameter straight to that attribute's storage.
3. close() dispatches beforetoggle.
4. The listener's button.removeAttribute('value') destroys the Attribute, releasing the last reference and freeing the AtomStringImpl.
5. close() resumes and consumes the now-dangling reference when populating m_returnValue.

To convert this into a primitive an attacker would need, in the listener window, to reclaim the freed allocation — the value length is fully attacker-chosen, so the target size class can be selected, and the listener itself is an ideal grooming site since arbitrary JS runs between the free and the use. If the reclaiming allocation holds attacker-controlled or sensitive heap data, the subsequent String copy into m_returnValue would read a length and buffer pointer out of the reclaimed bytes, and reading back dialog.returnValue could disclose out-of-bounds or stale heap contents to script. Separately, the copy performs a refcount increment on the freed header, which could corrupt whatever object now occupies that slot, and the eventual release of m_returnValue could then decrement and free a pointer derived from reclaimed memory. Each of these escalation steps is conditional on winning the reclaim with controlled contents; the immediately observed effect the test targets is an ASAN-detectable use-after-free read.

This vulnerability weakens memory safety inside the WebContent process. The security model assumption it breaks is that a string handed from the DOM into an internal state-machine call remains alive for the duration of that call; before the fix, ordinary page script running in a beforetoggle listener could drop the last reference while the callee still held an alias to it. An attacker who controls the page could free an attacker-sized, attacker-populated heap allocation at a precisely chosen point and have the engine subsequently read it and store a String referring to it in m_returnValue, which is script-readable — so successful exploitation would most plausibly yield heap-contents disclosure and a refcount-manipulation foothold in the renderer rather than direct control-flow hijack.

The dangerous shape is an accessor chain that never materializes ownership: invoker.value() returns const AtomString& and .string() returns const String&, so a call site that looks like it is passing a value is actually passing a two-hop alias into DOM attribute storage. Callees in WebCore routinely take const String& for exactly this efficiency reason, which is fine right up until the callee dispatches an event. HTMLButtonElement.h already annotates value() with NODELETE and HTMLDialogElement.h annotates returnValue() with LIFETIME_BOUND — the codebase is aware of lifetime hazards on these accessors, but those annotations catch temporaries and direct escapes, not liveness across an intervening script re-entrancy. The declarative command-invoker feature is recent surface that plumbs attribute values straight into popover and dialog state machines whose entire design is to fire beforetoggle mid-transition, so this pairing is structurally bug-prone.

Audit directions

• A callee's const& parameter aliasing caller-owned, script-mutable storage across an event dispatch. The invariant is no reference derived from DOM state may be live across a call that can run JavaScript. Narrow: grep WebCore for call sites passing .value(), .string(), attributeWithoutSynchronization(...), or getAttribute(...) results directly as arguments to functions that dispatch events — start with the sibling invoker paths HTMLElement::handleCommandInternal (popover show/hide/toggle) and other Element::handleCommandInternal overrides, and with HTMLDialogElement::requestClose/close/queueDialogToggleEventTask. Wider: the same class appears through other non-owning accessors — const AtomString& attribute getters, StringView locals, Vector/HashMap element references, and iterators held across dispatchEvent, ScriptRunner execution, promise resolution, or forced style/layout. In code review, any function taking const String&/const AtomString&/StringView whose body contains dispatchEvent or queue*Task before it consumes the parameter is the visual tell. Widest: a borrowed reference must not outlive a yield point into untrusted code — carry it into any host/script embedding (V8/Blink String borrows across MaybeLocal callbacks, Python C-API borrowed refs across PyObject_Call, Rust FFI slices across a callback into managed code); the audit question is always "which yield points exist between borrow and use, and can code at those points drop the owner?"

• All state read after a beforetoggle dispatch, not just string parameters. show() and showModal() in HTMLDialogElement.cpp already re-check isOpen(), isConnected(), and isPopoverShowing() after dispatching, which shows the authors know the dispatch is a re-entrancy barrier — enumerate every value that crosses that barrier in close(), requestClose(), queueDialogToggleEventTask(), and the popover equivalents in HTMLElement.cpp, and check each for either a re-validation or an owning copy. Wider: the same audit applies to every WebCore routine that fires a cancelable pre-event and then commits (beforeinput, beforematch, beforeunload, submit); the tell is a local or parameter captured before the dispatch and dereferenced after it with no intervening recheck. Widest: check-then-act across a callback boundary requires re-checking after the callback — it holds in any event-driven system with cancelable pre-events, including DOM listeners generally, Node's EventEmitter hooks, and database before-triggers.

• The Element command-invoker surface as a whole, since it is the newest path plumbing invoker-owned data into unrelated elements' state machines. Trace every consumer of the HTMLButtonElement& invoker parameter across all handleCommandInternal overrides (Element.h base, HTMLElement.cpp popover version, HTMLDialogElement.cpp) and confirm that anything read off the invoker — value(), command(), commandForElement(), commandType() — is either copied before, or re-read after, any dispatch; the match tell is a read from invoker that is not adjacent to its use. Wider: the same caller-object-survives-only-by-convention shape applies to any WebCore API taking Element& source / Element* source and then firing events — note that close(const String&, Element* = nullptr) and showModal(Element*) both take a raw Element* source pointer, worth checking whether those raw pointers are guaranteed live across their dispatches or only incidentally so. Widest: wherever an API takes a bare reference to a caller-owned object and hands control to third-party code before using it, ask who holds the strong reference during the callback — the same question applies in Cocoa delegate callbacks, Rust &mut passed into FFI, and any plugin architecture.

• The inverse sweep, from the callee side. Grep for functions whose signature is void f(const String&, ...) or (const AtomString&, ...) in WebCore/html and WebCore/dom that store the parameter into a long-lived member (like m_returnValue) after an event dispatch. A hit is any body matching dispatchEvent(...) followed by m_something = param. This finds the same defect class independent of which accessor produced the argument, and unlike the caller-side grep it does not depend on knowing which accessors return non-owning references.