Closed Bug 1856695 Opened 1 year ago Closed 1 year ago

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

Categories

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

Product:
Core
Component:
DOM: Device Interfaces
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
120 Branch
Tracking Flags:
Tracking Status
firefox-esr115 119+ fixed
firefox118 --- wontfix
firefox119 + fixed
firefox120 + verified

People

(Reporter: jkratzer, Assigned: gsvelto)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [bugmon:bisected,confirmed][adv-main119+r][adv-ESR115.4+r])

Attachments

(4 files, 1 obsolete file)

Testcase
415 bytes, text/plain
Details
Bug 1856695 - Improve how we handle MIDI port creation r=padenot
48 bytes, text/x-phabricator-request
dmeehan
: approval-mozilla-beta+
dmeehan
: approval-mozilla-esr115+
tjr
: sec-approval+
Details | Review
Bug 1856695 - Drop a few useless includes r=padenot
48 bytes, text/x-phabricator-request
Details | Review
[PATCH] Bug 1851829 for esr115
12.13 KB, text/plain
Details

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
Attached file Testcase —
Group: core-security → dom-core-security
Keywords: csectype-uaf

NI Gabriele, since it's MIDI related.

Assignee: nobody → gsvelto
Severity: -- → S3
Status: NEW → ASSIGNED
Flags: needinfo?(gsvelto)
Priority: -- → P1
Assignee: gsvelto → nobody
Status: ASSIGNED → NEW

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

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.

Keywords: bugmon

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.

Flags: needinfo?(gsvelto)
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED

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 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.

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.

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

Keywords: sec-high

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?

Flags: needinfo?(jkratzer)

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.

Flags: needinfo?(gsvelto)

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.

Flags: needinfo?(jkratzer)
Keywords: bugmon, pernosco-wanted

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.

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

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

Flags: needinfo?(gsvelto)

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.

Flags: needinfo?(gsvelto)

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.

Keywords: pernosco-wanted → pernosco
Whiteboard: [bugmon:confirm] → [bugmon:bisected,confirmed]

A pernosco session for this bug can be found here.

(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.

(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.

Flags: needinfo?(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.

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.

Attachment #9356617 - Attachment is obsolete: true

Depends on D190487

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
Attachment #9357461 - Flags: sec-approval?

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

Approved to request uplift and land

Attachment #9357461 - Flags: sec-approval? → sec-approval+
Pushed by gsvelto@mozilla.com: 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

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.

Flags: needinfo?(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).
Flags: needinfo?(gsvelto)
Attachment #9357461 - Flags: approval-mozilla-esr115?
Attachment #9357461 - Flags: approval-mozilla-beta?

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.

Status: RESOLVED → VERIFIED
status-firefox120: fixed → verified
Keywords: bugmon
Group: dom-core-security → core-security-release

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

Approved for 119.0b9

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

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

Approved for 115.4esr.

Attachment #9357461 - Flags: approval-mozilla-esr115? → approval-mozilla-esr115+

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

Flags: needinfo?(gsvelto)
Whiteboard: [bugmon:bisected,confirmed] → [bugmon:bisected,confirmed][adv-main119+r]
Whiteboard: [bugmon:bisected,confirmed][adv-main119+r] → [bugmon:bisected,confirmed][adv-main119+r][adv-ESR115.4+r]

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

Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: