1856695 - AddressSanitizer: heap-use-after-free [@ Hdr] with READ of size 8

Status: VERIFIED FIXED

(Core :: DOM: Device Interfaces, defect, P1)

Description

[Jason Kratzer [:jkratzer]]

Testcase found while fuzzing mozilla-central rev 53af11a26cb6 (built with: --enable-address-sanitizer --enable-fuzzing).

Testcase can be reproduced using the following commands:

$ pip install fuzzfetch grizzly-framework
$ python -m fuzzfetch --build 53af11a26cb6 --asan --fuzzing -n firefox
$ python -m grizzly.replay ./firefox/firefox testcase.html

AddressSanitizer: heap-use-after-free [@ Hdr] with READ of size 8

    =================================================================
    ==1101636==ERROR: AddressSanitizer: heap-use-after-free on address 0x51000003e3d8 at pc 0x7fd45c8f1e8e bp 0x7ffcd6c68da0 sp 0x7ffcd6c68d98
    READ of size 8 at 0x51000003e3d8 thread T0 (Isolated Web Co)
        #0 0x7fd45c8f1e8d in Hdr /builds/worker/workspace/obj-build/dist/include/nsTArray.h:587:51
        #1 0x7fd45c8f1e8d in Elements /builds/worker/workspace/obj-build/dist/include/nsTArray.h:1197:48
        #2 0x7fd45c8f1e8d in unsigned long nsTArray_Impl<mozilla::Observer<mozilla::void_t>*, nsTArrayInfallibleAllocator>::IndexOf<mozilla::Observer<mozilla::void_t>*, nsDefaultComparator<mozilla::Observer<mozilla::void_t>*, mozilla::Observer<mozilla::void_t>*>>(mozilla::Observer<mozilla::void_t>* const&, unsigned long, nsDefaultComparator<mozilla::Observer<mozilla::void_t>*, mozilla::Observer<mozilla::void_t>*> const&) const /builds/worker/workspace/obj-build/dist/include/nsTArray.h:1354:30
        #3 0x7fd45c8f235b in IndexOf<mozilla::Observer<mozilla::void_t> *> /builds/worker/workspace/obj-build/dist/include/nsTArray.h:1373:12
        #4 0x7fd45c8f235b in bool nsAutoTObserverArray<mozilla::Observer<mozilla::void_t>*, 0ul>::RemoveElement<mozilla::Observer<mozilla::void_t>*>(mozilla::Observer<mozilla::void_t>* const&) /builds/worker/workspace/obj-build/dist/include/nsTObserverArray.h:221:31
        #5 0x7fd45c8ef1d7 in RemoveObserver /builds/worker/workspace/obj-build/dist/include/mozilla/Observer.h:56:23
        #6 0x7fd45c8ef1d7 in RemovePortListener /dom/midi/MIDIAccess.cpp:240:25
        #7 0x7fd45c8ef1d7 in mozilla::dom::MIDIPort::~MIDIPort() /dom/midi/MIDIPort.cpp:54:24
        #8 0x7fd45c8eef1d in mozilla::dom::MIDIInput::~MIDIInput() /builds/worker/workspace/obj-build/dist/include/mozilla/dom/MIDIInput.h:28:24
        #9 0x7fd453373b66 in SnowWhiteKiller::~SnowWhiteKiller() /xpcom/base/nsCycleCollector.cpp:2473:7
        #10 0x7fd453372915 in nsCycleCollector::FreeSnowWhite(bool) /xpcom/base/nsCycleCollector.cpp:2663:3
        #11 0x7fd45337e6b7 in nsCycleCollector::BeginCollection(mozilla::CCReason, ccIsManual, nsICycleCollectorListener*) /xpcom/base/nsCycleCollector.cpp:3660:3
        #12 0x7fd45337d3d1 in nsCycleCollector::Collect(mozilla::CCReason, ccIsManual, js::SliceBudget&, nsICycleCollectorListener*, bool) /xpcom/base/nsCycleCollector.cpp:3484:9
        #13 0x7fd45337ccba in nsCycleCollector::ShutdownCollect() /xpcom/base/nsCycleCollector.cpp:3418:20
        #14 0x7fd453380066 in nsCycleCollector::Shutdown(bool) /xpcom/base/nsCycleCollector.cpp:3722:5
        #15 0x7fd453382760 in nsCycleCollector_shutdown(bool) /xpcom/base/nsCycleCollector.cpp:4046:18
        #16 0x7fd45362a9e2 in mozilla::ShutdownXPCOM(nsIServiceManager*) /xpcom/build/XPCOMInit.cpp:702:3
        #17 0x7fd464123f62 in XRE_InitChildProcess(int, char**, XREChildData const*) /toolkit/xre/nsEmbedFunctions.cpp:660:16
        #18 0x558f7481c16c in content_process_main /browser/app/../../ipc/contentproc/plugin-container.cpp:57:28
        #19 0x558f7481c16c in main /browser/app/nsBrowserApp.cpp:375:18
        #20 0x7fd47ab33d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
        #21 0x7fd47ab33e3f in __libc_start_main csu/../csu/libc-start.c:392:3
        #22 0x558f74740388 in _start (/home/jkratzer/builds/m-c-20231003093318-fuzzing-asan-opt/firefox+0xdc388) (BuildId: b7cff8be13666bb78088f1ee0fcebf9908f4c257)
    
    0x51000003e3d8 is located 152 bytes inside of 184-byte region [0x51000003e340,0x51000003e3f8)
    freed by thread T0 (Isolated Web Co) here:
        #0 0x558f747dbf46 in free /builds/worker/fetches/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:52:3
        #1 0x7fd453373b66 in SnowWhiteKiller::~SnowWhiteKiller() /xpcom/base/nsCycleCollector.cpp:2473:7
        #2 0x7fd453372915 in nsCycleCollector::FreeSnowWhite(bool) /xpcom/base/nsCycleCollector.cpp:2663:3
        #3 0x7fd453380059 in nsCycleCollector::Shutdown(bool) /xpcom/base/nsCycleCollector.cpp:3719:3
        #4 0x7fd453382760 in nsCycleCollector_shutdown(bool) /xpcom/base/nsCycleCollector.cpp:4046:18
        #5 0x7fd45362a9e2 in mozilla::ShutdownXPCOM(nsIServiceManager*) /xpcom/build/XPCOMInit.cpp:702:3
        #6 0x7fd464123f62 in XRE_InitChildProcess(int, char**, XREChildData const*) /toolkit/xre/nsEmbedFunctions.cpp:660:16
        #7 0x558f7481c16c in content_process_main /browser/app/../../ipc/contentproc/plugin-container.cpp:57:28
        #8 0x558f7481c16c in main /browser/app/nsBrowserApp.cpp:375:18
        #9 0x7fd47ab33d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    
    previously allocated by thread T0 (Isolated Web Co) here:
        #0 0x558f747dc1ee in malloc /builds/worker/fetches/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:69:3
        #1 0x558f748213f5 in moz_xmalloc /memory/mozalloc/mozalloc.cpp:52:15
        #2 0x7fd45c8ddfc7 in operator new /builds/worker/workspace/obj-build/dist/include/mozilla/cxxalloc.h:33:10
        #3 0x7fd45c8ddfc7 in mozilla::dom::MIDIAccessManager::CreateMIDIAccess(nsPIDOMWindowInner*, bool, mozilla::dom::Promise*) /dom/midi/MIDIAccessManager.cpp:149:24
        #4 0x7fd45c8e825e in Allow /dom/midi/MIDIPermissionRequest.cpp:90:8
        #5 0x7fd45c8e825e in mozilla::dom::MIDIPermissionRequest::Run() /dom/midi/MIDIPermissionRequest.cpp:101:7
        #6 0x7fd4535784ba in mozilla::RunnableTask::Run() /xpcom/threads/TaskController.cpp:559:16
        #7 0x7fd453562ab8 in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /xpcom/threads/TaskController.cpp:886:26
        #8 0x7fd45355f4c7 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /xpcom/threads/TaskController.cpp:709:15
        #9 0x7fd45355fda9 in mozilla::TaskController::ProcessPendingMTTask(bool) /xpcom/threads/TaskController.cpp:495:36
        #10 0x7fd45357ff91 in operator() /xpcom/threads/TaskController.cpp:218:37
        #11 0x7fd45357ff91 in mozilla::detail::RunnableFunction<mozilla::TaskController::TaskController()::$_0>::Run() /xpcom/threads/nsThreadUtils.h:548:5
        #12 0x7fd4535a9f93 in nsThread::ProcessNextEvent(bool, bool*) /xpcom/threads/nsThread.cpp:1198:16
        #13 0x7fd4535b7bfa in NS_ProcessNextEvent(nsIThread*, bool) /xpcom/threads/nsThreadUtils.cpp:480:10
        #14 0x7fd4551bfd9e in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /ipc/glue/MessagePump.cpp:85:21
        #15 0x7fd454fea80a in RunInternal /ipc/chromium/src/base/message_loop.cc:370:10
        #16 0x7fd454fea80a in RunHandler /ipc/chromium/src/base/message_loop.cc:363:3
        #17 0x7fd454fea80a in MessageLoop::Run() /ipc/chromium/src/base/message_loop.cc:345:3
        #18 0x7fd45e6774f9 in nsBaseAppShell::Run() /widget/nsBaseAppShell.cpp:148:27
        #19 0x7fd46412498e in XRE_RunAppShell() /toolkit/xre/nsEmbedFunctions.cpp:721:20
        #20 0x7fd454fea80a in RunInternal /ipc/chromium/src/base/message_loop.cc:370:10
        #21 0x7fd454fea80a in RunHandler /ipc/chromium/src/base/message_loop.cc:363:3
        #22 0x7fd454fea80a in MessageLoop::Run() /ipc/chromium/src/base/message_loop.cc:345:3
        #23 0x7fd464123f33 in XRE_InitChildProcess(int, char**, XREChildData const*) /toolkit/xre/nsEmbedFunctions.cpp:656:34
        #24 0x558f7481c16c in content_process_main /browser/app/../../ipc/contentproc/plugin-container.cpp:57:28
        #25 0x558f7481c16c in main /browser/app/nsBrowserApp.cpp:375:18
        #26 0x7fd47ab33d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    
    SUMMARY: AddressSanitizer: heap-use-after-free /builds/worker/workspace/obj-build/dist/include/nsTArray.h:587:51 in Hdr
    Shadow bytes around the buggy address:
      0x51000003e100: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
      0x51000003e180: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
      0x51000003e200: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
      0x51000003e280: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 fa
      0x51000003e300: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
    =>0x51000003e380: fd fd fd fd fd fd fd fd fd fd fd[fd]fd fd fd fa
      0x51000003e400: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
      0x51000003e480: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa
      0x51000003e500: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
      0x51000003e580: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 fa
      0x51000003e600: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
    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
    ==1101636==ABORTING

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: Testcase

Comment 2

[Chris Martin [:cmartin]]

NI Gabriele, since it's MIDI related.

Comment 3

[Chris Martin [:cmartin]]

(Sorry about that, figured I'd let Gabriele assign it to himself if he wants to)

Comment 4

[Bugmon [:jkratzer for issues]]

Unable to reproduce bug 1856695 using build mozilla-central 20231003093318-53af11a26cb6. Without a baseline, bugmon is unable to analyze this bug.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 5

[Gabriele Svelto [:gsvelto]]

So IIUC we have a MIDI port that's being destroyed and it holds a reference to the MIDIAccess object, but the later was already destroyed. This is definitely a situation where we shouldn't be in.

Comment 6

[Gabriele Svelto [:gsvelto]]

I think I know what's going on: we never disconnect the MIDI ports from the access manager when the latter is destroyed. This is odd because it's a pretty big oversight, so I'm checking if this isn't a regression.

Comment 7

[Gabriele Svelto [:gsvelto]]

Comment 6 is wrong, we do disconnect the ports and the flow happens through the destructor of the MIDIAccessManager object so in theory this shouldn't happen... unless we're hitting a race of some kind.

Comment 8

[Gabriele Svelto [:gsvelto]]

There's only one condition I can think of which could lead us into this situation: if we have a port list with a non-zero number of ports. Then we update the port list and send an empty one. At that point we should still have some MIDIPort objects around but none of them would reference the MIDIAccess object anymore. However I can't seem to reproduce the issue locally, not with the STR in comment 0 nor with a local build. I suspect that this might be because my machine has some MIDI ports available.

Comment 9

[Andrew McCreight [:mccr8]]

I'll assume this is a sec-high. If this is somehow specific to shutdown in a weird way, maybe it could get lowered.

Comment 10

[Gabriele Svelto [:gsvelto]]

Attachment: WIP: Bug 1856695 - Improve the MIDI objects lifecycle

Comment 11

[Gabriele Svelto [:gsvelto]]

Jason, I'm unable to reproduce the issue locally and I suspect it's down to the setup of my machine VS the setup of the fuzzer with regards to the number of MIDI ports available. Could you test if the patch in attachment 9356617 [details] solves the problem? Alternatively is there a way to run the testcase in automation in a way that reproduces the fuzzer's setup?

Comment 12

[Andrew McCreight [:mccr8]]

Is there some test case Jason could run on the machines that can reproduce the UAF that would would help you understand how to better reproduce it? Also I wonder if there's some way fuzzing could be improved to get coverage of a different number of MIDI ports or what have you.

Comment 13

[Jason Kratzer [:jkratzer]]

Bugmon was missing the prefs required to trigger the bug. I've updated it and re-enabled it for this bug. We should have a pernosco session for it shortly. In the meantime, I'll test out your patch.

Comment 14

[Jason Kratzer [:jkratzer]]

Regarding your ability to reproduce it, could you check to see if you have the latest versions of grizzly-framework and prefpicker? Both can be installed via pip.

Comment 15

[Jason Kratzer [:jkratzer]]

I can still reproduce this issue with the supplied patch in comment 10.

Comment 16

[Gabriele Svelto [:gsvelto]]

Thanks, so it must be a race somewhere else. I'll try reproducing with the prefs applied once they're available.

Comment 17

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

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:gsvelto, could you consider increasing the severity of this security bug?

For more information, please visit BugBot documentation.

Comment 18

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20231005092542-97d412cf7a31.
Unable to bisect testcase (Testcase reproduces on start build!):

Start: b240ded22d2669e57be192ee17c9384288fc6354 (20221006041355)
End: 53af11a26cb69e0616d9f481f988276e0a6da93e (20231003093318)
BuildFlags: BuildFlags(asan=True, tsan=False, debug=False, fuzzing=True, coverage=False, valgrind=False, no_opt=False, fuzzilli=False, nyx=False)

Successfully recorded a pernosco session. A link to the pernosco session will be added here shortly.

Comment 19

[Bugmon [:jkratzer for issues]]

A pernosco session for this bug can be found here.

Comment 20

[Bobby Holley (:bholley)]

(In reply to Bugmon [:jkratzer for issues] from comment #19)

A pernosco session for this bug can be found here.

As an aside — That is a fantastic use of tooling. Thanks to everyone who was involved in making this work.

Comment 21

[Gabriele Svelto [:gsvelto]]

(In reply to Bobby Holley (:bholley) from comment #20)

As an aside — That is a fantastic use of tooling. Thanks to everyone who was involved in making this work.

Yeah, this is fantastic. Having a pernosco session ready can turn a 1-2 weeks of trial-and-error into a matter of hours, or sometimes minutes.

Comment 22

[Gabriele Svelto [:gsvelto]]

I haven't yet figured out what's going on here but the Pernosco session is shedding some clarity on what might be happening: it seems that we reach a point where the MIDIAccess object has a non-zero reference count, but all the references are in a loop, so the cycle collector gets rid of it before it gets rid of the ports. So there must be an error not in how many time we reference the MIDIAccess object but rather in what other objects hold references to it.

Comment 23

[Gabriele Svelto [:gsvelto]]

OK, I figured out what's going wrong: both here and here we create the MIDI ports and store them in a raw pointer. If port creation fails we return a nullptr both here and here but crucially we don't delete the object we have built. This would just leak the objects under normal conditions, causing some memory consumption, but crucially these objects are cycle-collected... so later on the cycle-collector gets to them and tries to destroy the ports. The MIDIAccess object they were supposed to be attached to is long dead (and didn't have references to them... because we failed creating them!), so accessing it causes the UAF.

Comment 24

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1856695 - Improve how we handle MIDI port creation r=padenot

Comment 25

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1856695 - Drop a few useless includes r=padenot

Depends on D190487

Comment 26

[Gabriele Svelto [:gsvelto]]

Comment on attachment 9357461 [details]
Bug 1856695 - Improve how we handle MIDI port creation r=padenot

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The patch fixes the consequences of the MIDIPort creation failure, but this happens in a different function than where the error that triggers this bug occurs. So it's not obvious that the condition can be triggered by the current document not being present at the moment of creation. Since there's no mention of how the issue is triggered (the leaked port being destroyed by he cycle-collector) it should be hard to figure it out without extensive knowledge of our codebase.
• 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 older supported branches are affected by this flaw?: esr115
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: The patch applies cleanly to ESR115 after grafting the patches from bug 1851847 and bug 1851829, both of which fix issues that occur in similar scenarios as the one fixed by this patch.
• How likely is this patch to cause regressions; how much testing does it need?: The affected code is well covered by tests and thus not very likely to cause regressions
• Is Android affected?: No

Comment 27

[Tom Ritter [:tjr]]

Comment on attachment 9357461 [details]
Bug 1856695 - Improve how we handle MIDI port creation r=padenot

Approved to request uplift and land

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7d324930546
Improve how we handle MIDI port creation r=padenot
https://hg.mozilla.org/integration/autoland/rev/dd39c7f4404d
Drop a few useless includes r=padenot

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f7d324930546
https://hg.mozilla.org/mozilla-central/rev/dd39c7f4404d

Comment 30

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

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

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

For more information, please visit BugBot documentation.

Comment 31

[Gabriele Svelto [:gsvelto]]

Comment on attachment 9357461 [details]
Bug 1856695 - Improve how we handle MIDI port creation r=padenot

Beta/Release Uplift Approval Request

• User impact if declined: Malicious content can crash the browser, since the crash is a use-after-free access this makes it potentially dangerous.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: Bug 1851847, Bug 1851829
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch affects code that is used by a very small minority of our users and it's well covered with tests
• String changes made/needed: none
• Is Android affected?: No

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Malicious content can crash the browser, since the crash is a use-after-free access this makes it extremely dangerous.
• Fix Landed on Version: 120
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch affects code that is used by a very small minority of our users and it's well covered with tests. Additionally this patch grafts cleanly to the ESR115 branch once bug 1851847 and bug 1851829 have also been applied (both also fix crashes discovered via fuzzing).

Comment 32

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20231011092203-bbe69c1ce114.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 33

[Donal Meehan [:dmeehan]]

Comment on attachment 9357461 [details]
Bug 1856695 - Improve how we handle MIDI port creation r=padenot

Approved for 119.0b9

Comment 34

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/04369975d6c3
https://hg.mozilla.org/releases/mozilla-beta/rev/f10952f166f5

Comment 35

[Donal Meehan [:dmeehan]]

Comment on attachment 9357461 [details]
Bug 1856695 - Improve how we handle MIDI port creation r=padenot

Approved for 115.4esr.

Comment 36

[Pulsebot]

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

Comment 37

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-esr115/rev/196e5959d8d4
Backed out changeset de9c4b028631 (bug 1856695) with 1851729 and 185147 for causing crashtests failures
https://treeherder.mozilla.org/jobs?repo=mozilla-esr115&selectedTaskRun=LMiilIeWRE-SwK-iGYD0ag.0&searchStr=crashtest&revision=122fd292adeb25eede6a3fd37b77dd1100fa5b60&group_state=expanded

Comment 38

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Bug 1851829 for esr115

This is the patch for bug 1851829 modified to remove the preference that triggers the assertion on esr115.

Comment 39

[Pulsebot]

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

Comment 40

[Daniel Veditz [:dveditz]]

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