1986816 - (CVE-2025-11721) Crash in [@ mozilla::storage::StatementCache<T>::CreateStatement]

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[BugBot [:suhaib / :marco]]

Crash report: https://crash-stats.mozilla.org/report/index/fa1afc59-79bc-4d0b-a7d2-8339a0250830

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  xul.dll  nsCOMPtr<mozIStorageConnection>::get const  xpcom/base/nsCOMPtr.h:751
0  xul.dll  nsCOMPtr<mozIStorageConnection>::operator-> const  xpcom/base/nsCOMPtr.h:772
0  xul.dll  mozilla::storage::StatementCache<mozIStorageStatement>::CreateStatement  storage/StatementCache.h:103
1  xul.dll  mozilla::storage::StatementCache<mozIStorageStatement>::GetCachedStatement  storage/StatementCache.h:53
1  xul.dll  mozilla::places::ConcurrentConnection::GetStatementOnHelperThread  toolkit/components/places/ConcurrentConnection.cpp:421
2  xul.dll  mozilla::places::ConnectionAdapter::GetStatement const  toolkit/components/places/FaviconHelpers.h:282
2  xul.dll  mozilla::places::  toolkit/components/places/FaviconHelpers.cpp:438
3  xul.dll  mozilla::places::AsyncGetFaviconForPageRunnable::Run  toolkit/components/places/FaviconHelpers.cpp:943
4  xul.dll  nsThread::ProcessNextEvent  xpcom/threads/nsThread.cpp:1151
4  xul.dll  NS_ProcessNextEvent  xpcom/threads/nsThreadUtils.cpp:461

By querying Nightly crashes reported within the last 2 months, here are some insights about the signature:

• First crash report: 2025-08-07
• Process type: Parent
• Is startup crash: No
• Has user comments: No
• Is null crash: Yes - 1 out of 8 crashes happened on null or near null memory address
• Is use after free crash: Yes - 5 out of 8 crashes happened on or near an allocator poison value

Comment 1

[Daniel Veditz [:dveditz]]

The earliest build I see is a 143 Nightly build 20250804213332 bp-29f295aa-530c-4038-8a7e-945a40250807 (ignoring an ESR-128 crash on a null-dereference and a different stack).

I don't see any storage changes right before then, but I do see bug 599980 doing something in places code with favicons, both of which show up in the stack.

Does that seem likely, Yazan and Mak?

Comment 2

[BugBot [:suhaib / :marco]]

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

Comment 3

[Marco Bonardo [:mak] (Away 10-21 August)]

GetStatementOnHelperThread should probably check mIsConnectionReady before trying to use mHelperThreadStatements.
GetCachedStatement should probably hold sharedAsyncExecutionMutex, because it looks like the connection is being teared down. <= this is not necessary, StatementCache doesn't own the connection and relies on the consumer to properly handle teardown.

Comment 4

[BugBot [:suhaib / :marco]]

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

For more information, please visit BugBot documentation.

Comment 5

[Marco Bonardo [:mak] (Away 10-21 August)]

Attachment: (secure)

Comment 6

[Marco Bonardo [:mak] (Away 10-21 August)]

The patch should be fine, though I'm seeing various hangs on shutdown in some b-c tests. I suspect it's Bug 1987725 and not my changes, but I would like to double check.

Comment 7

[BugBot [:suhaib / :marco]]

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

Comment 8

[Donal Meehan [:dmeehan]]

:mak, do you need to find a different reviewer for this?
The planned Fx143 dot release overlaps with your PTO. Wondering if this will be able to get a review, sec-approval, uplift requests etc. in time.

Comment 9

[Marco Bonardo [:mak] (Away 10-21 August)]

Comment on attachment 9512450 [details]
(secure)

Security Approval Request

• How easily could an exploit be constructed based on the patch?: This can only happen at specific times during startup/shutdown of the app, and may depend on thread scheduling as a helper thread is accessing a member setup by the main thread.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: beta, release
• If not all supported branches, which bug introduced the flaw?: Bug 599980
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: The patch should apply cleanly
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely, existing CI should be sufficient.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: No

Comment 10

[Marco Bonardo [:mak] (Away 10-21 August)]

(In reply to Donal Meehan [:dmeehan] from comment #8)

:mak, do you need to find a different reviewer for this?
The planned Fx143 dot release overlaps with your PTO. Wondering if this will be able to get a review, sec-approval, uplift requests etc. in time.

I will keep in touch with Chris Bellini about this during PTO, cc-ed him to the bug so eventually other members of the Search team can handle it.
I don't think it's an high risk for Release as it doesn't look like an easily exploitable vector, though sec team has a lot more experience than me on this matter.

Comment 11

[Donal Meehan [:dmeehan]]

(In reply to Marco Bonardo [:mak] (away 22-26 Sept) from comment #10)

(In reply to Donal Meehan [:dmeehan] from comment #8)

:mak, do you need to find a different reviewer for this?
The planned Fx143 dot release overlaps with your PTO. Wondering if this will be able to get a review, sec-approval, uplift requests etc. in time.

I will keep in touch with Chris Bellini about this during PTO, cc-ed him to the bug so eventually other members of the Search team can handle it.
I don't think it's an high risk for Release as it doesn't look like an easily exploitable vector, though sec team has a lot more experience than me on this matter.

Thanks, there is also some crash volume here

Comment 12

[Tom Ritter [:tjr]]

Comment on attachment 9512450 [details]
(secure)

Approved to land and request uplift

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/cc9b8254d95b
https://hg.mozilla.org/integration/autoland/rev/94edf612803b
r=places-reviewers,Standard8

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/67129b68ccab
https://hg.mozilla.org/integration/autoland/rev/09db749b7a05
Revert "Bug 1986816 r=places-reviewers,Standard8" for causing mochitest assertions @ nsThreadManager.cpp

Comment 15

[Marco Bonardo [:mak] (Away 10-21 August)]

I'm not sure why my multiple anonymous try builds didn't show any of that...
Anyway, the assertion shows there's more complexity than expected here.

We initially assumed ConcurrentConnection was created on the main-thread and accessed on either the MT or the helper thread (And this was trye until the regressing bug). It looks like now there's cases where it may be created on the helper thread, and that may actually be necessary (the helper thread must be able to register requests that will be handled later).
We must ensure the object can be created on either thread, and the main-thread stuff must be dispatched to the main-thread, some members must be wrapped by nsMainThreadPtrHandle. And I must check all the other properties are thread-safe.

I filed bug 1989632 to manage all of that.

In the meanwhile, what wer can do is just fail to return an instance if we would try to create one off the main-thread.
That means some favicon requests on startup may fail (return no favicon), but we don't have a better option short term to stop the crash.

Comment 16

[Marco Bonardo [:mak] (Away 10-21 August)]

there's still issues with other unexpected assertions (actually I think it's https://bugzilla.mozilla.org/show_bug.cgi?id=1947444), I'm not sure I can make this for 143, too many unknowns that I don't like.

Comment 17

[BugBot [:suhaib / :marco]]

The bug is marked as tracked for firefox143 (release) and tracked for firefox144 (beta). However, the bug still has low severity.

:cbellini, could you please increase the severity for this tracked bug? If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Comment 18

[BugBot [:suhaib / :marco]]

The bug is linked to a topcrash signature, which matches the following criterion:

• Top 20 desktop browser crashes on release

For more information, please visit BugBot documentation.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Hi Marco, any updates on this bug? It remains a pretty high-volume crash on Release and 144 is going to RC next week.

Comment 20

[Marco Bonardo [:mak] (Away 10-21 August)]

I have updated the patch and re-requested review.
There is still a downside that until we fix Bug 1989632 some early startup favicon requests may fail. Fixing that bug should solve it, but I prefer to not do that here to avoid taking more risks.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/ecf9614c118f
https://hg.mozilla.org/integration/autoland/rev/d04db31e303f
r=places-reviewers,Standard8

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/d04db31e303f

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Please request Beta uplift on this. We'll keep an eye on the crash stats over the weekend before hopefully including it in the RC builds on Monday.

Comment 24

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: Top Crash
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: medium
• Explanation of risk level: While this should avoid the crash it will cause some early startup favicon requests to fail.
• String changes made/needed: no
• Is Android affected?: no

Comment 25

[Marco Bonardo [:mak] (Away 10-21 August)]

Attachment: (secure)

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

Comment 26

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/5b061525f486
https://hg.mozilla.org/releases/mozilla-beta/rev/83227b7d7d1a