1739923 - (CVE-2022-22742) SEGV on unknown address 0x14141414 in nsTextFragment::CharAt

Status: VERIFIED FIXED

(Core :: DOM: Editor, task)

Description

[Irvan Kurniawan (:sourc7)]

Attachment: testcase.html

From the testcase after the insertParagraph executed, Firefox will crash with AV on 0x14141414 (32-bit) or 0x000014141414 (64-bit), the crash address is controllable through insertAdjacentText new line or space character length, on the testcase I'm using decimal 336860180 which converted to hex 0x14141414.

When execute insertAdjacentText to fill DOM with a lot of space character, the memory will increase, currently one new line or space character is equivalent to one SEGV address increase, it will be more interesting if there are workaround to optimize the memory to hit higher SEGV address.

Comment 1

[Irvan Kurniawan (:sourc7)]

Attachment: firefox-32-release.crashdump.txt

Comment 2

[Irvan Kurniawan (:sourc7)]

Attachment: firefox-64-nightly.crashdump.txt

Comment 3

[Andrew McCreight [:mccr8]]

The test case seems to involve some interaction of SVG and Editor, so I'll move it to editor for now.

I'm not really sure what is going on here, but getting the browser to crash on an address that is at least somewhat controllable sounds bad so I'll tentatively mark it sec-high.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

mozregression got me down to https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c68fe15a81fc2dc9fc5765f3be2573519c09b6c1&tochange=567a8768593eb06a86deb263f94d9de2d3d3e8fa

Bug 1642594 looks possibly relevant.

EDIT: Hrm, only on Windows apparently. In my Linux VM, this crashes basically since ever.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

In debug build, I got hitting asserts:

###!!! ASSERTION: The offset is out of bounds: '!mParent || mOffset.value() <= mParent->Length()', file m:/fx64-dbg/dist/include\mozilla/EditorDOMPoint.h:506
#01: NS_DebugBreak (m:\src\xpcom\base\nsDebugImpl.cpp:429)
#02: mozilla::AutoEditorDOMPointChildInvalidator::InvalidateChild (m:\fx64-dbg\dist\include\mozilla\EditorDOMPoint.h:1308)
#03: mozilla::WhiteSpaceVisibilityKeeper::MakeSureToKeepVisibleWhiteSpacesVisibleAfterSplit (m:\src\editor\libeditor\WSRunObject.cpp:2381)
#04: mozilla::WhiteSpaceVisibilityKeeper::PrepareToSplitBlockElement (m:\src\editor\libeditor\WSRunObject.cpp:103)
#05: mozilla::HTMLEditor::HandleInsertParagraphInListItemElement (m:\src\editor\libeditor\HTMLEditSubActionHandler.cpp:7435)

https://searchfox.org/mozilla-central/rev/6c8d325e61b0b445ed2e04899da38c3a4c266cba/editor/libeditor/EditorDOMPoint.h#505-506
https://searchfox.org/mozilla-central/rev/6c8d325e61b0b445ed2e04899da38c3a4c266cba/editor/libeditor/WSRunObject.cpp#2353,2375

Assertion failure: aPoint.IsSetAndValid(), at m:/src/editor/libeditor/WSRunObject.cpp:2497
#01: mozilla::WSRunScanner::TextFragmentData::GetPreviousEditableCharPoint<nsCOMPtr<nsINode>,nsCOMPtr<nsIContent> > (m:\src\editor\libeditor\WSRunObject.cpp:2497)
#02: mozilla::WhiteSpaceVisibilityKeeper::MakeSureToKeepVisibleWhiteSpacesVisibleAfterSplit (m:\src\editor\libeditor\WSRunObject.cpp:2383)
#03: mozilla::WhiteSpaceVisibilityKeeper::PrepareToSplitBlockElement (m:\src\editor\libeditor\WSRunObject.cpp:103)
#04: mozilla::HTMLEditor::HandleInsertParagraphInListItemElement (m:\src\editor\libeditor\HTMLEditSubActionHandler.cpp:7435)

https://searchfox.org/mozilla-central/rev/6c8d325e61b0b445ed2e04899da38c3a4c266cba/editor/libeditor/WSRunObject.cpp#2495,2497
https://searchfox.org/mozilla-central/rev/6c8d325e61b0b445ed2e04899da38c3a4c266cba/editor/libeditor/WSRunObject.cpp#2383-2384

It seems that the former is the root cause.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

There are 2 methods which do similar things so that we need to fix them too, but each method change should be landed separately into autoland for mozregressions because change around there may cause web-compat issues.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1739923 - part 1: Track split point at replacing collapsible white-spaces r=m_kato!

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1739923 - part 2: Make `WhiteSpaceVisibilityKeeper::InsertBRElement()` track insert position in smaller block r=m_kato!

And also invisible white-space range and replacing white-space position should
also be tracked too.

Depends on D130722

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1739923 - part 3: Make `WhiteSpaceVisibilityKeeper::ReplaceText()` track insert position in smaller block r=m_kato!

Similar to the previous patch, this patch also make it track invisible
white-space ranges and clear outdated things.

Depends on D130723

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1739923 - part 3: Make `WhiteSpaceVisibilityKeeper::ReplaceText()` track insert position in smaller block (for ESR 91) r=m_kato!

Due to the change of bug 1727844 and bug 1724650, only "part 3" cannot be
grafted cleanly.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9249922 [details]
Bug 1739923 - part 1: Track split point at replacing collapsible white-spaces r=m_kato!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: If the attacker knows how built-in editor handles white-space sequence, they may realize what's the problem.
• 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?: 91ESR
• If not all supported branches, which bug introduced the flaw?: Bug 1642594
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: part 1 and part 2 are grafted cleanly from mozilla-central. Only part 3 requires the new patch due to the change of bug 1727844 (93) and bug 1724650 (94).
• How likely is this patch to cause regressions; how much testing does it need?: Unfortunately, this could change the behavior of HTMLEditor when inserting text around white-spaces, deleting around white-spaces etc. I cannot say there are enough tests (WPT nor mochitests) for the text editing because there is crazy number of cases. So I recommend to land this as soon as possible for getting feedback from testers.

Comment 12

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

These were landed in Nightly in Bug 1740847 and will ride the trains.

Comment 13

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

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Done

Comment 15

[Daniel Bodea [:danibodea]]

Using the test case attached, the browser crashes in Release v95.0.2, and does not crash in Beta v96.0b7 or Nightly v97.0a1.
This was tested in Windows 10, Ubuntu 20.04.3 LTS and Mac OS 11.6.2.
Closing as verified fixed.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Fixed on ESR for 91.5esr.
https://bugzilla.mozilla.org/show_bug.cgi?id=1740847#c13

Comment 17

[Frederik Braun [:freddy]]

Attachment: advisory.txt

Comment 18

[Daniel Bodea [:danibodea]]

The fix for ESR v91.5.0esr was verified in Windows 10, Ubuntu 20.04.3 LTS and Mac OS 11.6.2. The crash no longer occurs when using the test case in comment 0.
Thanks!