1943179 - (CVE-2025-1414) Poison crash in [@ nsINode::HasChildren]

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/60b991eb-967d-483a-9280-71a3d0250119

Reason:

SIGSEGV / SI_KERNEL

Top 10 frames:

0  libxul.so  nsCOMPtr<nsIContent>::operator bool() const  xpcom/base/nsCOMPtr.h:767
0  libxul.so  nsINode::HasChildren() const  dom/base/nsINode.h:672
0  libxul.so  mozilla::ContentIteratorBase<nsINode*>::NextNode(nsINode*)  dom/base/ContentIterator.cpp:694
0  libxul.so  mozilla::ContentIteratorBase<nsINode*>::Next()  dom/base/ContentIterator.cpp:798
0  libxul.so  mozilla::dom::SelectionNodeCache::MaybeCollect(mozilla::dom::Selection const*...  dom/base/Selection.cpp:350
0  libxul.so  nsBaseHashtable<nsPtrHashKey<mozilla::dom::Selection const>, nsTBaseHashSet<n...  xpcom/ds/nsBaseHashtable.h:733
0  libxul.so  nsBaseHashtable<nsPtrHashKey<mozilla::dom::Selection const>, nsTBaseHashSet<n...  xpcom/ds/nsBaseHashtable.h:430
0  libxul.so  nsBaseHashtable<nsPtrHashKey<mozilla::dom::Selection const>, nsTBaseHashSet<n...  xpcom/ds/nsBaseHashtable.h:843
0  libxul.so  nsTHashtable<nsBaseHashtableET<nsPtrHashKey<mozilla::dom::Selection const>, n...  xpcom/ds/nsTHashtable.h:438
0  libxul.so  _ZN12PLDHashTable15WithEntryHandleIZN12nsTHashtableI17nsBaseHashtableETI12nsP...  xpcom/ds/PLDHashTable.h:605

jjaschke noticed that a number of these crashes are on poison values. The crashes are probably all the same issue but given that the other one is fairly old I figured we could have a new bug.

Comment 1

[Andrew McCreight [:mccr8]]

The poison crashes aren't particularly common. There are 914 crashes with this signature in the last month, and only 37 of them are on poison values. Only about a half dozen of crashes with this signature didn't have SelectionNodeCache in the proto signature.

The correlations tab also shows a heavy correlation with the German locale:
(79.55% in signature vs 10.18% overall) useragent_locale = de

A lot of comments on this bug talk about using the some kind of monitoring page for a FritzBox, which appears to be a popular German brand a routers and modems. I've translated most of the comments below using Firefox from German, but I can get the original comments to jjaschke if that would help.

For instance, on poison crashes:
bp-f1d5c215-ce55-4d5e-a8f1-4eaa70250107

I had just been in my FritzBox 6690 Cable.

bp-d246aec2-563f-4bc8-8822-891fe0241226

In FritzBox 7590 AX Version 8.00 Web interface under Map tab online monitor. In the Single Devices view while changing the display period from 2 months back to 1 day and then 10 minutes. The tab is now the 2. crashed in a row.

bp-962236a8-f537-41c5-99f9-21cbf0241224 (this didn't translate into anything useful)

info led belegt - absturz

Lots more similar comments from crashes not on poison values:
bp-27a5b253-6159-455e-b88e-fc9920250105

was in the Fritz Box Menu

bp-84b9321b-4352-42e4-884c-a046d0241227

On a Fritzbox fiber router 5530 by avm.de, I switched the timescale from 24 hours to 2 months (Internet -> Online-Monitor)

bp-12e0238f-77cc-4eed-959c-6171e0241126

In the online monitor when switching from 1 day to one hours

bp-3ab94128-1cb6-4a97-ae92-464550241121

InternetOnline Monitor/Single Devices Crash when changing tab from 10 minutes to 2 months

bp-9a824982-7a7f-4992-a0d9-8f3c90241107

Change monitor from one hour to day

bp-2ce807eb-a3f2-4358-a4c6-55c1d0241101

Switch from 10min to 1day in online monitor probably not having enough data stored (update 10min before)

bp-8d243b26-396d-4ecb-92ac-ee1980241007

When switching to "2 months"

Lots of URLs like https://fritz.box/#/online-monitor.

This is looking back at 6 months of crashes so maybe some of these are for an issue that was already fixed? URLs for test cases from bug 1920381 show up a few times.

Comment 2

[Andrew McCreight [:mccr8]]

Jan, could you take a look? Thanks.

Comment 3

[Jan Jaeschke [:jjaschke]]

Attachment: Bug 1943179 - Add `[[nodiscard]]` to `ContentIterator` methods that return `nsresult`. r=masayuki!

Comment 4

[Jan Jaeschke [:jjaschke]]

UnsafePreContentIterator::Init() failed, which left UnsafePreContentIterator::mCurNode uninitialized. The return value of Init() was not checked, therefore the content iterator started iterating on random addresses or poison.
This patch default-initializes (raw) pointers in the content iterator classes and adds [[nodiscard]] to Init() to ensure that the return value isn't ignored.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

And the issue was found by reading the code. No testcase for this issue, but https://bugzilla.mozilla.org/show_bug.cgi?id=1920381 is basically the same bug.

Comment 6

[Jan Jaeschke [:jjaschke]]

Comment on attachment 9461391 [details]
Bug 1943179 - Add [[nodiscard]] to ContentIterator methods that return nsresult. r=masayuki!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It is an uninitialized pointer, which could point to anything (null, poison, ...). We could not find out how to reproduce the crash though.
• 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?: beta, release
• If not all supported branches, which bug introduced the flaw?: Bug 1867249
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Fix is small and not complex. The patches should apply to the other branches, and if not it's trivial to create backports.
• How likely is this patch to cause regressions; how much testing does it need?: The patch only ensures that the ContentIterator objects are initialized correctly. I don't expect regressions. I did not run tests on Try though.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 7

[Tom Ritter [:tjr] (one week until extended leave)]

Comment on attachment 9461391 [details]
Bug 1943179 - Add [[nodiscard]] to ContentIterator methods that return nsresult. r=masayuki!

Approved to land. When/after landing please request uplift - we're targeting this for the 135.0.1 dot release, not 135.0.0

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/afeecd52c4a7
Add `[[nodiscard]]` to `ContentIterator` methods that return `nsresult`. r=masayuki

Comment 9

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

Backed out for bustage in FilteredContentIterator.cpp:
https://hg.mozilla.org/integration/autoland/rev/4cdc358a2a4e6f0b87b73b41a125aaa76e7cd147

Push with bustage
Build log

[task 2025-01-28T16:10:31.155Z] 16:10:31    ERROR -  /builds/worker/checkouts/gecko/editor/spellchecker/FilteredContentIterator.cpp(310,11): error: ignoring return value of function declared with 'nodiscard' attribute [-Werror,-Wunused-result]
[task 2025-01-28T16:10:31.155Z] 16:10:31     INFO -    310 |           mCurrentIterator->PositionAt(content);
[task 2025-01-28T16:10:31.156Z] 16:10:31     INFO -        |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b76896b3f58
Add `[[nodiscard]]` to `ContentIterator` methods that return `nsresult`. r=masayuki

Comment 11

[Jan Jaeschke [:jjaschke]]

Comment on attachment 9461391 [details]
Bug 1943179 - Add [[nodiscard]] to ContentIterator methods that return nsresult. r=masayuki!

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: Uninitialized pointers. Might crash, unsure how to reproduce.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch just default-initializes members and adds [[nodiscard]] annotations to functions.
• String changes made/needed:
• Is Android affected?: Yes

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/4b76896b3f58

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9461391 [details]
Bug 1943179 - Add [[nodiscard]] to ContentIterator methods that return nsresult. r=masayuki!

This is targeting the 135.0.1 release; moving the request accordingly.

Comment 14

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

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

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

For more information, please visit BugBot documentation.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9461391 [details]
Bug 1943179 - Add [[nodiscard]] to ContentIterator methods that return nsresult. r=masayuki!

Approved for 135.0.1

Comment 16

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-release/rev/ae08499d7dcd