Ensure the child passed to nsINode::InsertBefore is kept alive

RESOLVED FIXED in Firefox -esr45

Status

()

defect
RESOLVED FIXED
3 years ago
5 months ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

({csectype-uaf, sec-critical})

50 Branch
mozilla52
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox49 wontfix, firefox-esr4550+ fixed, firefox50+ fixed, firefox51+ fixed, firefox52+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main50+][adv-esr45.5+])

Attachments

(2 attachments)

bug 1308922 fixed one instance, but I noticed today couple of more in HTML Table code.
Assignee: nobody → bugs
The issue is way more common than I thought :(
Posted patch patchSplinter Review
Attachment #8800451 - Flags: review?(ehsan)
I decided to go this route and not change InsertBefore, since changing InsertBefore would make it slower in common case (JS using it) and we are just violating com rules here.
We really need some kind of GC-like analysis for DOM nodes. :(
I'm going to assume that at least something in there can be triggered.
It seems possible to avoid this general pattern by making InsertBefore take a |const nsCOMPtr<nsINode>&| as its second argument.  Is there any reason why you didn't do that?
Flags: needinfo?(bugs)
Comment on attachment 8800451 [details] [diff] [review]
patch

Review of attachment 8800451 [details] [diff] [review]:
-----------------------------------------------------------------

At any rate, this patch itself is good, but I still think we should also do comment 6, and backport that as well.
Attachment #8800451 - Flags: review?(ehsan) → review+
(In reply to :Ehsan Akhgari from comment #6)
> It seems possible to avoid this general pattern by making InsertBefore take
> a |const nsCOMPtr<nsINode>&| as its second argument.  Is there any reason
> why you didn't do that?
This is a method added for and used by webidl bindings, and they pass raw pointer.
No need to slow that down.

And, wouldn't const nsCOMPtr<nsINode>& still allow passing member variables.
Flags: needinfo?(bugs)
Comment on attachment 8800451 [details] [diff] [review]
patch

[Security approval request comment]
See bug 1308922. This is very similar

How easily could an exploit be constructed based on the patch?
It is still a bit unclear to me how to construct an exploit from this.
Initially we end up doing deletedobject[someoffset] == <something else> comparison.
If someone manages to control the memory area for deleted object and the right offset, then worse things could happen.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Commit message could be: 
Bug 1309720, ensure expected DOM tree operations when calling insertBefore, r=ehsan

Which older supported branches are affected by this flaw?
All

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I don't have yet, but should be easy to create.
The patch applies cleanly to aurora and beta, but esr45 needs a new patch because there has been some file renaming.

How likely is this patch to cause regressions; how much testing does it need?
safe stuff. Just keeping objects alive longer.
Attachment #8800451 - Flags: sec-approval?
(In reply to Olli Pettay [:smaug] from comment #8)
> (In reply to :Ehsan Akhgari from comment #6)
> > It seems possible to avoid this general pattern by making InsertBefore take
> > a |const nsCOMPtr<nsINode>&| as its second argument.  Is there any reason
> > why you didn't do that?
> This is a method added for and used by webidl bindings, and they pass raw
> pointer.
> No need to slow that down.

Can we use a binaryName for that, and call it InsertBeforeWebIDLConsumerOnly or some such and not use that version in our internal code?

> And, wouldn't const nsCOMPtr<nsINode>& still allow passing member variables.

It would.  Is that not OK?
(In reply to :Ehsan Akhgari from comment #10)
> It would.  Is that not OK?
That is definitely not ok in general. We need to follow COM rules here and elsewhere, caller is supposed to keep params alive, and using member variables doesn't guarantee that.

But, nsCOMPtr<>& as a param might make this code less fragile.
I'll file a followup.
sec-approval+ for trunk.
I think we should take this everywhere but I need release management to approve these.
Attachment #8800451 - Flags: sec-approval? → sec-approval+
(In reply to Olli Pettay [:smaug] from comment #11)
> (In reply to :Ehsan Akhgari from comment #10)
> > It would.  Is that not OK?
> That is definitely not ok in general. We need to follow COM rules here and
> elsewhere, caller is supposed to keep params alive, and using member
> variables doesn't guarantee that.

Right, but that's similar to the cases where we have kungfu death grips...

> But, nsCOMPtr<>& as a param might make this code less fragile.
> I'll file a followup.

Thanks!
Given that this is rated as a sec-crit, we can take it on Beta50, Aurora51 and ESR45. Please nominate patches for uplift.
Flags: needinfo?(rkothari)
Posted patch esr45Splinter Review
Went through again all the InsertBefore instance in tree, and couldn't find more.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
It is sec-crit
User impact if declined: security sensitive crashes. 
Fix Landed on Version: About to land to FF52
Risk to taking this patch (and alternatives if risky): Should be safe. Just more kungfuDeathGrips
String or UUID changes made by this patch:  NA
Attachment #8801084 - Flags: approval-mozilla-esr45?
Comment on attachment 8800451 [details] [diff] [review]
patch

Approval Request Comment
See comment 15
Attachment #8800451 - Flags: approval-mozilla-beta?
Attachment #8800451 - Flags: approval-mozilla-aurora?
Comment on attachment 8800451 [details] [diff] [review]
patch

Approving branch patches based on release management comments.
Attachment #8800451 - Flags: approval-mozilla-beta?
Attachment #8800451 - Flags: approval-mozilla-beta+
Attachment #8800451 - Flags: approval-mozilla-aurora?
Attachment #8800451 - Flags: approval-mozilla-aurora+
Attachment #8801084 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
https://hg.mozilla.org/mozilla-central/rev/63d0a40426ec
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Group: dom-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main50+][adv-esr45.5+]
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.