1896555 - (CVE-2024-5696) Crash SIGSEGV GetCaretFrameForNodeOffset at 0xffffffff (32-bit) or 0x0000ffffffff (64-bit) after malloc failure on nsTextFragment::SetTo

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Irvan Kurniawan [:sourc7]]

Attachment: log_minidump_00.txt

When run testcase with fillMemory and setRangeText, on Firefox official Nightly 32-bit build I notice it able to crash on GetCaretFrameForNodeOffset at 0xffffffff on 32-bit build or 0x0000ffffffff 64-bit build, the crash occur after malloc failure on nsTextFragment::SetTo.

On original testcase I able to crash at GetCaretFrameForNodeOffset as attached log_minidump_00.txt, but it still require many tries to crash reliably, have to tweak the fillMemory code. For now to reproduce the crash reliably we can simulate the malloc failure on nsTextFragment::SetTo and visit the testcase-for-patch.html.

On local build I able to crash as described after checkout on commit
Bug 1860328 - Track nsCaret position at the DOM level

Steps to reproduce:

1. Apply nsTextFragment-setTo-failure.patch
2. Compile Firefox
3. Visit attached testcase-for-patch.html
4. Firefox crash on GetCaretFrameForNodeOffset

Comment 1

[Irvan Kurniawan [:sourc7]]

Attachment: testcase-for-patch.html

Comment 2

[Irvan Kurniawan [:sourc7]]

Attachment: nsTextFragment-SetTo-failure.patch

Comment 3

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

:sefeng, since you are the author of the regressor, bug 1860328, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 4

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

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

Comment 5

[Irvan Kurniawan [:sourc7]]

Attachment: log_minidump_64bit.txt

Comment 6

[Sean Feng [:sefeng211]]

Ah, based on the description, this relates to tracking caret position.

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1896555 - Fix nsTextFragment OOM handling. r=#dom-core,smaug

Don't release the text eagerly until we know we have a buffer to put the
new text in.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

I can't remove the graphics-core-security tag, and if I add dom-core-security then I restrict who can see it to people that are on both groups... Can someone remove gfx-core-security and add dom-core-security?

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Not really a regression from that bug. I mean, it is in that particular test-case, but the bug was latent and pretty sure it could be made to crash elsewhere :)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f37c7158ec4
Fix nsTextFragment OOM handling. r=smaug

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/0f37c7158ec4

Comment 12

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

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

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

For more information, please visit BugBot documentation.

Comment 13

[Emilio Cobos Álvarez [:emilio]]

Not sure it's worth uplifting, wdyt?

Comment 14

[Irvan Kurniawan [:sourc7]]

Attachment: SEGV_FindChar_0x01312d14.txt

(In reply to Emilio Cobos Álvarez (:emilio) from comment #9)

Not really a regression from that bug. I mean, it is in that particular test-case, but the bug was latent and pretty sure it could be made to crash elsewhere :)

Thanks Emilio for the patch!

Yes, it's able to crash elsewhere.

I found new testcase, it crash on FindChar code, interestingly I able to control the SEGV address through setRangeText(String.fromCodePoint(990630).repeat(x), the crash address is controllable through the x number.

This crash is also fixed by patch on comment 7.

I'll share the testcase soon.

Comment 15

[Irvan Kurniawan [:sourc7]]

Attachment: minidump_0x01414144.txt

Comment 16

[Irvan Kurniawan [:sourc7]]

Attachment: testcase.FindChar.0x01414144.html

Ok, here the new testcase, the crash address is controllable through crashAddress variable, in this testcase I use 10526871 it will crash at 0x01414144.

However since it use setRangeText and fromCodePoint.repeat more higher the number, more slower to crash the Firefox.

Steps to reproduce:

1. Download the nsTextFragment-setTo-failure.patch
2. Change the runCount == 452 to runCount == 20 (or lower than 30)
3. Apply the patch then compile Firefox
4. Visit attached testcase.FindChar.0x01414144.html
5. After couple of seconds, Firefox crash at 0x01414144

Comment 17

[Daniel Veditz [:dveditz]]

Upgrading to sec-moderate based on the new testcase showing PC control.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #13)

Not sure it's worth uplifting, wdyt?

Actually we'd like this uplifted just in case someone can build on this and figure out how to trigger the OOM at the right time in a stock Firefox.

Comment 18

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9402156 [details]
Bug 1896555 - Fix nsTextFragment OOM handling. r=#dom-core,smaug

Beta/Release Uplift Approval Request

• User impact if declined: Potential security issue in case of OOM.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• 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): Relatively well scoped patch, but might need a rebase to account for nsStringBuffer -> StringBuffer rename. Should be trivial tho.
• String changes made/needed: none
• Is Android affected?: Yes

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: See above.
• User impact if declined: see above
• Fix Landed on Version: 128
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Very straight-forward patch to code that's very exercised on regular conditions.

Comment 19

[Donal Meehan [:dmeehan]]

Comment on attachment 9402156 [details]
Bug 1896555 - Fix nsTextFragment OOM handling. r=#dom-core,smaug

Rejecting release uplift request since we don't ship secbugs in dot releases.
This will ride the train with Fx127

Comment 20

[Pascal Chevrel:pascalc]

Emilio, the patch doesn't apply to the beta and esr115 branches as I think the work in bug 1892257 conflicts. Could you provide a rebased patch? Thanks

Comment 21

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1896555 - Fix nsTextFragment OOM handling.

Don't release the text eagerly until we know we have a buffer to put the
new text in.

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

Comment 22

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1896555 - Fix nsTextFragment OOM handling.

Don't release the text eagerly until we know we have a buffer to put the
new text in.

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

Comment 23

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: comment 0
• Code covered by automated testing: no
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: low
• Explanation of risk level: trivial change to oom handling code which is otherwise exercised very often.
• String changes made/needed: none
• Is Android affected?: yes

Comment 24

[Phabricator Automation]

esr115 Uplift Approval Request

• User impact if declined: comment 0
• Code covered by automated testing: no
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: low
• Explanation of risk level: See other patch and bug.
• String changes made/needed: none
• Is Android affected?: yes

Comment 25

[Emilio Cobos Álvarez [:emilio]]

Provided rebased patches via phab, lmk if that doesn't work for some reason?

Comment 26

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/5f1c9266e7ef

Comment 27

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/5ff085122aa8

Comment 28

[Tom Ritter [:tjr] (OOTO until April)]

Attachment: advisory.txt