Closed Bug 1584170 (CVE-2019-17005) Opened 5 years ago Closed 5 years ago

Access heap-allocated array with index out of bounds in `nsPlainTextSerializer`

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla72
Tracking Status
thunderbird_esr68 --- fixed
firefox-esr68 71+ verified
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 + verified
firefox72 --- verified

People

(Reporter: mbrodesser-Igalia, Assigned: mbrodesser-Igalia)

Details

(Keywords: csectype-bounds, sec-moderate, Whiteboard: [adv-main71+][adv-esr68.3+])

Attachments

(4 files)

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.

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

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.

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.

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?

Flags: needinfo?(mbrodesser)
Group: core-security → dom-core-security

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.

Flags: needinfo?(mbrodesser)

I have a fix pending.

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

Assignee: nobody → mbrodesser
Priority: -- → P2
Attachment #9095493 - Attachment description: Bug 1584170: add test for 101 `<ol>`s. → Bug 1584170: part 1) Add test for 101 `<ol>`s. r=hsivonen

@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.

Flags: needinfo?(dveditz)

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.)

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

(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.

Flags: needinfo?(dveditz)

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

Attachment #9099169 - Attachment description: Bug 1584170: part 2) Factor out duplicated code to `IsInOlOrUl`. r=hsivonen → Bug 1584170: part 1) Factor out duplicated code to `IsInOlOrUl`. r=hsivonen
Attachment #9099170 - Attachment description: Bug 1584170: part 3) Replace array with `AutoTArray`. r=hsivonen → Bug 1584170: part 2) Replace array with `AutoTArray`. r=hsivonen
Attachment #9095493 - Attachment description: Bug 1584170: part 1) Add test for 101 `<ol>`s. r=hsivonen → Bug 1584170: part 3) Add test for 101 `<ol>`s. r=hsivonen
Flags: in-testsuite?
Group: dom-core-security → core-security-release

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

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.

Flags: needinfo?(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 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:
Attachment #9099170 - Flags: approval-mozilla-beta?
Attachment #9099169 - Flags: approval-mozilla-beta?

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.

Attachment #9099170 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9099169 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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.

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

Flags: needinfo?(mbrodesser)

I think this is ready for an ESR68 uplift request.

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

Status: RESOLVED → VERIFIED
Flags: qe-verify+ → needinfo?(alexandru.trif)

: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).

Flags: needinfo?(mbrodesser)

(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?

Flags: needinfo?(mbrodesser)

: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.

Flags: needinfo?(mbrodesser)

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:
Attachment #9099170 - Flags: approval-mozilla-esr68?
Attachment #9099169 - Flags: approval-mozilla-esr68?

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.

Attachment #9099169 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Attachment #9099170 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

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

Flags: needinfo?(mbrodesser)

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

Flags: needinfo?(mbrodesser)

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.

Flags: needinfo?(alexandru.trif)
Alias: CVE-2019-17005
Whiteboard: [adv-main71+]

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?

Flags: needinfo?(mbrodesser)
Attached file advisory.txt
Whiteboard: [adv-main71+] → [adv-main71+][adv-esr68.3+]

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).

Flags: needinfo?(mbrodesser)

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.

Flags: needinfo?(dveditz)

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

Flags: needinfo?(dveditz)
Flags: in-testsuite? → in-testsuite+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.