1906727 - (CVE-2024-7522) AddressSanitizer: heap-buffer-overflow in GetIntegerValue nsAttrValueInlines.h

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Irvan Kurniawan [:sourc7]]

Attachment: testcase.html

After set document.designMode to on, set font element size, then execCommand to run "selectAll", "createLink", and "fontSize".

On Firefox ASan, the tab crash with output AddressSanitizer: heap-buffer-overflow in GetIntegerValue with READ of size 4

Steps to reproduce

1. Download Firefox ASan
2. Visit attached testcase.html
3. Firefox ASan tab crash with output AddressSanitizer: heap-buffer-overflow

Comment 1

[Irvan Kurniawan [:sourc7]]

Attachment: asan.heap-buffer-overflow-read4.txt

Comment 2

[Irvan Kurniawan [:sourc7]]

I can reproduce this on Firefox ASan from branch mozilla-release, mozilla-beta, and mozilla-central.

Comment 3

[Irvan Kurniawan [:sourc7]]

I can also reproduce this on Firefox ASan esr115 branch

Comment 4

[Andrew McCreight [:mccr8]]

In a debug build, not surprisingly you get an assertion: Assertion failure: Type() == eInteger (wrong type), at dom/base/nsAttrValueInlines.h:146. Looks like some kind of type confusion with nsAttrValue.

Comment 5

[Andrew McCreight [:mccr8]]

Masayuki, could you please take a look? Thanks.

Comment 6

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

Sure, but I don't know the proper severity for this. Here could cause reading invalid address, but only 32bit space.

Comment 7

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

Ah, it's already set to sec-high. Setting it to S2.

Comment 8

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

Attachment: Bug 1906727 - Make `HTMLEditor::SplitAncestorStyledInlineElementsAt` check attr value type first r=peterv!

Comment 9

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

Attachment: Bug 1906727 - Make `HTMLEditor::SplitAncestorStyledInlineElementsAt` check attr value type first r=peterv!

This patch is for ESR 115.

Comment 10

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20240715095103-325ffb165783.
Unable to bisect testcase (Testcase reproduces on start build!):

Start: 89d21fac92b8f98201b8715c6c050aace4b46afb (20230717160307)
End: d17c727b8dd28811d11817390883cbb554eb2347 (20240708093609)
BuildFlags: BuildFlags(asan=True, tsan=False, debug=False, fuzzing=True, coverage=False, valgrind=False, no_opt=False, fuzzilli=False, nyx=False)

Comment 11

[Donal Meehan [:dmeehan]]

:masayuki we are in the final week of beta for Fx129.
There are only 2 beta builds left in the cycle.
Do you need to find a different reviewer for this?
Wondering it it can get sec-approval and uplift requests in time

Comment 12

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

Comment on attachment 9411669 [details]
Bug 1906727 - Make HTMLEditor::SplitAncestorStyledInlineElementsAt check attr value type first r=peterv!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It touches the entrance from JS. Therefore, if they realize that it's a handler of document.exeCommand, they might find the way to reproduce this.
• 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, ESR128, ESR115
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: This fixes the use of the internal API, shouldn't cause any regressions.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 13

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

Comment on attachment 9411669 [details]
Bug 1906727 - Make HTMLEditor::SplitAncestorStyledInlineElementsAt check attr value type first r=peterv!

Beta/Release Uplift Approval Request

• User impact if declined: The attacker might be possible to read the memory.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: 1. Run a debug build and load the testcase

Then, if this is reproducible, you'll see an assertion failure which is immediately before the crash point.

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Only the fix of internal API use.
• String changes made/needed: No
• Is Android affected?: Yes

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
• User impact if declined: See above.
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Only the fix of internal API use.

Comment 14

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

Comment on attachment 9411933 [details]
Bug 1906727 - Make HTMLEditor::SplitAncestorStyledInlineElementsAt check attr value type first r=peterv!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high.
• User impact if declined: See above.
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Only the fix of internal API use.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 9411669 [details]
Bug 1906727 - Make HTMLEditor::SplitAncestorStyledInlineElementsAt check attr value type first r=peterv!

sec-approval+ = dveditz
please land on nightly, and beta before Friday (last beta build starts)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/80bfb01e76a5
Make `HTMLEditor::SplitAncestorStyledInlineElementsAt` check attr value type first r=peterv

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/80bfb01e76a5

Comment 18

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/825054131ae4

Comment 19

[Donal Meehan [:dmeehan]]

Comment on attachment 9411669 [details]
Bug 1906727 - Make HTMLEditor::SplitAncestorStyledInlineElementsAt check attr value type first r=peterv!

Approved for 128.1esr.

Comment 20

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/0ffb900257a9

Comment 21

[Donal Meehan [:dmeehan]]

Comment on attachment 9411933 [details]
Bug 1906727 - Make HTMLEditor::SplitAncestorStyledInlineElementsAt check attr value type first r=peterv!

Approved for 115.14esr

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/6599014712b1

Comment 23

[Bugmon [:jkratzer for issues]]

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

Comment 24

[Bogdan Maris, Desktop Test Engineering [OOO until Dec 16th]]

Reproduced the crash with AddressSanitizer: heap-buffer-overflow using an old Nightly Asan build from 2024-07-23. Verified that using Firefox 128esr and 115esr builds the crash does not occur anymore on Windows 11 and Ubuntu 22.04. (unfortunately I could not start a build-macosx64-asan-fuzzing/opt build on my mac13.6 so I could not verify here as well).

Comment 25

[Tyson Smith [:tsmith]]

Attachment: advisory.txt

Comment 26

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

a month ago, dveditz placed a reminder on the bug using the whiteboard tag [reminder-test 2024-09-23] .

masayuki, please refer to the original comment to better understand the reason for the reminder.

Comment 27

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

dveditz: I think you added the reminder to adding the test into the tree. Can I do that right now even though this bug is still hidden? And If we should do so, where is a good place to work on? this bug? or a new open bug?

Comment 28

[Daniel Veditz [:dveditz]]

Yes, you can land tests now. If there was a reviewed test patch already part of this bug you could land it from here. Since there isn't it's probably best to open a new "Land tests for bug 1906727" bug and get reviews and such there. It should block this bug but it doesn't need to be a security bug.