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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

bug 1308922 fixed one instance, but I noticed today couple of more in HTML Table code.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

The issue is way more common than I thought :(

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 4

[Andrew McCreight [:mccr8]]

We really need some kind of GC-like analysis for DOM nodes. :(

Comment 5

[Andrew McCreight [:mccr8]]

I'm going to assume that at least something in there can be triggered.

Comment 6

[(no longer active)]

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?

Comment 7

[(no longer active)]

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.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 10

[(no longer active)]

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

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 12

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk.
I think we should take this everywhere but I need release management to approve these.

Comment 13

[(no longer active)]

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

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Given that this is rated as a sec-crit, we can take it on Beta50, Aurora51 and ESR45. Please nominate patches for uplift.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: esr45

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

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8800451 [details] [diff] [review]
patch

Approval Request Comment
See comment 15

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63d0a40426ecc68cb9bbf93179e769ab06c7afa0

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8800451 [details] [diff] [review]
patch

Approving branch patches based on release management comments.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/24f9300535ef
https://hg.mozilla.org/releases/mozilla-beta/rev/af995f34ee2e
https://hg.mozilla.org/releases/mozilla-esr45/rev/8e0bab4216de

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/63d0a40426ec