Root questionable-looking stack variables

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

({csectype-uaf, sec-high})

Trunk
mozilla47
Points:
---

Firefox Tracking Flags

(firefox44 wontfix, firefox45+ fixed, firefox46+ fixed, firefox47+ fixed, firefox-esr3845+ fixed, firefox-esr45 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main45+][adv-esr38.7+] dom-triaged btpp-active)

Attachments

(1 attachment)

Assignee

Description

3 years ago
I looked through where we call ReplaceChild(), AppendChild(), RemoveChild(), and InsertBefore() to find places that do not root |this| on the stack, and I found a few places that look sketchy. We should fix these. I'm not sure if it is worth trying to write a test case, or if we should treat these as real sec bugs or just more of an audit situation. I'll file a separate bug for adding a static analysis for these problems.

In HTMLTableElement::InsertRow(), parent is not rooted:
538         parent->AppendChild(*newRow, aError);
http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLTableElement.cpp#538

In nsTreeSanitizer::SanitizeChildren(), parent is not rooted:
1423           parent->InsertBefore(*child, node, rv);
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsTreeSanitizer.cpp#1423

In HTMLSelectElement::Add(), parent is not rooted:
590   parent->InsertBefore(aElement, aBefore, aError);
http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLSelectElement.cpp#590

In nsRange::CutContents(), parent is not rooted:
2097         parent->RemoveChild(*node, error);
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsRange.cpp#2097
There's some kind of mutation guard, but AFAICT in the scenario of a hostile mutation observer that destroys parent, it is already too late by the time we check the guard.
Assignee

Updated

3 years ago
Group: core-security → dom-core-security
Whiteboard: dom-triaged btpp-active
Yeah, I think they all looks very suspicious.

(and you're talking about mutation events, not mutation observer ;) )
Assignee

Updated

3 years ago
Btw, looks like regressions from bug 773780 and bug 807075 and bug 824907.
Assignee

Comment 3

3 years ago
I haven't done much testing but this should be okay.
Attachment #8722602 - Flags: review?(bugs)
Attachment #8722602 - Flags: review?(bugs) → review+
Assignee

Comment 4

3 years ago
Comment on attachment 8722602 [details] [diff] [review]
Use more nsCOMPtrs for stack variables in DOM code.

[Security approval request comment]
How easily could an exploit be constructed based on the patch? I'm not entirely sure if this is exploitable, but if it is, it would not be very hard.

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 older supported branches are affected by this flaw? all

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The patches should be the same.

How likely is this patch to cause regressions; how much testing does it need? Low risk.
Attachment #8722602 - Flags: sec-approval?
We should take this for trunk. I'm not sure, given where we are in the cycle, that we'd want to take it on Beta or ESR38. We should probably land on Aurora though.
Attachment #8722602 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/025b917f39b3
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Assignee

Comment 8

3 years ago
Comment on attachment 8722602 [details] [diff] [review]
Use more nsCOMPtrs for stack variables in DOM code.

Approval Request Comment
[Feature/regressing bug #]: old (see comment 2)
[User impact if declined]: sec-high
[Describe test coverage new/current, TreeHerder]: this code is well-tested
[Risks and why]: very low, it just holds some things alive
[String/UUID change made/needed]: none
Attachment #8722602 - Flags: approval-mozilla-esr38?
Attachment #8722602 - Flags: approval-mozilla-beta?
Attachment #8722602 - Flags: approval-mozilla-aurora?
Comment on attachment 8722602 [details] [diff] [review]
Use more nsCOMPtrs for stack variables in DOM code.

Let's uplift to Aurora46 to stabilize. Given that this may not be easily exploitable, I am leaning towards not taking this in ESR38.7 or Beta45. Sylvestre, please let me know if you are ok with that plan.
Attachment #8722602 - Flags: approval-mozilla-esr38?
Attachment #8722602 - Flags: approval-mozilla-esr38-
Attachment #8722602 - Flags: approval-mozilla-beta?
Attachment #8722602 - Flags: approval-mozilla-beta-
Attachment #8722602 - Flags: approval-mozilla-aurora?
Attachment #8722602 - Flags: approval-mozilla-aurora+
Comment on attachment 8722602 [details] [diff] [review]
Use more nsCOMPtrs for stack variables in DOM code.

This so clearly pinpoints where the issue is, and it is sec-critical and fix should be super safe, I would definitely take this to all the branches asap.
Attachment #8722602 - Flags: approval-mozilla-beta- → approval-mozilla-beta?
Attachment #8722602 - Flags: approval-mozilla-esr38- → approval-mozilla-esr38?
Comment on attachment 8722602 [details] [diff] [review]
Use more nsCOMPtrs for stack variables in DOM code.

OK, let's take it then.
Attachment #8722602 - Flags: approval-mozilla-esr38?
Attachment #8722602 - Flags: approval-mozilla-esr38+
Attachment #8722602 - Flags: approval-mozilla-beta?
Attachment #8722602 - Flags: approval-mozilla-beta+
Assignee

Comment 14

3 years ago
Sorry, I guess I undersold this a bit.
Carsten, Wes: Could you please uplift this fix to esr38.7? Thanks!
Flags: needinfo?(wkocher)
Flags: needinfo?(cbook)
I have this queued up for uplift, just waiting for approval in another patch to push the whole batch.
Flags: needinfo?(wkocher)
Flags: needinfo?(cbook)
Whiteboard: dom-triaged btpp-active → [adv-main45+][adv-esr38.7+] dom-triaged btpp-active
Whiteboard: [adv-main45+][adv-esr38.7+] dom-triaged btpp-active → [post-critsmash-triage][adv-main45+][adv-esr38.7+] dom-triaged btpp-active
Group: dom-core-security → core-security-release
Group: core-security-release
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.