1584170 - (CVE-2019-17005) Access heap-allocated array with index out of bounds in `nsPlainTextSerializer`

Status: VERIFIED FIXED

(Core :: DOM: Serializers, defect, P2)

Description

[Mirko Brodesser (:mbrodesser-Igalia)]

mOLStackIndex - 1 may exceed OLStackSize (https://searchfox.org/mozilla-central/rev/f43ae7e1c43a4a940b658381157a6ea6c5a185c1/dom/base/nsPlainTextSerializer.cpp#779).

I'll attach a review with a unit-test demonstrating it.

It's unclear if the code-path can be reached in Gecko code, because it depends on nsIDocumentEncoder::OutputFormatted. It's likely to be reachable in Thunderbird.

Comment 1

[Mirko Brodesser (:mbrodesser-Igalia)]

Attachment: Bug 1584170: part 3) Add test for 101 `<ol>`s. r=hsivonen

The first assertion is commented out to allow to reproduce this for
debug builds. In release build, it would be a no-op anyway.

Comment 2

[Mirko Brodesser (:mbrodesser-Igalia)]

This is marked as a security issue, because it potentially is. However, I'm not sure if this can indeed be exploited. Feedback from the security team would be great.

Comment 3

[Mirko Brodesser (:mbrodesser-Igalia)]

Thanks to Emilio, we found out that this indeed can make Firefox crash. Reproduce with:

1. Open:
   data:text/html,<ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><ol><li>X</li></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol></ol><li>X</li></ol>

2. File -> Save Page as -> Text Files

Expected: file is saved.
Actual: tab crashes.

Comment 4

[Daniel Veditz [:dveditz]]

Setting sec-moderate because we currently think it would require uncommon user action to trigger: in Firefox saving a file as text, in Thunderbird setting your mail display to the non-default plain-text view. The latter is relatively popular though so this is probably more important in Tbird than Firefox.

Is there a regression range for this or has it been around for a long time?

Comment 5

[Daniel Veditz [:dveditz]]

bp-1f9e00d9-b8ce-4c44-a390-7beba0190926

Comment 6

[Mirko Brodesser (:mbrodesser-Igalia)]

I guess this bug existed since 2010: https://searchfox.org/mozilla-central/rev/e1481bf4bb0e3fcc6648eea0d4bd9c16c20037ce/dom/base/nsPlainTextSerializer.cpp#674.

I've refactored the same file a lot in the last months but the semantics of this code wasn't changed.

Comment 7

[Mirko Brodesser (:mbrodesser-Igalia)]

I have a fix pending.

Edit: slightly more complicated than I thought, will have to continue on it after PTO.

Comment 8

[Mirko Brodesser (:mbrodesser-Igalia)]

@dveditz: having read https://wiki.mozilla.org/Security/Firefox_security_bug_fixing, I guess it's fine to submit the test with the fix (which I'll open a review for soon)?

Edit: however, https://wiki.mozilla.org/Security/Bug_Approval_Process states "Do not commit tests when checking in to mozilla-central or, later, branches, when the security bug fix is initially checked-in. Remember we don’t want to 0-day ourselves!" which seems to be a general guideline there. If it's not meant to apply to "sec-moderate", it would be helpful to state that more clearly.

Comment 9

[Mirko Brodesser (:mbrodesser-Igalia)]

Attachment: Bug 1584170: part 1) Factor out duplicated code to `IsInOlOrUl`. r=hsivonen

Depends on D47239

Comment 10

[Mirko Brodesser (:mbrodesser-Igalia)]

Attachment: Bug 1584170: part 2) Replace array with `AutoTArray`. r=hsivonen

Depends on D48320

Comment 11

[Kai Engert [:KaiE:]]

Dan, do you consider to uplift this to FF ESR 68 ?

(If not, Thunderbird should uplift the fix to THUNDERBIRD_68_VERBRANCH after it has been commited to m-c.)

Comment 12

[Kai Engert [:KaiE:]]

Adding Jörg, for tracking a potential uplift to TB 68, see comment 11.

Comment 13

[Daniel Veditz [:dveditz]]

(In reply to Mirko Brodesser (:mbrodesser) from comment #8)

Edit: however, https://wiki.mozilla.org/Security/Bug_Approval_Process states "Do not commit tests when checking in to mozilla-central or, later, branches, when the security bug fix is initially checked-in. Remember we don’t want to 0-day ourselves!" which seems to be a general guideline there. If it's not meant to apply to "sec-moderate", it would be helpful to state that more clearly.

It's a general guideline. A sec-low bug is unlikely to 0-day us, so checking in a test would be fine in most cases. sec-moderate gets trickier. IMHO this is borderline sec-high for Thunderbird so we shouldn't check in a test until later.

Comment 14

[Mirko Brodesser (:mbrodesser-Igalia)]

dveditz: thanks, then the test will be submitted six months after the fix.

Comment 15

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

https://hg.mozilla.org/integration/autoland/rev/8b6445c96a49e190644b674821638d7385ee5885
https://hg.mozilla.org/integration/autoland/rev/be0a2d1c25c15069d14f9fc5a4ac0c4571c287cf
https://hg.mozilla.org/mozilla-central/rev/8b6445c96a49
https://hg.mozilla.org/mozilla-central/rev/be0a2d1c25c1

Comment 16

[Kai Engert [:KaiE:]]

Please evaluate if this should be backported to ESR 68. Thunderbird would probably benefit from this fix.

Comment 17

[Mirko Brodesser (:mbrodesser-Igalia)]

Dan, what about uplifting the fix to Beta and ESR 68? For beta I guess the risk is low, because the code should differ little. For ESR it's presumably slightly higher, because nsPlainTextSerializer changed quite a lot, but I still consider it low.

I've verified the fix on Nightly.

Comment 18

[Daniel Veditz [:dveditz]]

I'd like to see this fixed in Thunderbird sooner than later, so yeah let's uplift to Beta so we can get it into the next ESR release (68.3)

Comment 19

[Mirko Brodesser (:mbrodesser-Igalia)]

Comment on attachment 9099170 [details]
Bug 1584170: part 2) Replace array with AutoTArray. r=hsivonen

Beta/Release Uplift Approval Request

• User impact if declined: For Firefox, the little used feature of saving a document to text contains a security vulnerability. This fix is more important for Thunderbird though, so it should make it to ESR 68.3.
• 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): The fix replaces a C++ array with a Mozilla specific array and the changes are only in one class.
• String changes made/needed:

Comment 20

[Pascal Chevrel:pascalc]

Comment on attachment 9099170 [details]
Bug 1584170: part 2) Replace array with AutoTArray. r=hsivonen

Approved for 71 beta 4, let's evaluate an ESR uplift afterwards. Thanks.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/81143a1b4202e6d8495cc1e578a21149da410d0f
https://hg.mozilla.org/releases/mozilla-beta/rev/1e6f7dcf4ea0b5ab45b826044e90644632dbfd1a

Comment 22

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Hello!
Reproduced the issue using Firefox 71.0a1 (20190926094200) and STR from comment 3. After saving the page as .txt the tab crashes.
The issue is verified fixed with Firefox 72.0a1 (20191024214023) and Firefox 71.0b4 (20191024095932) on Windows 10x64, macOS 10.14 and Ubuntu 18.04. No tab crashes encountered when saving as .txt extension. Leaving qe+ for verifying on esr if needed.

Comment 23

[Mirko Brodesser (:mbrodesser-Igalia)]

'ni?' myself in order to land the test in 6 months.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

I think this is ready for an ESR68 uplift request.

Comment 25

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Removing the qe+ flag and leaving a ni? on myself, to verify it when this lands on esr. Thank you!

Comment 26

[Mirko Brodesser (:mbrodesser-Igalia)]

:RyanVM: since the fix is in Nightly, it's going to end up in Firefox ESR 68.3 anyway. According to comment #18 that seems sufficient. So no uplifting to prior ESR releases. But please correct me if I'm wrong.

(removed the "ni?", have a calendar-event as a reminder to submit the test).

Comment 27

[Kai Engert [:KaiE:]]

(In reply to Mirko Brodesser (:mbrodesser) from comment #26)

:RyanVM: since the fix is in Nightly, it's going to end up in Firefox ESR 68.3 anyway. According to comment #18 that seems sufficient. So no uplifting to prior ESR releases. But please correct me if I'm wrong.

I wasn't aware there are automatisms that uplift from m-c to esr. How do those work?

Comment 28

[Mirko Brodesser (:mbrodesser-Igalia)]

:kaie: thanks for pointing this out. I guess I was wrong here and there is no such automatism. I'll file a 68.3 uplift request.

Comment 29

[Mirko Brodesser (:mbrodesser-Igalia)]

Comment on attachment 9099170 [details]
Bug 1584170: part 2) Replace array with AutoTArray. r=hsivonen

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: For Thunderbird, it's relevant for plain text view, which seems to be a popular feature.
• User impact if declined: For Firefox, the little used feature of saving a document to text contains a security vulnerability. This fix is more important for Thunderbird though, so it should make it to ESR 68.3.
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The fix replaces a C++ array with a Mozilla specific array and the changes are only in one class.
• String or UUID changes made by this patch:

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9099169 [details]
Bug 1584170: part 1) Factor out duplicated code to IsInOlOrUl. r=hsivonen

Low-risk fix for a sec issue affecting Firefox and TB. Approved for 68.3esr.

Comment 31

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

https://hg.mozilla.org/releases/mozilla-esr68/rev/1bcfb098011f
https://hg.mozilla.org/releases/mozilla-esr68/rev/4b95348b577e

Comment 32

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

There were some conflicts for the uplifts, maybe you want to verify that they got resolved correctly.

Comment 33

[Mirko Brodesser (:mbrodesser-Igalia)]

aryx: looks correct. Thanks for resolving the merge-conflicts.

Comment 34

[Alexandru Trif, Desktop Test Engineering [:atrif]]

The issue is verified using Firefox 68.3.0esr (20191101165134) from comment 31 on Windows 10x64, Ubuntu 18.04 and macOS 10.14. No tab crash encountered after following steps from comment 2.

Comment 35

[Tom Ritter [:tjr]]

I noticed this part of the diff:

 // Someday may want to make this non-const:
 static const uint32_t TagStackSize = 500;
-static const uint32_t OLStackSize = 100;

Are we certain there is not a similar bug with TagStack?

Comment 36

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 37

[Mirko Brodesser (:mbrodesser-Igalia)]

tjr: I checked that before, and at least couldn't come up with a problematic scenario. I've checked it again:

[mTagStack](https://searchfox.org/mozilla-central/search?q=symbol:F_%3CT_nsPlainTextSerializer%3E_mTagStack&redirect=false) is only accessed with indices in [0, TagStackSize), hence it should be safe.

However, in general it'd be safer to replace all arrays allocated with new[] (like mTagStack with AutoTArray or similar classes).

Comment 38

[Mirko Brodesser (:mbrodesser-Igalia)]

dveditz: I'd like to land the test. According to Firefox Security Bugs documentation enough time has passed. I presume enough time has passed for Thunderbird too, but to be safe, I'd like to get the security team's confirmation first.

Comment 39

[Daniel Veditz [:dveditz]]

Go right ahead. Plenty of time and Thunderbird/ESR is fixed.

Comment 40

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

https://hg.mozilla.org/integration/autoland/rev/4c6b2ceb8b2c6d4ea20317b4cf08968decbc330d
https://hg.mozilla.org/mozilla-central/rev/4c6b2ceb8b2c