<a class="header-button" href="https://bugzilla.mozilla.org/home" title="Go to home page"> Bugzilla

Reporter

Comment 1

•

5 months ago

Attached file asan.log — Details

Updated

•

5 months ago

Attachment #9590870 - Attachment mime type: text/x-log → text/plain

Comment 2

•

5 months ago

Jan, it sounds like maybe this is a problem with a JIT optimization and not the canvas 2d code? Or should I move it elsewhere. Thanks.

URL: https://github.com/mozilla-firefox/fi...

Group: firefox-core-security → javascript-core-security

Component: Security → JavaScript Engine: JIT

Flags: needinfo?(jdemooij)

Product: Firefox → Core

Comment 3

•

5 months ago

(I've edited comment 0 to add 4 more frames to the ASan stack for clarity.)

Iain Ireland [:iain]

Comment 4

•

5 months ago

(In reply to Hyeonjun Ahn [:deayzl] from comment #0)

Optimized JIT code hoists a typed-array backing-store pointer across an HTMLElement.innerText getter.

Assuming that the writeup is accurate, this is the bug. We should not be able to do this. Looking at the JSJitInfo that the DOM provides for HTMLElement.innerText, its alias set is JSJitInfo::AliasDOMSets, which implies that it has no side-effects (and that its value can only be changed by changing something in the DOM). If it can trigger a callback that runs arbitrary code (here, MozScrolledAreaChanged), then it should instead have JSJitInfo::AliasEverything.

I'm not an expert on how webidl turns into JSJitInfo, but cross-referencing some other getters with the same alias set, it looks the Pure annotation is to blame here.

Comment 5

•

5 months ago

Okay, great. I guess a bad annotation for innerText would make this more of a DOM issue then.

Group: javascript-core-security → dom-core-security

Component: JavaScript Engine: JIT → DOM: Core & HTML

Keywords: csectype-uaf

Jan de Mooij [:jandem]

Comment 6

•

5 months ago

Clearing the NI since Iain already looked at this.

Flags: needinfo?(jdemooij)

Reporter

Comment 7

•

5 months ago

Attached file firefox-outertext-xhr-canvas-dash-uaf.html — Details

Reporter

Comment 8

•

5 months ago

Attached file asan-outertext-canvas-dash.log — Details

Kagami Rosylight 🏳️‍🌈🏳️‍⚧️ [:saschanaz] (they/them)

Reporter

Comment 9

•

5 months ago

The [Pure] of outerText is also problematic, so heap-buffer-overflow is reproducible using it.

ASAN_OPTIONS='quarantine_size_mb=0' \
  /home/slave/firefox-bug-bounty/obj-x86_64-pc-linux-gnu-asan-browser/dist/bin/firefox \
  'http://127.0.0.1:8765/firefox-outertext-xhr-canvas-dash-uaf.html?warmup=2200&warmupIters=20&attempts=20&triggerIters=1&payloadSize=2048&dashLength=510&sprayCanvases=8000&getDashStride=1&p
reTouch=1'

Result:

=================================================================
==3587411==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x791c399a0a80 at pc 0x774c2543bcbe bp 0x7ffd56fddc20 sp 0x7ffd56fddc18
READ of size 4 at 0x791c399a0a80 thread T0 (Isolated Web Co)
    #0 0x774c2543bcbd in Construct<const float &> /home/slave/firefox-bug-bounty/obj-x86_64-pc-linux-gnu-asan-browser/dist/include/nsTArray.h:554:36
    ...
    #37 0x774c22f8c9ae in mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) /home/slave/firefox-bug-bounty/obj-x86_64-pc-linux-gnu-asan-browser/dom/base/./../../../firefox/dom/base/Document.cpp:11635:16
    #38 0x774c267406b9 in nsGenericHTMLElement::GetInnerText(mozilla::dom::DOMString&, mozilla::ErrorResult&) /home/slave/firefox-bug-bounty/obj-x86_64-pc-linux-gnu-asan-browser/dom/html/./../../../firefox/dom/html/nsGenericHTMLElement.cpp:3393:10
    #39 0x774c235d6c82 in GetOuterText /home/slave/firefox-bug-bounty/firefox/dom/html/nsGenericHTMLElement.h:270:12
    #40 0x774c235d6c82 in mozilla::dom::HTMLElement_Binding::get_outerText(JSContext*, JS::Handle<JSObject*>, void*, JSJitGetterCallArgs) /home/slave/firefox-bug-bounty/obj-x86_64-pc-linux-gnu-asan-browser/dom/bindings/./HTMLElementBinding.cpp:1387:24
    #41 0x352de352eef8  ([anon:js-executable-memory]+0x10ef8)
    #42 0x352de34fe3bd  ([anon:js-executable-memory]+0x3bd)
    #43 0x774c2e4d9cc8 in EnterJit /home/slave/firefox-bug-bounty/obj-x86_64-pc-linux-gnu-asan-browser/js/src/jit/./../../../../firefox/js/src/jit/Jit.cpp:105:5
    #44 0x774c2e4d9cc8 in js::jit::MaybeEnterJit(JSContext*, js::RunState&) /home/slave/firefox-bug-bounty/obj-x86_64-pc-linux-gnu-asan-browser/js/src/jit/./../../../../firefox/js/src/jit/Jit.cpp:253:10

(you can see mozilla::dom::HTMLElement_Binding::get_outerText in attached asan-outertext-canvas-dash.log)

So I applied below patch, and both htmls that i attached did not reproduce the bug.

diff --git a/dom/webidl/HTMLElement.webidl b/dom/webidl/HTMLElement.webidl
index 034aa0de194f..e5d4fea347ee 100644
--- a/dom/webidl/HTMLElement.webidl
+++ b/dom/webidl/HTMLElement.webidl
@@ -25,9 +25,9 @@ interface HTMLElement : Element {
   [CEReactions, SetterThrows, Pure]
            attribute DOMString dir;
 
-  [CEReactions, GetterThrows, Pure]
+  [CEReactions, GetterThrows]
            attribute [LegacyNullToEmptyString] DOMString innerText;
-  [CEReactions, GetterThrows, SetterThrows, Pure]
+  [CEReactions, GetterThrows, SetterThrows]
            attribute [LegacyNullToEmptyString] DOMString outerText;
 
   // user interaction

Comment 10

•

5 months ago

I don't understand how they can be "Pure" which translates to Affects=Nothing. That can't be true?

Olli, do you remember the background behind bug 923913, other than "it's faster that way"?

Flags: needinfo?(smaug)

Reporter

Comment 11

•

5 months ago

Attached file firefox-innertext-xhr-datalist-mfunc-call.html — Details

Reporter

Comment 12

•

5 months ago

For confirming its highest impact, here's a reproducer for pc control.

The optimized loop reads generic HTMLElement.innerText.
The same loop performs an in-bounds typed-array store.
Warmup compiles the loop.
After warmup, the script makes the descendant of the same div dirty by child.style.height = (230000 + index) + "px";.
The large child height changes the scroll area during GetInnerText’s flush, so the layout flush dispatches MozScrolledAreaChanged.
The event handler detaches the buffer and makeListBucket() sprays new datalist elements and accesses each dl.options to reclaim the freed ArrayBuffer backing-store as ContentList object.
The stale typed-array store overwrites ContentList::mFunc with 0x414141414141 at offset 240, then options.length triggers ContentList::Match() which executes return (*mFunc)(aElement, mMatchNameSpaceId, mXMLMatchAtom, mData);.

ASAN_OPTIONS='quarantine_size_mb=0' /home/slave/firefox-bug-bounty/obj-x86_64-pc-linux-gnu-asan-browser/dist/bin/firefox 'http://127.0.0.1:8765/firefox-innertext-xhr-datalist-mfunc-call.html?payload=payload-280.bin?x=1&warmup=2200&warmupIters=20&attempts=40&triggerIters=120&payloadSize=280&sprayLists=12000&writeIndex=30&callTarget=0x0000414141414141&preTouch=1' > pc-control.log 2>&1

Result:

...
REPRO warmup 2000/2200
REPRO warmup done
REPRO starting attempts=40
REPRO_XHR status=200 bytes=280
AddressSanitizer:DEADLYSIGNAL
=================================================================
==3940175==ERROR: AddressSanitizer: SEGV on unknown address 0x414141414141 (pc 0x414141414141 bp 0x7ffcc2d22db0 sp 0x7ffcc2d22d78 T0)
==3940175==The signal is caused by a READ memory access.
AddressSanitizer:DEADLYSIGNAL
AddressSanitizer: nested bug in the same thread, aborting.
[Parent 3939949, IPC I/O Parent] WARNING: process 3940175 exited with status 1: file firefox/ipc/chromium/src/chrome/common/process_watcher_posix_sigchld.cc:155

Assignee

Comment 13

•

5 months ago

•

Edited

(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #10)

I don't understand how they can be "Pure" which translates to Affects=Nothing. That can't be true?

Olli, do you remember the background behind bug 923913, other than "it's faster that way"?

This is not about innerHTML, but innerText. They are quite different beasts.
innerHTML should be fine

Flags: needinfo?(smaug)

Kagami Rosylight 🏳️‍🌈🏳️‍⚧️ [:saschanaz] (they/them)

Assignee

Updated

•

5 months ago

Assignee: nobody → smaug

Comment 14

•

5 months ago

And innetHTML is "purer" than innerText? 🤔 Still confused

innerText https://searchfox.org/firefox-main/rev/e28b34ab33dbf49364999070168cbb7e11e8e5bd/dom/html/nsGenericHTMLElement.cpp#3342,3393

Assignee

Comment 15

•

5 months ago

innerHTML getter just stringifies DOM.

Comment 16

•

5 months ago

I haven't verified this yet, but it sounds plausible, so I'll mark it sec-high. I think the underlying issue here is around JIT optimization, so I'll change it to csectype-jit although this is more of a misrepresentation by DOM code than the fault of the actual JIT.

Keywords: csectype-uaf → csectype-jit, sec-high

Assignee

Comment 17

•

5 months ago

I'm still trying to reproduce this. No luck with my normal asan build, but perhaps gczeal is needed.

Jan de Mooij [:jandem]

Comment 18

•

5 months ago

(In reply to Olli Pettay [:smaug][bugs@pettay.fi] from comment #17)

I'm still trying to reproduce this. No luck with my normal asan build, but perhaps gczeal is needed.

I haven't tried to reproduce this but make sure the loop runs for at least a few thousand iterations, to hit Ion. javascript.options.ion.offthread_compilation = false might also help.

Assignee

Comment 19

•

5 months ago

with that and ASAN_OPTIONS='quarantine_size_mb=0' I managed to reproduce. And removing Pure helps as expected.

Updated

•

5 months ago

Status: UNCONFIRMED → ASSIGNED

Ever confirmed: true

Updated

•

5 months ago

Summary: heap-buffer-overflow from corrupted canvas line dash metadata → HTMLElement.innerText and outerText are marked [Pure] but can run script via MozScrolledAreaChanged

Comment 20

•

5 months ago

I added JS::AutoAssertNoGC nogc; to the start of nsGenericHTMLElement::GetInnerText and it fires off when we are creating the reflector for MozScrolledAreaChanged, which can give you a sense of how we're running content JS. That's not exactly the invariant we probably would want, but there's a JS::Call later on in HandleEvent. Here's an excerpt from that stack:

    #11 0x000133758604 in mozilla::dom::EventListener::HandleEvent(mozilla::dom::BindingCallContext&, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&)
    #18 0x0001397fa5a8 in mozilla::ScrollContainerFrame::FireScrolledAreaEvent()
    #19 0x0001397fa2c0 in mozilla::ScrollContainerFrame::ScrolledAreaEvent::Run()
    #20 0x000131625a68 in nsContentUtils::RemoveScriptBlocker()
    #21 0x000139638ab4 in mozilla::PresShell::ProcessReflowCommands(bool)
    #22 0x00013960f184 in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush)
    #23 0x000131aac778 in mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush)
    #24 0x000134e2f444 in nsGenericHTMLElement::GetInnerText(mozilla::dom::DOMString&, mozilla::ErrorResult&)
    #25 0x0001321d6cb4 in mozilla::dom::HTMLElement_Binding::get_innerText(JSContext*, JS::Handle<JSObject*>, void*, JSJitGetterCallArgs)

Steve Fink [:sfink] [:s:]

Assignee

Comment 21

•

5 months ago

Attached file (secure) — Details

Comment 22

•

5 months ago

(In reply to Andrew McCreight [:mccr8] from comment #20)

I added JS::AutoAssertNoGC nogc; to the start of nsGenericHTMLElement::GetInnerText and it fires off when we are creating the reflector for MozScrolledAreaChanged, which can give you a sense of how we're running content JS. That's not exactly the invariant we probably would want, but there's a JS::Call later on in HandleEvent. Here's an excerpt from that stack:

Can event dispatch run JS code? This is from an older build, but I see things like this (which uses gfx::VRManager, so perhaps not too relevant):

mozilla::ScrollContainerFrame::FireScrolledAreaEvent()
mozilla::EventDispatcher::Dispatch(mozilla::dom::EventTarget*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, uint32*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*)
mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&)
mozilla::EventTargetChainItem::PostHandleEvent(mozilla::EventChainPostVisitor&)
mozilla::dom::EventTarget.PostHandleEvent:0
nsGlobalWindowInner.PostHandleEvent:0
nsGlobalWindowInner::PostHandleEvent(mozilla::EventChainPostVisitor&)
mozilla::dom::VREventObserver::NotifyAfterLoad()
mozilla::gfx::VRManagerChild::FireDOMVRDisplayConnectEventsForLoad(mozilla::gfx::VRManagerEventObserver*)
nsContentUtils::AddScriptRunner(already_AddRefed<nsIRunnable>)
nsIRunnable.Run:0
(js-code)

or if you don't like the VR stuff:

mozilla::ScrollContainerFrame::FireScrolledAreaEvent()
mozilla::EventDispatcher::Dispatch(mozilla::dom::EventTarget*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, uint32*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*)
mozilla::dom::PerformanceMainThread* nsPresContext::GetPerformanceMainThread() const
mozilla::dom::Performance* nsPIDOMWindowInner::GetPerformance()
nsPIDOMWindowInner::CreatePerformanceObjectIfNeeded()
mozilla::dom::Performance::CreateForMainThread(nsPIDOMWindowInner*, nsIPrincipal*, nsDOMNavigationTiming*, nsITimedChannel*)
mozilla::dom::PerformanceMainThread::PerformanceMainThread(nsPIDOMWindowInner*, nsDOMNavigationTiming*, nsITimedChannel*)
_ZN7mozilla3dom21PerformanceMainThreadC4EP18nsPIDOMWindowInnerP21nsDOMNavigationTimingP15nsITimedChannel
mozilla::dom::PerformanceMainThread::CreateNavigationTimingEntry()
PerformanceMainThread.cpp:void mozilla::dom::{anonymous}::GetURLSpecFromChannel(nsITimedChannel*, const nsTSubstring<char16_t>&)
nsIChannel.GetURI:0
(js-code)

(but this is from the hazard analysis, which is excessively conservative about such things in Gecko code.)

Kagami Rosylight 🏳️‍🌈🏳️‍⚧️ [:saschanaz] (they/them)

Comment 23

•

5 months ago

(In reply to Steve Fink [:sfink] [:s:] from comment #22)

Can event dispatch run JS code?

Yes as it triggers event listeners which are JS functions.

Assignee

Updated

•

5 months ago

Severity: -- → S2

Assignee

Comment 24

•

5 months ago

Comment on attachment 9591549 [details]
(secure)

Security Approval Request

How easily could an exploit be constructed based on the patch?: I think it is not very obvious
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?: I think backports are the same patch except excluding the EditContext part (an API which isn't shipping yet anywhere)
How likely is this patch to cause regressions; how much testing does it need?: If this causes regressions, those should be performance issues. Possible, but not very likely.
Is the patch ready to land after security approval is given?: Yes
Is Android affected?: Yes

Attachment #9591549 - Flags: sec-approval?

Updated

•

5 months ago

status-firefox151: --- → wontfix

status-firefox152: --- → affected

status-firefox153: --- → affected

status-firefox-esr115: --- → affected

status-firefox-esr140: --- → affected

tracking-firefox152: --- → +

tracking-firefox153: --- → +

tracking-firefox-esr115: --- → 152+

tracking-firefox-esr140: --- → 152+

Updated

•

5 months ago

Attachment #9591549 - Flags: sec-approval? → sec-approval+

Assignee

Comment 25

•

5 months ago

The initial patch applies cleanly to beta.
esr140/115 need another patch.

Assignee

Comment 26

•

5 months ago

Attached file (secure) — Details

Comment 27

•

5 months ago

Pushed by opettay@mozilla.com: https://github.com/mozilla-firefox/firefox/commit/72649967bc7b https://hg.mozilla.org/integration/autoland/rev/a82f2f3a9d25 don't try to over-optimize some getter/function calls, r=jandem,webidl,emilio

https://hg-edge.mozilla.org/mozilla-central/rev/a82f2f3a9d25

Comment 28

•

5 months ago

Group: dom-core-security → core-security-release

Status: ASSIGNED → RESOLVED

Closed: 5 months ago

status-firefox153: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → 153 Branch

BugBot [:suhaib / :marco]

Comment 29

•

5 months ago

The patch landed in nightly and beta is affected.
:smaug, is this bug important enough to require an uplift?

If yes, please nominate the patch for beta approval.
- See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
If no, please set status-firefox152 to wontfix.

For more information, please visit BugBot documentation.

Flags: needinfo?(smaug)

Updated

•

5 months ago

Whiteboard: [client-bounty-form] → [client-bounty-form][adv-esr115.37+]

Updated

•

5 months ago

Whiteboard: [client-bounty-form][adv-esr115.37+] → [client-bounty-form]

Assignee

Updated

•

5 months ago

Flags: needinfo?(smaug)

Assignee

Comment 30

•

5 months ago

Comment on attachment 9592700 [details]
(secure)

ESR Uplift Approval Request

If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Unsafe crashes
Fix Landed on Version: 153
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): This removes couple of JIT<->DOM optimizations

Attachment #9592700 - Flags: approval-mozilla-esr140?

Attachment #9592700 - Flags: approval-mozilla-esr115?

Comment 31

•

5 months ago

•

Edited

This still needs a Beta request.

Flags: needinfo?(smaug)

Assignee

Comment 32

•

5 months ago

Comment on attachment 9591549 [details]
(secure)

Beta/Release Uplift Approval Request

User impact if declined/Reason for urgency: Unsafe crashes
Is this code covered by automated tests?: No
Has the fix been verified in Nightly?: Yes
Needs manual test from QE?: No
If yes, steps to reproduce: Use ASAN build
List of other uplifts needed: None
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): Removing some JIT<->DOM optimizations
String changes made/needed: NA
Is Android affected?: Yes

Flags: needinfo?(smaug)

Attachment #9591549 - Flags: approval-mozilla-beta?

Comment 33

•

5 months ago

Comment on attachment 9591549 [details]
(secure)

Approved for 152.0b8.

Attachment #9591549 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

•

5 months ago

status-firefox152: affected → fixed

https://github.com/mozilla-firefox/firefox/commit/cabba222ce1b https://hg.mozilla.org/releases/mozilla-beta/rev/cd5787e12c30

Comment 34

•

5 months ago

uplift

Comment 35

•

5 months ago

Comment on attachment 9592700 [details]
(secure)

Approved for 140.12esr and 115.37esr.

Attachment #9592700 - Flags: approval-mozilla-esr140?

Attachment #9592700 - Flags: approval-mozilla-esr140+

Attachment #9592700 - Flags: approval-mozilla-esr115?

Attachment #9592700 - Flags: approval-mozilla-esr115+

Updated

•

5 months ago

status-firefox-esr140: affected → fixed

https://hg.mozilla.org/releases/mozilla-esr140/rev/fea8b5fbd906

Comment 36

•

5 months ago

uplift

Updated

•

5 months ago

status-firefox-esr115: affected → fixed

https://hg.mozilla.org/releases/mozilla-esr115/rev/6b4023aab4c9

Comment 37

•

5 months ago

uplift

Andrei Vaida [:avaida]

Updated

•

5 months ago

QA Whiteboard: [sec] [uplift] [qa-triage-done-c153/b152]

Updated

•

5 months ago

Whiteboard: [client-bounty-form] → [client-bounty-form][adv-main152+]

Updated

•

5 months ago

Whiteboard: [client-bounty-form][adv-main152+] → [client-bounty-form][adv-main152+][adv-esr115.37+]

Updated

•

5 months ago

Whiteboard: [client-bounty-form][adv-main152+][adv-esr115.37+] → [client-bounty-form][adv-main152+][adv-esr115.37+][adv-esr140.12+]