2024653 - Heap use-after-free in [@ nsObjectLoadingContent::UnloadObject] via missing script blocker on same-value setAttribute

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Bugmon [:jkratzer for issues]]

Summary

Heap use-after-free in nsObjectLoadingContent::UnloadObject (dom/base/nsObjectLoadingContent.cpp:1608). The root cause is a missing script blocker on the OnAttrSetButNotChanged path in Element::SetAttrInternal (dom/base/Element.cpp:3641). When setAttribute('data', sameValue) is called on an <object>, the LoadObject script runner executes synchronously instead of being deferred, allowing TriggerInnerFallbackLoads to call StartObjectLoad on child <object> elements via raw pointers while no strong reference exists on the stack. The child's frame-loader destruction fires a synchronous pagehide event whose handler frees the child, and UnloadObject then writes mFrameLoader = nullptr into freed memory.

Steps to Reproduce

Requires a Firefox ASan build with FuzzingFunctions enabled (--enable-fuzzing). The testcase uses FuzzingFunctions.spinEventLoopFor() to deterministically drain the AsyncFreeSnowWhite idle task during the pagehide handler — but the underlying refcount-zero-on-live-stack-frame bug is independent of the fuzzing API.

Affected Code

File: dom/base/Element.cpp, line 3641

if (OnlyNotifySameValueSet(aNamespaceID, aName, aPrefix, aValue, aNotify,
                           oldValue, &modType, &oldValueSet)) {
  OnAttrSetButNotChanged(aNamespaceID, aName, aValue, aNotify);  // ← no script blocker
  return NS_OK;
}
mozAutoDocUpdate updateBatch(document, aNotify);  // ← blocker only on the value-changed path

Crash-stack frame #13 shows SetAttrInternal invoking OnAttrSetButNotChanged at line 3641, and frame #11 shows nsContentUtils::AddScriptRunner at line 7278 executing the runnable immediately (runnable->Run() rather than appending to sBlockedScriptRunners).

File: dom/base/nsObjectLoadingContent.cpp, line 1716

} else if (auto* object = HTMLObjectElement::FromNode(child)) {
  object->StartObjectLoad(true, true);     // ← raw pointer; runs script; no kungFuDeathGrip
  child = child->GetNextNonChildNode(el);  // ← also UAF if child was freed
}

Frame #6 (TriggerInnerFallbackLoads:1716) is the caller that holds only a raw HTMLObjectElement* to the object that gets freed.

File: dom/base/nsObjectLoadingContent.cpp, line 1608 (crash site)

if (mFrameLoader) {
  mFrameLoader->Destroy();   // ← fires synchronous pagehide → JS frees `this`
  mFrameLoader = nullptr;    // ← UAF READ+WRITE at offset 184 of freed 400-byte HTMLObjectElement
}

Frames #0–#2 show RefPtr<nsFrameLoader>::operator=(nullptr) reading the old pointer value at this+184 inside a freed heap region (fd shadow bytes).

Exploit Chain

1. Setup. Outer <object id="outer" classid="x" data="dummy"> is created. The classid forces it into ObjectType::Fallback. An inner <object data="blob:…"> is inserted via innerHTML (alloc stack: NS_NewHTMLObjectElement @ HTMLObjectElement.cpp:279 ← SetInnerHTMLInternal). The inner blob document registers a pagehide listener and signals readiness via postMessage.
2. Trigger. JavaScript calls outer.setAttribute('data', 'dummy') — the same value already set.
3. Element::SetAttrInternal → OnlyNotifySameValueSet() returns true → calls OnAttrSetButNotChanged at line 3641 before mozAutoDocUpdate is constructed. No script blocker is active.
4. HTMLObjectElement::OnAttrSetButNotChanged (line 114) → AfterMaybeChangeAttr → nsContentUtils::AddScriptRunner(lambda). Because sScriptBlockerCount == 0, the lambda runs synchronously (frame #11).
5. The lambda (HTMLObjectElement.cpp:138) calls LoadObject(true, true) on outer. Outer is in Fallback → LoadObject reaches line 1333 and calls TriggerInnerFallbackLoads().
6. TriggerInnerFallbackLoads iterates children with a raw nsIContent* child. For the inner <object> it calls object->StartObjectLoad(true, true) at line 1716 — no strong reference taken.
7. Inner's StartObjectLoad (line 248) → LoadObject → UnloadObject (line 1131). Inner has a loaded document, so mFrameLoader is non-null → calls mFrameLoader->Destroy().
8. nsFrameLoader::StartDestroy → Document::FinalizeFrameLoader → AddScriptRunner(MaybeInitializeFinalizeFrameLoaders) — again runs synchronously → destroy runnable → nsFrameLoader::DestroyDocShell → nsDocShell::Destroy → FirePageHideNotification → pagehide event dispatched synchronously on the inner subdocument (free-stack frames #31–#38).
9. The pagehide handler executes parent.document.getElementById('outer').textContent = ''. The inner <object> is removed from the DOM; UnbindFromTree drops the tree reference. No strong ref remains anywhere (frame loader's mOwnerContent is weak, mEmbedderElement was already cleared in StartDestroy). Refcount hits 0; inner is queued as snow-white in the purple buffer.
10. The handler calls FuzzingFunctions.spinEventLoopFor(50). The nested event loop processes the AsyncFreeSnowWhite idle runnable → SnowWhiteKiller::Visit → nsIContent::Destroy() → free() (free-stack frames #2–#6). The 400-byte HTMLObjectElement is freed.
11. Stack unwinds back to UnloadObject. Line 1608 executes mFrameLoader = nullptr;. RefPtr::operator=(nullptr_t) reads this->mRawPtr at offset 184 of freed memory → heap-use-after-free READ, followed by an 8-byte WRITE to the same freed address.

Security Impact

Severity: High.

Attacker capability: A web page controlling a same-origin subdocument inside a nested <object> fallback achieves a use-after-free on a 400-byte HTMLObjectElement while one of its member functions is mid-execution. The RefPtr::operator=(nullptr) sequence reads the old nsFrameLoader* and calls Release() on it — if an attacker reclaims the freed slot between step 10 and step 11 with a crafted fake pointer at offset 184, the Release is a virtual call through an attacker-controlled vtable. A second UAF (child->GetNextNonChildNode) follows at line 1718 when control returns to TriggerInnerFallbackLoads.

Preconditions: The testcase uses FuzzingFunctions.spinEventLoopFor() (fuzzing-build-only) to deterministically drain AsyncFreeSnowWhite. The underlying bug — an object with refcount 0 while still live on the native stack — is independent of this API. Any content-reachable nested event loop that runs idle tasks (synchronous XHR, showModalDialog-style reentry, print()) is a potential substitute; at minimum, deferred AsyncFreeSnowWhite will eventually reuse the slot under memory pressure.

Suggested Fix

Apply defense in depth at all three sites:

1. Add a script blocker on the same-value path (dom/base/Element.cpp:~3639, and the parallel site in SetParsedAttr):

if (OnlyNotifySameValueSet(aNamespaceID, aName, aPrefix, aValue, aNotify,
                           oldValue, &modType, &oldValueSet)) {
  nsAutoScriptBlocker scriptBlocker;   // defer AddScriptRunner runnables
  OnAttrSetButNotChanged(aNamespaceID, aName, aValue, aNotify);
  return NS_OK;
}

2. Keep this alive and move-clear mFrameLoader before running script (dom/base/nsObjectLoadingContent.cpp:1605):

void nsObjectLoadingContent::UnloadObject(bool aResetState) {
  RefPtr<Element> kungFuDeathGrip = AsElement();
  if (RefPtr<nsFrameLoader> loader = std::move(mFrameLoader)) {
    loader->Destroy();
  }
  // ...
}

3. Snapshot children with strong refs before iterating (dom/base/nsObjectLoadingContent.cpp:~1707):

AutoTArray<RefPtr<nsIContent>, 4> targets;
for (nsIContent* c = el->GetFirstChild(); c;) {
  if (c->IsAnyOfHTMLElements(nsGkAtoms::embed, nsGkAtoms::object)) {
    targets.AppendElement(c);
    c = c->GetNextNonChildNode(el);
  } else {
    c = c->GetNextNode(el);
  }
}
for (auto& t : targets) {
  if (!t->IsInComposedDoc()) continue;
  if (auto* e = HTMLEmbedElement::FromNode(t))  e->StartObjectLoad(true, true);
  else if (auto* o = HTMLObjectElement::FromNode(t)) o->StartObjectLoad(true, true);
}

Comment 1

[Bugmon [:jkratzer for issues]]

Attachment: Crash stack trace

Comment 2

[Bugmon [:jkratzer for issues]]

Attachment: Testcase: test.html

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: (secure)

Comment 6

[Christian Holler (:decoder)]

--enable-fuzzing is not required to reach the bug — FuzzingFunctions.spinEventLoopFor() only drains AsyncFreeSnowWhite deterministically. The refcount-zero-on-live-stack condition is reached at step 9 via pure DOM (same-value setAttribute → unblocked OnAttrSetButNotChanged → synchronous pagehide), and the missing script blocker / kungFuDeathGrip at the three cited sites are all ungated production code. Not marking unsupported-config.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

This needs esr115 and esr140 patch(es).

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9555675 [details]
(secure)

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I'd say not very easy
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Shouldn't be hard.
• How likely is this patch to cause regressions; how much testing does it need?: This is a bit regression risky
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: sec-high
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky):
• String changes made/needed: NA
• Is Android affected?: Yes

Comment 9

[Tom Ritter [:tjr]]

Comment on attachment 9555675 [details]
(secure)

Approved to land and request uplift

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: (secure)

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: (secure)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/d561f627cd84
https://hg.mozilla.org/integration/autoland/rev/215ccdd50da0
be more consistent with non-changing attribute setting, r=farre

Comment 13

[Donal Meehan [:dmeehan]]

https://hg-edge.mozilla.org/mozilla-central/rev/215ccdd50da0

Comment 14

[Dianna Smith [:diannaS]]

Comment on attachment 9555675 [details]
(secure)

Approved for 150.0b5

Comment 15

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/a8c40203542f
https://hg.mozilla.org/releases/mozilla-beta/rev/bfdc2129d4ab

Comment 16

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/3272777f0fa9

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Not sure why esr115 was marked unaffected.

Comment 18

[Christian Holler (:decoder)]

Attachment: ESR115 backport of d561f627cd84

Comment 19

[Christian Holler (:decoder)]

ESR115 is affected. Reproduced the crash on a clean ESR115 ASAN build (SEGV in nsObjectLoadingContent::ConfigureFallback reached via Element::SetAttr -> HTMLObjectElement::OnAttrSetButNotChanged -> AddScriptRunner -> LoadObject, matching the upstream root cause).

Backported the central fix d561f627cd84 to ESR115. Adjustments vs. m-c:

• OnlyNotifySameValueSet's ESR115 signature has an extra bool* aHasListeners; logic change is the same (move OnAttrSetButNotChanged under the existing nsAutoScriptBlocker, drop the call from both SetAttr and SetParsedAttr).
• ESR115 has no TriggerInnerFallbackLoads; the equivalent loop lives inside ConfigureFallback. Snapshotted the targets into an AutoTArray<RefPtr<nsIContent>, 4> and re-checked IsInclusiveDescendantOf before invoking StartObjectLoad, preserving the existing hasHtmlFallback accumulation.
• Same RefPtr<nsFrameLoader> loader = std::move(mFrameLoader); loader->Destroy(); substitution in SetupDocShell, LoadObject's uriLoader-failure path, UnloadObject, and removal of the redundant mFrameLoader block from Destroy() (UnloadObject already covers it).

Verified: rebuilt and re-ran the testcase from attachment 9554941 [details] — no crash.

This is the analysis tool's suggested fix. Feel welcome to adopt it as a starting point and evolve it as needed to meet our coding standards.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D291058

Comment 21

[Phabricator Automation]

firefox-esr115 Uplift Approval Request

• User impact if declined/Reason for urgency: sec sensitive crash
• Code covered by automated testing?: no
• Fix verified in Nightly?: no
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing: See the bug
• Risk associated with taking this patch: medium
• Explanation of risk level: This isn't super trivial.
• String changes made/needed?: NA
• Is Android affected?: yes

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/bb2013278934