1988931 - (CVE-2025-11708) AddressSanitizer: heap-use-after-free on MediaTrackGraphImpl::GetInstance in add->isRemoved

Status: RESOLVED FIXED

(Core :: Audio/Video: MediaStreamGraph, defect)

Description

[Irvan Kurniawan [:sourc7]]

Attachment: testcase1.html

When run .canvas.captureStream() repeatedly to canvas that has media <video src> then call location.reload() after an interval, then after a seconds Firefox ASan will crash with SUMMARY: AddressSanitizer: heap-use-after-free /builds/worker/workspace/obj-build/dist/include/mozilla/HashTable.h:1182:35 in isRemoved

Tested on:

• Firefox Nightly 145.0a1 (2025-09-16) (64-bit)
• Firefox ESR 140.4.0esr (64-bit)

Steps to reproduce:

1. Open Firefox ASan build
   2 Visit attached testcase1.html
2. After a seconds, Firefox tab will crash with SUMMARY: AddressSanitizer: heap-use-after-free in isRemoved

Comment 1

[Irvan Kurniawan [:sourc7]]

Attachment: asan_log_nightly.txt

Comment 2

[Irvan Kurniawan [:sourc7]]

Attachment: asan_log_esr140.4.txt

Comment 3

[Andrew McCreight [:mccr8]]

Summary of the free stack:

[mozilla::Graphs() gets resized]
[unlinking a MediaStreamTrack triggers MediaTrackGraphImpl::RemoveTrack]
[finishing an existing cycle collection in nsCycleCollector::FinishAnyCurrentCollection]
[triggering a garbage collection in GCRuntime::collect]
[allocating a new JS object in XPCWrappedNative::Init]
[calling into an XPCOM object implemented in JS using XPConnect]
    #39 0x7f0d12c856b1 in mozilla::MediaTrackGraphImpl::AddShutdownBlocker() /builds/worker/checkouts/gecko/dom/media/MediaTrackGraph.cpp:1842:26
    #40 0x7f0d12c95cec in mozilla::MediaTrackGraphImpl::Init(mozilla::MediaTrackGraph::GraphDriverType, mozilla::MediaTrackGraph::GraphRunType, unsigned int) /builds/worker/checkouts/gecko/dom/media/MediaTrackGraph.cpp:3474:35
    #41 0x7f0d12c8c52f in mozilla::MediaTrackGraphImpl::GetInstance(mozilla::MediaTrackGraph::GraphDriverType, unsigned long, int, void const*, nsISerialEventTarget*) /builds/worker/checkouts/gecko/dom/media/MediaTrackGraph.cpp:3590:10
    #42 0x7f0d1297832c in mozilla::dom::CanvasCaptureMediaStream::Init(mozilla::dom::Optional<double> const&, nsIPrincipal*) /builds/worker/checkouts/gecko/dom/media/CanvasCaptureMediaStream.cpp:173:28

Then looking at the use stack:

    #2 0x7f0d12c8ca07 in add<mozilla::MediaTrackGraphImpl *&> /builds/worker/workspace/obj-build/dist/include/mozilla/HashTable.h:629:18
    #3 0x7f0d12c8ca07 in mozilla::MediaTrackGraphImpl::GetInstance(mozilla::MediaTrackGraph::GraphDriverType, unsigned long, int, void const*, nsISerialEventTarget*) /builds/worker/checkouts/gecko/dom/media/MediaTrackGraph.cpp:3591:3
    #4 0x7f0d1297832c in mozilla::dom::CanvasCaptureMediaStream::Init(mozilla::dom::Optional<double> const&, nsIPrincipal*) /builds/worker/checkouts/gecko/dom/media/CanvasCaptureMediaStream.cpp:173:28
    #5 0x7f0d1276e866 in mozilla::dom::HTMLCanvasElement::CaptureStream(mozilla::dom::Optional<double> const&, nsIPrincipal&, mozilla::ErrorResult&) /builds/worker/checkouts/gecko/dom/html/HTMLCanvasElement.cpp:892:25

The use is happening here. The issue is that we're using addPtr which came from Graphs()::lookupForAdd, but in the meanwhile we've mutated Graphs. It looks like you could use relookupOrAdd here to save rehashing but honestly it doesn't feel like this code can be so performance sensitive that this would matter, given that we're calling into JS.

It also doesn't make me feel good that we have a raw pointer to a MediaTrackGraphImpl on the stack here while we're doing a GC/CC, but I think in this specific situation it is safe because we don't addref or release it until after the GC/CC is done, so the CC is never aware of it.

There is a second callsite for MediaTrackGraphImpl::Init in MediaTrackGraphImpl::CreateNonRealtimeInstance. This has the raw pointer on the stack issue that I do not like, but it does not keep a Lookup object alive so it shouldn't have the same UAF problem as the test case.

Comment 5

[Andrew McCreight [:mccr8]]

jimm noted in bug 1983288 that there are some crashes in the wild that look like this. They aren't on poison values so I wouldn't have recognized them.

Comment 7

[Karl Tomlinson (:karlt)]

Gecko avoids releasing references from JS reflector object to C++ objects during JS GC, but nsCycleCollector::CollectWhite() can also release references to C++ objects (via Unlink() at least) and that is being run here during JS GC.

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: (secure)

Comment 9

[Karl Tomlinson (:karlt)]

nsIAsyncShutdownClient methods are JS_HAZ_CAN_RUN_SCRIPT, but AFAIK we can't mark the Ptr JS_HAZ_GC_INVALIDATED for these HashSet template parameters.
Perhaps we could add another type parameter and store an instance on Ptr in DEBUG builds.

Comment 10

[Karl Tomlinson (:karlt)]

Since https://hg.mozilla.org/mozilla-central/rev/a1d1cccb0f7fddf117ebdd1221d329be9bd03c56#l1.124

Comment 11

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1860954

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: (secure)

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 9513919 [details]
(secure)

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The testcase on bug 1988931 suggests that this would not be hard to exploit, but getting from the patch to an exploit would need something like wide understanding of code interactions or code coverage and fuzzing.
• 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?: 121 and subsequent. flags are correct
• If not all supported branches, which bug introduced the flaw?: Bug 1860954
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: Same patch should apply to esr140.
• How likely is this patch to cause regressions; how much testing does it need?: Low risk.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 14

[Tom Ritter [:tjr]]

Comment on attachment 9513919 [details]
(secure)

Approved to land and request uplift

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/fe6c46f32891
https://hg.mozilla.org/integration/autoland/rev/519981a066e9
use GetInstanceIfExists() from MediaTrackGraphImpl::GetInstance() r=pehrsons

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/519981a066e9

Comment 17

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:karlt, 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-firefox144 to wontfix.

For more information, please visit BugBot documentation.

Comment 18

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: sec-high
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: Small change that is closer to how the code was before https://hg.mozilla.org/mozilla-central/rev/a1d1cccb0f7fddf117ebdd1221d329be9bd03c56#l1.99
  Many tests use this code.
• String changes made/needed: None
• Is Android affected?: yes

Comment 19

[Karl Tomlinson (:karlt)]

Attachment: (secure)

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

Comment 20

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined: sec-high
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: Small change that is closer to how the code was before https://hg.mozilla.org/mozilla-central/rev/a1d1cccb0f7fddf117ebdd1221d329be9bd03c56#l1.99
  Many tests use this code.
• String changes made/needed: None.
• Is Android affected?: yes

Comment 21

[Karl Tomlinson (:karlt)]

Attachment: (secure)

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

Comment 22

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/28e11a377d90
https://hg.mozilla.org/releases/mozilla-beta/rev/c173513b3f7d

Comment 23

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/18789f17f2fe

Comment 24

[Daniel Veditz [:dveditz]]

Attachment: advisory.txt

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/8e36c005d00e
https://hg.mozilla.org/integration/autoland/rev/7653c9cf2ed9
Test MediaTrackGraph release during GetInstance r=pehrsons

Comment 26

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

https://hg.mozilla.org/mozilla-central/rev/7653c9cf2ed9