1737009 - nsTextFragment::SetTo should check given string length

Status: RESOLVED FIXED

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

Comment 1

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

Oh, some callers don't check the length before calling.

Comment 2

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

Attachment: Bug 1737009 - Make `nsTextFragment::SetTo()` check given string length first r=smaug!

Comment 3

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

For the other branches, I need to create a patch because of bug 1729170.

Comment 4

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

Attachment: Bug 1737009 - Make `nsTextFragment::SetTo()` check given string length first r=smaug!

Comment 5

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

(Sorry, before I realized that this is a security bug, I blocked a bug which depends on this bug and I pushed try server with bug#)

Comment 6

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

Comment on attachment 9247049 [details]
Bug 1737009 - Make nsTextFragment::SetTo() check given string length first r=smaug!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It's easy to reproduce this because the changed points are explicitly touch setter of text of character data modes (text, comment, cdata nodes).
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: esr78
• 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?: The patch makes too long text cause throwing exception. So this patch could rarely break existing web which set text to character data node whose length is over 0x1FFFFFFF. However, that means that at least 540 MB string. So it won't break non-malicious web apps.

Comment 7

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

Changing the priority to p2 as the bug is tracked by a release manager for the current nightly.
See What Do You Triage for more information

Comment 8

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

Comment on attachment 9247049 [details]
Bug 1737009 - Make nsTextFragment::SetTo() check given string length first r=smaug!

Approved to land and request uplift

Comment 9

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

Comment on attachment 9247049 [details]
Bug 1737009 - Make nsTextFragment::SetTo() check given string length first r=smaug!

Beta/Release Uplift Approval Request

• User impact if declined: May cause crash or allow attckers to access out of array (but requires too big memory space)

This is for m-c and beta. The other is for release and esr.

• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• 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): Just adding boundary check before creating unexpected length array.
• String changes made/needed:

Comment 10

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

Comment on attachment 9247062 [details]
Bug 1737009 - Make nsTextFragment::SetTo() check given string length first r=smaug!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: May cause crash or allow attackers to access out of array.
• Fix Landed on Version: 96
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just adding boundary check before creating unexpected length array.
• String or UUID changes made by this patch:

Comment 11

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

Should I land it to autoland before getting uplift approvals? Or wait it for reducing the risk of 0-day attack?

Comment 12

[Andrew McCreight [:mccr8]]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #11)

Should I land it to autoland before getting uplift approvals? Or wait it for reducing the risk of 0-day attack?

The idea of sec-approval is that once you have it, it is okay to land it on autoland. Our release cycles are pretty short now, so as long as we're not in the very end when we're not uplifting things, the window won't be too large.

Comment 13

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

Thank you for the explanation!

Comment 14

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

Make nsTextFragment::SetTo() check given string length first r=smaug
https://hg.mozilla.org/integration/autoland/rev/9140a2ea2c5a27e74e6b290caaea789b8cd0631d
https://hg.mozilla.org/mozilla-central/rev/9140a2ea2c5a

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9247062 [details]
Bug 1737009 - Make nsTextFragment::SetTo() check given string length first r=smaug!

Based on the information given in the bug, it doesn't sound like we need to expedite this into a 94 dot release. Let's ship it in 95/91.4esr per usual practice.

Comment 16

[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 17

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

Done.

Comment 18

[Dianna Smith [:diannaS]]

Comment on attachment 9247049 [details]
Bug 1737009 - Make nsTextFragment::SetTo() check given string length first r=smaug!

Approved for 95.0b6

Comment 19

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/66b7b87e8ccf

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9247062 [details]
Bug 1737009 - Make nsTextFragment::SetTo() check given string length first r=smaug!

Approved for 91.4esr.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/bd4d7d89779a