1915535 - (CVE-2025-0238) AddressSanitizer: heap-use-after-free on nsLineBreaker::FlushCurrentWord() after malloc failure

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect, P2)

Description

[Irvan Kurniawan [:sourc7]]

Attachment: minidump_0x838d084d.txt

After run document.execCommand("insertLineBreak") and setRangeText in loop, when malloc failure on nsLineBreaker::FlushCurrentWord() in if (!breakState.AppendElements(length, mozilla::fallible)) { Firefox able to crash with SIGSEGV/SEGV_ACCERR with changing address, sometimes SIGILL/ILL_ILLOPN with changing top stack, and ASan heap-use-after-free on nsLineBreaker::FlushCurrentWord() with READ of size 4 (32-bit) and READ of size 8 (64-bit) ASan build

Tested on:

• Firefox 129.0.2 (32-bit)
• Firefox Nightly 131.0a1 (2024-08-28) (32-bit)

Steps to reproduce:

1. Apply attached nsLineBreaker-FlushCurrentWord.patch
2. Compile Firefox
3. Visit attached testcase.forpatch.html
4. Firefox crash with SIGSEGV / SEGV_ACCERR or AddressSanitizer: heap-use-after-free on ASan build

Comment 1

[Irvan Kurniawan [:sourc7]]

Attachment: minidump_0x6573752d.txt

Comment 2

[Irvan Kurniawan [:sourc7]]

Attachment: minidump_sigill_0xebfdc845.txt

Comment 3

[Irvan Kurniawan [:sourc7]]

Attachment: nsLineBreaker-FlushCurrentWord.patch

Comment 4

[Irvan Kurniawan [:sourc7]]

Attachment: testcase.forpatch.html

Comment 5

[Irvan Kurniawan [:sourc7]]

Attachment: log_ffp_asan_546144.log.552434.txt

Comment 6

[Irvan Kurniawan [:sourc7]]

Attachment: log_ffp_asan_1188513.log.1277779.txt

Comment 7

[Andrew McCreight [:mccr8]]

nsLineBreaker is under dom/ but it looks mostly related to text, so I'll put it in Layout.

I'm marking this sec-moderate because it requires an OOM. If you can demonstrate a reliable test case without patching Firefox, we could probably raise it to sec-high.

Comment 8

[Irvan Kurniawan [:sourc7]]

(In reply to Andrew McCreight [:mccr8] from comment #7)

I'm marking this sec-moderate because it requires an OOM. If you can demonstrate a reliable test case without patching Firefox, we could probably raise it to sec-high.

Thanks, I originally able to reproduce this on Firefox Nightly 32-bit official build, however the testcase is still intermittent, I'll try to improve the testcase reliability to hit SEGV_ACCERR crash.

I also able to reproduce this on Firefox 115.14.0esr (32-bit).

Comment 9

[Irvan Kurniawan [:sourc7]]

Attachment: minidump_esr115.txt

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Let's start with S2, seems worth fixing.

Comment 11

[Jonathan Kew [:jfkthame]]

The problem here occurs because if we bail out of FlushCurrentWord due to an allocation failure (which could happen for either the breakState or capitalizationState arrays), we don't just fail to set up break positions and/or capitalization -- which is the expected result of the failure -- we also miss the final "cleanup" of state that's supposed to happen at the end of FlushCurrentWord.

That doesn't immediately cause a problem, but it does leave the mTextItems array holding pointers that were supposed to be cleared, and are about to be freed. So then by the next time we call FlushCurrentWord those items are no longer valid, and we crash trying to access them.

The fix should be to ensure that we always clear the mTextItems array when FlushCurrentWord finishes, even if an error occurred so that we weren't able to set up breaks.

After doing that, the testcase here just happily infinite-loops, as expected. (Presumably it will eventually OOM in some other place, as it is endlessly adding content...)

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1915535 - Ensure consistent cleanup in nsLineBreaker::FlushCurrentWord. r=#layout

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/10af4f9659db
Ensure consistent cleanup in nsLineBreaker::FlushCurrentWord. r=layout-reviewers,emilio

Comment 14

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

Backed out for non-unified build bustage on nsLineBreaker.cpp:
https://hg.mozilla.org/integration/autoland/rev/03d28e6bd1f816bcdadefd0f0f9de55af1dbdc6d

Push with failures
Build log

[task 2024-12-02T18:24:07.272Z] 18:24:07    ERROR -  /builds/worker/checkouts/gecko/dom/base/nsLineBreaker.cpp:154:18: error: use of undeclared identifier 'MakeScopeExit'; did you mean 'mozilla::MakeScopeExit'?
[task 2024-12-02T18:24:07.273Z] 18:24:07     INFO -    154 |   auto cleanup = MakeScopeExit([&] {
[task 2024-12-02T18:24:07.274Z] 18:24:07     INFO -        |                  ^~~~~~~~~~~~~
[task 2024-12-02T18:24:07.274Z] 18:24:07     INFO -        |                  mozilla::MakeScopeExit
[task 2024-12-02T18:24:07.274Z] 18:24:07     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/ScopeExit.h:119:39: note: 'mozilla::MakeScopeExit' declared here
[task 2024-12-02T18:24:07.274Z] 18:24:07     INFO -    119 | [[nodiscard]] ScopeExit<ExitFunction> MakeScopeExit(
[task 2024-12-02T18:24:07.274Z] 18:24:07     INFO -        |                                       ^

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac1f0158c2f2
Ensure consistent cleanup in nsLineBreaker::FlushCurrentWord. r=layout-reviewers,emilio

Comment 16

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

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

Comment 17

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

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

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

For more information, please visit BugBot documentation.

Comment 18

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1915535 - Ensure consistent cleanup in nsLineBreaker::FlushCurrentWord. r=#layout

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

Comment 19

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: potential use-after-free after a memory-allocation failure
• Code covered by automated testing: no
• Fix verified in Nightly: no
• 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: Just ensures early-return codepaths don't skip required cleanup
• String changes made/needed: none
• Is Android affected?: yes

Comment 20

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/53ea167def36

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR128 and ESR115 approval. It'll need a minor bit of rebasing for ESR115.

Comment 22

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1915535 - Ensure consistent cleanup in nsLineBreaker::FlushCurrentWord. r=#layout

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

Comment 23

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: potential use-after-free after a memory-allocation failure
• Code covered by automated testing: no
• Fix verified in Nightly: no
• 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: Just ensures early-return codepaths don't skip required cleanup
• String changes made/needed: none
• Is Android affected?: yes

Comment 24

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1915535 - [esr115] Ensure consistent cleanup in nsLineBreaker::FlushCurrentWord.

Comment 25

[Phabricator Automation]

esr115 Uplift Approval Request

• User impact if declined: potential use-after-free after a memory-allocation failure
• Code covered by automated testing: no
• Fix verified in Nightly: no
• 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: Just ensures early-return codepaths don't skip required cleanup
• String changes made/needed: none
• Is Android affected?: yes

Comment 26

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/1a28bb2e9696

Comment 27

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/29000f5f707e

Comment 28

[Frederik Braun [:freddy] (OOO until August 18th)]

Attachment: advisory.txt