Closed Bug 1249685 Opened 5 years ago Closed 5 years ago
Root questionable-looking stack variables
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.
Yeah, I think they all looks very suspicious. (and you're talking about mutation events, not mutation observer ;) )
I haven't done much testing but this should be okay.
Attachment #8722602 - Flags: review?(bugs)
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+
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
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-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.
Sorry, I guess I undersold this a bit.
Carsten, Wes: Could you please uplift this fix to esr38.7? Thanks!
I have this queued up for uplift, just waiting for approval in another patch to push the whole batch.
Whiteboard: [adv-main45+][adv-esr38.7+] dom-triaged btpp-active → [post-critsmash-triage][adv-main45+][adv-esr38.7+] dom-triaged btpp-active
You need to log in before you can comment on or make changes to this bug.