1861344 - (CVE-2023-6207) use-after-poison during ReadableByteStreamQueueEntry::Buffer

Status: RESOLVED FIXED

(Core :: DOM: Streams, defect)

Description

[m.cooolie]

Attachment: poc.zip

#Reproduce
OS:Win10 X64
121.0a1 (2023-10-24) (64-bit)

step:

1. python -m http.server 1337
2. python -m ffpuppet firefox.exe -p prefs.js -d -u http://localhost:1337/poc.html

#Analysis
Coming soon

#ASAN

==22712==ERROR: AddressSanitizer: use-after-poison on address 0x2d73e07df008 at pc 0x7ffb4b9987c2 bp 0x005fe51f7850 sp 0x005fe51f7898
READ of size 8 at 0x2d73e07df008 thread T0
#0 0x7ffb4b9987c1 in js::gc::detail::GetTenuredGCThingZone /builds/worker/workspace/obj-build/dist/include/js/HeapAPI.h:525
#1 0x7ffb4b9987c1 in JS::GetTenuredGCThingZone /builds/worker/workspace/obj-build/dist/include/js/HeapAPI.h:638
#2 0x7ffb4b9987c1 in js::gc::ExposeGCThingToActiveJS /builds/worker/workspace/obj-build/dist/include/js/HeapAPI.h:754
#3 0x7ffb4b9987c1 in JS::ExposeObjectToActiveJS /builds/worker/workspace/obj-build/dist/include/js/HeapAPI.h:817
#4 0x7ffb4b9987c1 in js::BarrierMethods<class JSObject *, void>::exposeToJS(class JSObject *) /builds/worker/workspace/obj-build/dist/include/js/RootingAPI.h:804
#5 0x7ffb54f4ff9f in JS::Heap<JSObject *>::exposeToActiveJS /builds/worker/workspace/obj-build/dist/include/js/RootingAPI.h:348
#6 0x7ffb54f4ff9f in JS::Heap<JSObject *>::get /builds/worker/workspace/obj-build/dist/include/js/RootingAPI.h:351
#7 0x7ffb54f4ff9f in JS::Heap<JSObject *>::operator JSObject *const & /builds/worker/workspace/obj-build/dist/include/js/RootingAPI.h:343
#8 0x7ffb54f4ff9f in mozilla::dom::ReadableByteStreamQueueEntry::Buffer /builds/worker/workspace/obj-build/dist/include/mozilla/dom/ReadableByteStreamController.h:189
#9 0x7ffb54f4ff9f in mozilla::dom::streams_abstract::ReadableByteStreamControllerFillReadRequestFromQueue(struct JSContext *, class mozilla::dom::ReadableByteStreamController *, struct mozilla::dom::ReadRequest *, class mozilla::ErrorResult &) /builds/worker/checkouts/gecko/dom/streams/ReadableByteStreamController.cpp:719
#10 0x7ffb54f531cb in mozilla::dom::ReadableByteStreamController::PullSteps(struct JSContext *, struct mozilla::dom::ReadRequest *, class mozilla::ErrorResult &) /builds/worker/checkouts/gecko/dom/streams/ReadableByteStreamController.cpp:1057
#11 0x7ffb54f6847d in mozilla::dom::streams_abstract::ReadableStreamDefaultReaderRead(struct JSContext *, class mozilla::dom::ReadableStreamGenericReader *, struct mozilla::dom::ReadRequest *, class mozilla::ErrorResult &) /builds/worker/checkouts/gecko/dom/streams/ReadableStreamDefaultReader.cpp:235
#12 0x7ffb54f7b91d in mozilla::dom::ReadableStreamDefaultReader::Read(class mozilla::ErrorResult &) /builds/worker/checkouts/gecko/dom/streams/ReadableStreamDefaultReader.cpp:262
#13 0x7ffb4ff360bf in mozilla::dom::ReadableStreamDefaultReader_Binding::read /builds/worker/workspace/obj-build/dom/bindings/./ReadableStreamDefaultReaderBinding.cpp:545
#14 0x7ffb4ff360bf in mozilla::dom::ReadableStreamDefaultReader_Binding::read_promiseWrapper /builds/worker/workspace/obj-build/dom/bindings/./ReadableStreamDefaultReaderBinding.cpp:561
#15 0x7ffb51599218 in mozilla::dom::binding_detail::GenericMethod<struct mozilla::dom::binding_detail::NormalThisPolicy, struct mozilla::dom::binding_detail::ConvertExceptionsToPromises>(struct JSContext *, unsigned int, class JS::Value *) /builds/worker/checkouts/gecko/dom/bindings/BindingUtils.cpp:3327
#16 0x7ffb5c537548 in CallJSNative /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:472
#17 0x7ffb5c537548 in js::InternalCallOrConstruct(struct JSContext *, class JS::CallArgs const &, enum js::MaybeConstruct, enum js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:566
#18 0x7ffb5c5552af in InternalCall /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:633
#19 0x7ffb5c5552af in js::CallFromStack /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:638
#20 0x7ffb5c5552af in js::Interpret(struct JSContext *, class js::RunState &) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:3053
#21 0x7ffb5c536171 in MaybeEnterInterpreterTrampoline /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:386
#22 0x7ffb5c536171 in js::RunScript(struct JSContext *, class js::RunState &) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:444
#23 0x7ffb5c537688 in js::InternalCallOrConstruct(struct JSContext *, class JS::CallArgs const &, enum js::MaybeConstruct, enum js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:598
#24 0x7ffb5c53948b in InternalCall /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:633
#25 0x7ffb5c53948b in js::Call(struct JSContext *, class JS::Handle<class JS::Value>, class JS::Handle<class JS::Value>, class js::AnyInvokeArgs const &, class JS::MutableHandle<class JS::Value>, enum js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:665
#26 0x7ffb5ae8f378 in js::Call /builds/worker/checkouts/gecko/js/src/vm/Interpreter.h:116
#27 0x7ffb5ae8f378 in PromiseReactionJob /builds/worker/checkouts/gecko/js/src/builtin/Promise.cpp:2244
#28 0x7ffb5c537548 in CallJSNative /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:472
#29 0x7ffb5c537548 in js::InternalCallOrConstruct(struct JSContext *, class JS::CallArgs const &, enum js::MaybeConstruct, enum js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:566
#30 0x7ffb5c53948b in InternalCall /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:633
#31 0x7ffb5c53948b in js::Call(struct JSContext *, class JS::Handle<class JS::Value>, class JS::Handle<class JS::Value>, class js::AnyInvokeArgs const &, class JS::MutableHandle<class JS::Value>, enum js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:665
#32 0x7ffb5b169b15 in JS::Call(struct JSContext *, class JS::Handle<class JS::Value>, class JS::Handle<class JS::Value>, class JS::HandleValueArray const &, class JS::MutableHandle<class JS::Value>) /builds/worker/checkouts/gecko/js/src/vm/CallAndConstruct.cpp:119
#33 0x7ffb4fd21da6 in mozilla::dom::PromiseJobCallback::Call(class mozilla::dom::BindingCallContext &, class JS::Handle<class JS::Value>, class mozilla::ErrorResult &) /builds/worker/workspace/obj-build/dom/bindings/./PromiseBinding.cpp:83
#34 0x7ffb4b997f05 in mozilla::dom::PromiseJobCallback::Call /builds/worker/workspace/obj-build/dist/include/mozilla/dom/PromiseBinding.h:198
#35 0x7ffb4b997f05 in mozilla::dom::PromiseJobCallback::Call /builds/worker/workspace/obj-build/dist/include/mozilla/dom/PromiseBinding.h:211
#36 0x7ffb4b997f05 in mozilla::PromiseJobRunnable::Run(class mozilla::AutoSlowOperation &) /builds/worker/checkouts/gecko/xpcom/base/CycleCollectedJSContext.cpp:210
#37 0x7ffb4b9705ae in mozilla::CycleCollectedJSContext::PerformMicroTaskCheckPoint(bool) /builds/worker/checkouts/gecko/xpcom/base/CycleCollectedJSContext.cpp:673
#38 0x7ffb55413aec in mozilla::CycleCollectedJSContext::LeaveMicroTask /builds/worker/workspace/obj-build/dist/include/mozilla/CycleCollectedJSContext.h:246
#39 0x7ffb55413aec in mozilla::nsAutoMicroTask::~nsAutoMicroTask /builds/worker/workspace/obj-build/dist/include/mozilla/CycleCollectedJSContext.h:394
#40 0x7ffb55413aec in mozilla::dom::ScriptLoader::EvaluateScript(class nsIGlobalObject *, class JS::loader::ScriptLoadRequest *) /builds/worker/checkouts/gecko/dom/script/ScriptLoader.cpp:2760
#41 0x7ffb55411b62 in mozilla::dom::ScriptLoader::EvaluateScriptElement(class JS::loader::ScriptLoadRequest *) /builds/worker/checkouts/gecko/dom/script/ScriptLoader.cpp:2551
#42 0x7ffb5540807d in mozilla::dom::ScriptLoader::ProcessRequest(class JS::loader::ScriptLoadRequest *) /builds/worker/checkouts/gecko/dom/script/ScriptLoader.cpp:2193
#43 0x7ffb55403412 in mozilla::dom::ScriptLoader::ProcessInlineScript(class nsIScriptElement *, enum JS::loader::ScriptKind) /builds/worker/checkouts/gecko/dom/script/ScriptLoader.cpp:1434
#44 0x7ffb553eb0b2 in mozilla::dom::ScriptLoader::ProcessScriptElement(class nsIScriptElement *, class nsTAutoStringN<char16_t, 64> const &) /builds/worker/checkouts/gecko/dom/script/ScriptLoader.cpp:1015
#45 0x7ffb553ea0e4 in mozilla::dom::ScriptElement::MaybeProcessScript(void) /builds/worker/checkouts/gecko/dom/script/ScriptElement.cpp:195
#46 0x7ffb4db84118 in nsIScriptElement::AttemptToExecute /builds/worker/workspace/obj-build/dist/include/nsIScriptElement.h:223
#47 0x7ffb4db84118 in nsHtml5TreeOpExecutor::RunScript(class nsIContent *, bool) /builds/worker/checkouts/gecko/parser/html/nsHtml5TreeOpExecutor.cpp:957
#48 0x7ffb4db7eb65 in nsHtml5TreeOpExecutor::RunFlushLoop(void) /builds/worker/checkouts/gecko/parser/html/nsHtml5TreeOpExecutor.cpp:742
#49 0x7ffb4db8ec71 in nsHtml5ExecutorReflusher::Run(void) /builds/worker/checkouts/gecko/parser/html/nsHtml5TreeOpExecutor.cpp:83
#50 0x7ffb4bbd5d1e in mozilla::RunnableTask::Run(void) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:549
#51 0x7ffb4bbb7bc5 in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(class mozilla::detail::BaseAutoLock<class mozilla::Mutex &> const &) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:876
#52 0x7ffb4bbb3315 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(class mozilla::detail::BaseAutoLock<class mozilla::Mutex &> const &) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:699
#53 0x7ffb4bbb3f64 in mozilla::TaskController::ProcessPendingMTTask(bool) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:485
#54 0x7ffb4bbd9661 in mozilla::TaskController::TaskController::<lambda_5>::operator() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:211
#55 0x7ffb4bbd9661 in mozilla::detail::RunnableFunction<`lambda at /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:211:7'>::Run /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.h:548
#56 0x7ffb4bc096d5 in nsThread::ProcessNextEvent(bool, bool *) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1198
#57 0x7ffb4bc1a59a in NS_ProcessNextEvent(class nsIThread *, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:480
#58 0x7ffb4d30e117 in mozilla::ipc::MessagePump::Run(class base::MessagePump::Delegate *) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:85
#59 0x7ffb4d22b3e3 in MessageLoop::RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:370
#60 0x7ffb4d22b3e3 in MessageLoop::RunHandler(void) /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:363
#61 0x7ffb4d22b1aa in MessageLoop::Run(void) /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:345
#62 0x7ffb5596097c in nsBaseAppShell::Run(void) /builds/worker/checkouts/gecko/widget/nsBaseAppShell.cpp:148
#63 0x7ffb55b83527 in nsAppShell::Run(void) /builds/worker/checkouts/gecko/widget/windows/nsAppShell.cpp:824
#64 0x7ffb5a4dd9de in XRE_RunAppShell(void) /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:721
#65 0x7ffb4d22b3e3 in MessageLoop::RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:370
#66 0x7ffb4d22b3e3 in MessageLoop::RunHandler(void) /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:363
#67 0x7ffb4d22b1aa in MessageLoop::Run(void) /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:345
#68 0x7ffb5a4dcfc4 in XRE_InitChildProcess(int, char **const, struct XREChildData const *) /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:656
#69 0x7ff760ce2713 in content_process_main /builds/worker/checkouts/gecko/browser/app/../../ipc/contentproc/plugin-container.cpp:57
#70 0x7ff760ce2713 in NS_internal_main(int, char **, char **) /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:375
#71 0x7ff760ce14d8 in wmain /builds/worker/checkouts/gecko/toolkit/xre/nsWindowsWMain.cpp:151
#72 0x7ff760dc4337 in invoke_main D:\a_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:90
#73 0x7ff760dc4337 in __scrt_common_main_seh D:\a_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288
#74 0x7ffc13287613 (C:\WINDOWS\System32\KERNEL32.DLL+0x180017613)
#75 0x7ffc134a26b0 (C:\WINDOWS\SYSTEM32\ntdll.dll+0x1800526b0)

Address 0x2d73e07df008 is a wild pointer inside of access range of size 0x000000000008.
SUMMARY: AddressSanitizer: use-after-poison /builds/worker/workspace/obj-build/dist/include/js/HeapAPI.h:525 in js::gc::detail::GetTenuredGCThingZone
Shadow bytes around the buggy address:
0x2d73e07ded80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x2d73e07dee00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x2d73e07dee80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x2d73e07def00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x2d73e07def80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x2d73e07df000: 00[f7]00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x2d73e07df080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x2d73e07df100: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x2d73e07df180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x2d73e07df200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x2d73e07df280: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
==22712==ABORTING

Comment 1

[m.cooolie]

Attachment: log_ffp_asan_18556.log.22712.txt

Comment 2

[Andrew McCreight [:mccr8]]

This is probably an issue with DOM Streams rather than the GC rooting infrastructure.

Comment 3

[Andrew McCreight [:mccr8]]

Can you take a look, Kagami? Thanks.

Comment 4

[Daniel Veditz [:dveditz]]

Big long list of custom prefs (e.g. fuzzing functions are enabled and used to do the GC/CC) but I didn't see any that jumped out as things that would change how ReadableStreams work.

Comment 5

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: WIP: Bug 1861344

Comment 6

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: WIP: Bug 1861344 - Add a test

Depends on D192098

Comment 7

[Kagami Rosylight [:saschanaz] (they/them)]

So I can repro this by rooting the buffer before calling pull callback, but is that a requirement? Is JS::Heap not enough to grab the object?

Hello stream peers who I think know a lot better about the JS engine, any idea?

Comment 8

[Matthew Gaudet (he/him) [:mgaudet]]

Uhm. Something is messy here. Need to read more later, but it should be traced already... that root should be superflous

https://searchfox.org/mozilla-central/source/dom/streams/ReadableByteStreamController.h#201

Comment 9

[Andrew McCreight [:mccr8]]

I don't know anything about this code, but I'll say that having a refcounted object that is traced by some other object is dangerous.

Comment 10

[Kagami Rosylight [:saschanaz] (they/them)]

We do trace it that way somehow forever since bug 1741941, was there a good reason not to have the trace macro for ReadableByteStreamQueueEntry itself (and also for PullIntoDescriptor)?

Not like I understand why it's dangerous, but at least I can try fixing it if there's no good reason.

Comment 11

[Andrew McCreight [:mccr8]]

The issue in general is that if the owning object drops its reference to the traced object, but somehow the traced object is still alive, then you won't end up tracing the inner object. If an object is responsible for tracing its own references, then you don't have to worry about the lifetimes falling out of sync.

Comment 12

[Kagami Rosylight [:saschanaz] (they/them)]

Thanks! That makes sense.

BTW, ReadableByteStreamQueueEntry itself is a list that includes itself, that's not pretty at least as it's not expected to be a nested list AFAIK... (Edit: LinkedListElement, not List, 😅)

Comment 13

[Matthew Gaudet (he/him) [:mgaudet]]

I'm travelling, but would like to check this again -- I must have had a reason I did it this way, just don't remember.

Comment 14

[Kagami Rosylight [:saschanaz] (they/them)]

I filed bug 1861742 for that and submitted to try: https://treeherder.mozilla.org/jobs?repo=try&revision=c6a2321c82f5ba7949ce5fc0f1befb311a2bd9ac

It didn't fix this issue, though.

Comment 15

[Matthew Gaudet (he/him) [:mgaudet]]

Ok; I don't love the peculiar rooting change you've got in your patch. It doesn't quite feel right by principle, but it's too late for me to reason about, and I get on a plane tomorrow -- so early next week I would expect to think about this again.

Comment 16

[Kagami Rosylight [:saschanaz] (they/them)]

You mean the patch for this bug? Yup, I don't love it either, it works but I don't understand why and that's the reason I don't love it.

Comment 17

[Matthew Gaudet (he/him) [:mgaudet]]

Well, presumably the rooting being moved is keeping the buffer alive longer; but yes, there's no real principle behind it is also my feeling.

Comment 18

[Kagami Rosylight [:saschanaz] (they/them)]

Huh, the reason why bug 1861742 didn't fix it last week was because the patch was flawed, and now it does fix this issue. Should I hide that bug behind sec flag too for uplift purpose? 😅

Comment 19

[Andrew McCreight [:mccr8]]

(For bug bounty purposes: to summarize, the situation here is that bug 1861742 was spun off from this one as a speculative related cleanup, but it turned out to actually fix the issue here.)

Comment 20

[Daniel Veditz [:dveditz]]

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

Huh, the reason why bug 1861742 didn't fix it last week was because the patch was flawed, and now it does fix this issue. Should I hide that bug behind sec flag too for uplift purpose? 😅

discussion summary: since the "clean-up" patch is already checked in we'll just leave that public, but we'll track uplifts and advisory info here. We don't have a patch here to hook sec-approval requests on but we'll figure that out. We should wait until next week for the uplift, say Nov 8, but no later than Nov 9 so it can make the last beta.

Comment 21

[Matthew Gaudet (he/him) [:mgaudet]]

Ok, I'm very glad to see that Bug 1861742 did fix this -- that patch makes a lot more sense to me.

I wanted to see if I could recover my original reasoning for not tracing QueueEntries and PullInto descriptors themselves -- alas, I was unable to figure that out. My notes don't seem to cover it.

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

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

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox120 to wontfix.

For more information, please visit BugBot documentation.

Comment 23

[Daniel Veditz [:dveditz]]

We should wait until next week for the uplift, say Nov 8, but no later than Nov 9 so it can make the last beta.

Release Managers want this in now for more testing. I'm fine with that. sec-approval+ = dveditz for uplifting

Comment 24

[Kagami Rosylight [:saschanaz] (they/them)]

Uplift requests just happened on bug 1861742 👍

Comment 25

[Daniel Veditz [:dveditz]]

Attachment: advisory.txt

Comment 26

[Daniel Veditz [:dveditz]]

Bulk-unhiding security bugs fixed in Firefox 119-121 (Fall 2023). Use "moo-doctrine-subsidy" to filter