1920381 - (CVE-2024-9936) High Address SEGV in SelectionNodeCache::MaybeCollect() from PaintTextWithSelection

Status: VERIFIED FIXED

(Core :: DOM: Selection, defect)

Description

[Atte Kettunen]

Attachment: SEGV-nown-operator-HasChildren-NextNode-mozillaContentIteratorBasensINodeNext.html

Tested on:

OS: Ubuntu 22.04
Firefox: fuzzfetch fetched:
Identified task: https://firefox-ci-tc.services.mozilla.com/api/index/v1/task/gecko.v2.mozilla-central.latest.firefox.linux64-asan-opt

Task ID: fL6HkB9ZRQ2U2MIiBJx_ag
Rank: 1726996556
Changeset: f51fed24a523285f52651d636da8782050b1c8e7
Build ID: 20240922091556

Debug build reports:
[1336886] Hit MOZ_CRASH(assertion failed: distance_to_stack_bottom <= max_allowable_distance) at servo/components/style/context.rs:609

ASAN-output snippet (from release ASAN-build):
==1326486==ERROR: AddressSanitizer: SEGV on unknown address (pc 0x7fffe1a83e80 bp 0x7fffffff5af0 sp 0x7fffffff5ae0 T0)
==1326486==The signal is caused by a READ memory access.
==1326486==Hint: this fault was caused by a dereference of a high value address (see register values below). Disassemble the provided pc to learn which register was used.
#0 0x7fffe1a83e80 in operator bool /builds/worker/workspace/obj-build/dist/include/nsCOMPtr.h:767:45
#1 0x7fffe1a83e80 in HasChildren /builds/worker/checkouts/gecko/dom/base/nsINode.h:672:39
#2 0x7fffe1a83e80 in NextNode /builds/worker/checkouts/gecko/dom/base/ContentIterator.cpp:694:15
#3 0x7fffe1a83e80 in mozilla::ContentIteratorBase<nsINode*>::Next() /builds/worker/checkouts/gecko/dom/base/ContentIterator.cpp:798:14
#4 0x7fffe1d3e419 in operator() /builds/worker/checkouts/gecko/dom/base/Selection.cpp:343:35
#5 0x7fffe1d3e419 in OrInsertWith<(lambda at /builds/worker/checkouts/gecko/dom/base/Selection.cpp:329:56)> /builds/worker/checkouts/gecko/xpcom/ds/nsBaseHashtable.h:726:23
#6 0x7fffe1d3e419 in operator()<nsBaseHashtable<nsPtrHashKey<const mozilla::dom::Selection>, nsTBaseHashSet<nsPtrHashKey<const nsINode> >, nsTBaseHashSet<nsPtrHashKey<const nsINode> >, nsDefaultConverter<nsTBaseHashSet<nsPtrHashKey<const nsINode> >, nsTBaseHashSet<nsPtrHashKey<const nsINode> > > >::EntryHandle> /builds/worker/checkouts/gecko/xpcom/ds/nsBaseHashtable.h:423:26
#7 0x7fffe1d3e419 in operator()<nsTHashtable<nsBaseHashtableET<nsPtrHashKey<const mozilla::dom::Selection>, nsTBaseHashSet<nsPtrHashKey<const nsINode> > > >::EntryHandle> /builds/worker/checkouts/gecko/xpcom/ds/nsBaseHashtable.h:836:18
#8 0x7fffe1d3e419 in operator()<PLDHashTable::EntryHandle> /builds/worker/checkouts/gecko/xpcom/ds/nsTHashtable.h:437:18
#9 0x7fffe1d3e419 in WithEntryHandle<(lambda at /builds/worker/checkouts/gecko/xpcom/ds/nsTHashtable.h:436:9)> /builds/worker/checkouts/gecko/xpcom/ds/PLDHashTable.h:605:12
#10 0x7fffe1d3e419 in WithEntryHandle<(lambda at /builds/worker/checkouts/gecko/xpcom/ds/nsBaseHashtable.h:835:15)> /builds/worker/checkouts/gecko/xpcom/ds/nsTHashtable.h:434:25
#11 0x7fffe1d3e419 in WithEntryHandle<(lambda at /builds/worker/checkouts/gecko/xpcom/ds/nsBaseHashtable.h:422:34)> /builds/worker/checkouts/gecko/xpcom/ds/nsBaseHashtable.h:834:18
#12 0x7fffe1d3e419 in LookupOrInsertWith<(lambda at /builds/worker/checkouts/gecko/dom/base/Selection.cpp:329:56)> /builds/worker/checkouts/gecko/xpcom/ds/nsBaseHashtable.h:422:12
#13 0x7fffe1d3e419 in mozilla::dom::SelectionNodeCache::MaybeCollect(mozilla::dom::Selection const*) /builds/worker/checkouts/gecko/dom/base/Selection.cpp:329:25
#14 0x7fffe1d4dcd3 in MaybeCollectNodesAndCheckIfFullySelected /builds/worker/checkouts/gecko/dom/base/Selection.h:94:12
#15 0x7fffe1d4dcd3 in mozilla::dom::Selection::LookUpSelection(nsIContent*, unsigned int, unsigned int, mozilla::UniquePtr<SelectionDetails, mozilla::DefaultDelete<SelectionDetails>>, mozilla::SelectionType, bool) /builds/worker/checkouts/gecko/dom/base/Selection.cpp:2016:16
#16 0x7fffe83e5baf in nsFrameSelection::LookUpSelection(nsIContent*, int, int, bool) const /builds/worker/checkouts/gecko/layout/generic/nsFrameSelection.cpp:1511:30
#17 0x7fffe860a446 in GetSelectionDetails /builds/worker/checkouts/gecko/layout/generic/nsTextFrame.cpp:4761:57
#18 0x7fffe860a446 in nsTextFrame::PaintTextWithSelection(nsTextFrame::PaintTextSelectionParams const&, nsTextFrame::ClipEdges const&) /builds/worker/checkouts/gecko/layout/generic/nsTextFrame.cpp:6484:41
#19 0x7fffe860d082 in nsTextFrame::PaintText(nsTextFrame::PaintTextParams const&, int, int, nsPoint const&, bool, float) /builds/worker/checkouts/gecko/layout/generic/nsTextFrame.cpp:6865:9
#20 0x7fffe89a49e9 in mozilla::nsDisplayText::RenderToContext(gfxContext*, mozilla::nsDisplayListBuilder*, nsRect const&, float, bool) /builds/worker/checkouts/gecko/layout/painting/nsDisplayList.cpp:7656:6
#21 0x7fffe89a5435 in mozilla::nsDisplayText::CreateWebRenderCommands(mozilla::wr::DisplayListBuilder&, mozilla::wr::IpcResourceUpdateQueue&, mozilla::layers::StackingContextHelper const&, mozilla::layers::RenderRootStateManager*, mozilla::nsDisplayListBuilder*) /builds/worker/checkouts/gecko/layout/painting/nsDisplayList.cpp:7578:3
#22 0x7fffe0967ddb in mozilla::layers::WebRenderCommandBuilder::CreateWebRenderCommands(mozilla::nsDisplayItem*, mozilla::wr::DisplayListBuilder&, mozilla::wr::IpcResourceUpdateQueue&, mozilla::layers::StackingContextHelper const&, mozilla::nsDisplayListBuilder*) /builds/worker/checkouts/gecko/gfx/layers/wr/WebRenderCommandBuilder.cpp:1859:41
#23 0x7fffe0965cbd in mozilla::layers::WebRenderCommandBuilder::CreateWebRenderCommandsFromDisplayList(mozilla::nsDisplayList*, mozilla::nsDisplayItem*, mozilla::nsDisplayListBuilder*, mozilla::layers::StackingContextHelper const&, mozilla::wr::DisplayListBuilder&, mozilla::wr::IpcResourceUpdateQueue&, bool) /builds/worker/checkouts/gecko/gfx/layers/wr/WebRenderCommandBuilder.cpp:2115:7
#24 0x7fffe898a5ba in CreateWebRenderCommandsNewClipListOption /builds/worker/checkouts/gecko/layout/painting/nsDisplayList.cpp:4605:30
....

Comment 1

[Atte Kettunen]

Attachment: SEGV-nown-operator-HasChildren-NextNode-mozillaContentIteratorBasensINodeNext.log

Full ASAN-output.

Comment 2

[Andrew McCreight [:mccr8]]

Emilio, what does the Stylo assertion means? Sounds like it could be some kind of stack overflow issue? Thanks.

Comment 3

[Andrew McCreight [:mccr8]]

The selection part of the stack looks similar to bug 1913849, except that was getting called from reflow instead of nsTextFrame::PaintText.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Yeah I think it indicates our stack depth checks are going wrong, but it shouldn't impact correctness.

Comment 5

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20240923142214-f47709f0fa48.
The bug appears to have been introduced in the following build range:

Start: 7862376f4564170615e7e5d4c3d25c4b5a493420 (20240805122647)
End: 3b521f0c4aa78411bf17d1ee2e7b53150e53acff (20240805161158)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7862376f4564170615e7e5d4c3d25c4b5a493420&tochange=3b521f0c4aa78411bf17d1ee2e7b53150e53acff

Comment 6

[Andrew McCreight [:mccr8]]

Apparently we're using an invalid static range so that sounds bad. I'll mark it sec-high. jjaschke said the fix is easy.

Comment 7

[Jan Jaeschke [:jjaschke]]

The cache I introduced in bug 1867249, which collects all selected nodes of all selections, didn’t check if a static range was valid or not before iterating over it. A StaticRange being invalid is a legit situation (as can be seen in the attached test case). Ignoring invalid static ranges when populating the cache solves the issue here.

Comment 8

[Jan Jaeschke [:jjaschke]]

Attachment: Bug 1920381 - Added missing sanity checks before inserting ranges into cache. r=sefeng!

Comment 9

[Jan Jaeschke [:jjaschke]]

Comment on attachment 9427354 [details]
Bug 1920381 - Added missing sanity checks before inserting ranges into cache. r=sefeng!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The patch only contains some sanity-checks. However, the attached test case points to the exploit. I will land the test case later in a separate commit.
• 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?: The regressing commit landed in the 131 branch.
• 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?: The patch can be applied easily.
• How likely is this patch to cause regressions; how much testing does it need?: The patch only adds sanity checks. I don't expect regressions, I also don't think it requires additional testing.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 10

[Tom Ritter [:tjr]]

Comment on attachment 9427354 [details]
Bug 1920381 - Added missing sanity checks before inserting ranges into cache. r=sefeng!

approved to land and uplift

Comment 11

[Jan Jaeschke [:jjaschke]]

Attachment: Bug 1920381 - Added missing sanity checks before inserting ranges into cache. r=sefeng!

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

Comment 12

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Security issue
• Code covered by automated testing: no
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: Run html attached to the bug. If it doesn't crash, it's fixed.
• Risk associated with taking this patch: The patch only adds sanity checks.
• Explanation of risk level: Low risk - only adds sanity checks.
• String changes made/needed: No
• Is Android affected?: yes

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f9aebc5a440
Added missing sanity checks before inserting ranges into cache. r=sefeng

Comment 14

[BugBot [:suhaib / :marco]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7f9aebc5a440

Comment 16

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/4d51176c728b

Comment 17

[Bugmon [:jkratzer for issues]]

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

Comment 18

[Dianna Smith [:diannaS]]

Do we want to include this in the planned dot release for 131?

Comment 19

[Jan Jaeschke [:jjaschke]]

If there's a dot release planned, we can include this. The risk is quite low.

Comment 20

[Dianna Smith [:diannaS]]

:jjaschke thanks, can you create an uplift request?

Comment 21

[Jan Jaeschke [:jjaschke]]

Attachment: Bug 1920381 - Added missing sanity checks before inserting ranges into cache. r=sefeng!

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

Comment 22

[Phabricator Automation]

release Uplift Approval Request

• User impact if declined: Security issue
• Code covered by automated testing: no
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: Run html file attached to the bug. If it doesn't crassh, it's fixed.
• Risk associated with taking this patch: The patch only adds sanity checks.
• Explanation of risk level: Low risk - only adds sanity checks.
• String changes made/needed: No
• Is Android affected?: yes

Comment 23

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-release/rev/61386f62d6ae

Comment 24

[Raluca Popovici, Desktop QA]

I was able to reproduce this bug(crash occurs) on Ubuntu 22 and Ubuntu 24.10 using Fx 131.0.3 (treeherder build) and Fx 132.0b6.
I notice that the bug was fixed only on Nightly 133.oa1.
Should I reopen this bug? Is there any additional step(changed component or something) for Fx 131.0.3 and Fx 132.0b6?

Comment 25

[Jan Jaeschke [:jjaschke]]

I can repro the crash on Ubuntu with 131.0.2 (latest Snap version), but not with 131.0.3.

Did the page actually crash, or did the tab just close? (Unfortunately, the test case has a window.close() at its end, I should remove that)

Comment 26

[Jan Jaeschke [:jjaschke]]

Attachment: Test Case without window.close()

Comment 27

[Raluca Popovici, Desktop QA]

Thank you for clarifying. That was strange because the HTML worked as expected in Nightly 133.0a1(the message "One" was displayed, and the tab was not closed).
Now, I re-tested on Ubuntu 24.10 using Fx 131.0.3 and Fx 132.0b6, and the bug is fixed(the last time I verified, the tab was just closed but only in these versions, and now the message "One" is displayed and no crash occurs).
Based on comment 25 and with the test case from comment 26, I will mark this bug as verified on Fx 131.0.3.