930091 - Do a subsumes check before passing objects through a COW membrane

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Jesse suggested this, and I think it might just work. If it does, it will fix bugs like bug 924329. This doesn't remove the need to move everything to WebIDL, but it will fix this particular attack pattern in a backportable way.

One of the problems we run into with COWs is that content can pass cross-origin objects (like Location) to chrome, and trick it into performing a toString() and passing back the result. The fact that this object shouldn't be toString()ed is undetectable by the time the object reaches chrome, because wrappers don't record a history of where they've been passed - each time something ends up on the other side of a membrane, we strip off all the old wrappers and compute a new one.

However, we _can_ override the behavior of COWs such that, before bouncing to the CrossCompartmentWrapper machinery, we do a subsumes check on any value passing through. I'm going to give this a shot.

Comment 1

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=a7c0ab7da92e

Comment 2

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=a3e3593fb902

Comment 3

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=2ce6bee06448

Comment 4

[Bobby Holley (:bholley)]

So, this is mostly green, but there are still some oranges to deal with. However, I realized that it doesn't solve all of the attack patterns here.

In particular, while content cannot do:

chromeFun(xolocation);
chromeObject.foo = xolocation;

It can still do:

chromeFun({foopy: xolocation});
chromeObject.foo = {foopy: xolocation};

Fixing the latter case requires bug 856067. So I'm going to upload the patches, block on that, and give up hope that this will be a quick fix  for bugs like bug 928415.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add opt-out mechanism for SpecialPowers. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 2 - Check objects at COW membranes. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 3 - Tests. v1

Comment 8

[Andrew McCreight [:mccr8]]

Is this ready for review?  It has sat here for a month, and is blocking a sec-high bug.  Thanks!

Comment 9

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #8)
> Is this ready for review?  It has sat here for a month, and is blocking a
> sec-high bug.  Thanks!

No, and it's not the quick fix I was hoping for. See comment 4.

Comment 10

[Bobby Holley (:bholley)]

The right fix for this is bug 856067.

Comment 11

[Bobby Holley (:bholley)]

Actually, bug 856067 doesn't cover all the cases here. We need this as the COW analog of bug 1036214.

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 822902 [details] [diff] [review]
Part 1 - Add opt-out mechanism for SpecialPowers. v1

This moved into bug 1038844.

Comment 13

[Bobby Holley (:bholley)]

Attachment: Check objects at COW membranes. v2

The deps for this should now be in place. Hopefully we can get it in before
the merge.

Comment 14

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=b0d261a30bfd

Comment 15

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8458439 [details] [diff] [review]
Check objects at COW membranes. v2

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

Patch looks great, if the failing tests are easy to be fixed then r=me.

::: js/xpconnect/wrappers/ChromeObjectWrapper.cpp
@@ +115,5 @@
> +        return true;
> +
> +    // COWs are fine to pass back if and only if they have __exposedProps__,
> +    // since presumably content should never have a reason to pass an opaque
> +    // object back to chrome.

Is this really needed / worth it? How far do you want to back-port this? Thinking about JS function objects...

Comment 16

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #15)
> Comment on attachment 8458439 [details] [diff] [review]
> Check objects at COW membranes. v2
> 
> Review of attachment 8458439 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Patch looks great, if the failing tests are easy to be fixed then r=me.
> 
> ::: js/xpconnect/wrappers/ChromeObjectWrapper.cpp
> @@ +115,5 @@
> > +        return true;
> > +
> > +    // COWs are fine to pass back if and only if they have __exposedProps__,
> > +    // since presumably content should never have a reason to pass an opaque
> > +    // object back to chrome.
> 
> Is this really needed / worth it?

At the very least, it's needed for the |this| on an __exposedProps__-implemented API object, right? i.e. fooAPI.bar().

> How far do you want to back-port this?

This isn't really useful without bug 856067, so I'd only land it for this cycle (or backport to aurora if it lands next week).

> Thinking about JS function objects...

I don't follow.

Comment 17

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=01d92847b46a

Comment 18

[Bobby Holley (:bholley)]

Attachment: Clone marionette args into the sandbox rather than using a COW. v1

The current setup causes the arguments object (which is modified from the
sandbox) to be a COW, which causes problems with our new restrictions on
COWs. According to jgriffin, these are should be JSON-serializable, so
the clone should be fine.

Comment 19

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #16)
> > > +    // COWs are fine to pass back if and only if they have __exposedProps__,
> > > +    // since presumably content should never have a reason to pass an opaque
> > > +    // object back to chrome.
> > 
> > Is this really needed / worth it?
> 
> At the very least, it's needed for the |this| on an
> __exposedProps__-implemented API object, right? i.e. fooAPI.bar().

I get that, what I was curious about the "only if" part. It's hard for me to come up with a use case...
Then again better safe than sorry, just was curious if we had any related bugs (out of general interest).

> > Thinking about JS function objects...
> 
> I don't follow.

The only example I could come up with for the how/why could content have a reference to chrome object without __exposedProps__ case, is old style COW for function objects exported by an add-on. In which case it would be not _that_ far-fetched idea to pass it back to chrome at some point. That's why I asked the how far do you want to back-port this. Since now-days functions should get an xray anyway. Does this make any sense? Sorry if I'm mixing up something...

Comment 20

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #19)
> (In reply to Bobby Holley (:bholley) from comment #16)
> > > > +    // COWs are fine to pass back if and only if they have __exposedProps__,
> > > > +    // since presumably content should never have a reason to pass an opaque
> > > > +    // object back to chrome.
> > > 
> > > Is this really needed / worth it?
> > 
> > At the very least, it's needed for the |this| on an
> > __exposedProps__-implemented API object, right? i.e. fooAPI.bar().
> 
> I get that, what I was curious about the "only if" part.

Ah, sorry - I misunderstood. Yeah, it's a general defense-in-depth thing. It's causing a couple of scattered failures, mostly from COWs that are implicitly usable without __exposedProps__ (i.e. Function and Array). It looks tractable though, so I'd rather do that if we can.

> The only example I could come up with for the how/why could content have a
> reference to chrome object without __exposedProps__ case, is old style COW
> for function objects exported by an add-on.

Yeah, that matches the two failures I ran into (one in the devtools tests, and one in the marionette harness).

> In which case it would be not
> _that_ far-fetched idea to pass it back to chrome at some point. That's why
> I asked the how far do you want to back-port this. Since now-days functions
> should get an xray anyway.

No, because we ForceCOWBehavior for Functions. :-(

> Does this make any sense? Sorry if I'm mixing up
> something...

Makes total sense. We should definitely leave the option open to special-case Functions here if we encounter breakage.

Comment 21

[Bobby Holley (:bholley)]

Attachment: Clone marionette args into the sandbox rather than using a COW. v2

Talking to mdas, it sounds like we need wrapReflectors: true here.

Comment 22

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=dab81a09718a

Comment 23

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 8458876 [details] [diff] [review]
Clone marionette args into the sandbox rather than using a COW. v2

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

::: testing/marionette/marionette-listener.js
@@ +646,5 @@
>    }
>    else {
>      try {
> +      sandbox.__marionetteParams = Cu.cloneInto(elementManager.convertWrappedArguments(
> +        msg.json.args, curFrame), sandbox, { wrapReflectors: true });

This should be applied on line 515 too, it's similar code.

I tested locally with both the line changes, and it works as I expect.

Comment 24

[Malini Das [:mdas] - Away, not checking bugmail]

oops, I mean r+ provided you add it to the second instance of that line as well. Thanks!

Comment 25

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=55d2b36a0f91

Comment 26

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=8652469912ab

Comment 27

[Bobby Holley (:bholley)]

Union of the last two try pushes is green:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/030adb40ad19
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2eddd81bb167

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/030adb40ad19
https://hg.mozilla.org/mozilla-central/rev/2eddd81bb167

Comment 29

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

Does this need to be back ported to ESR31?

Comment 30

[Bobby Holley (:bholley)]

(In reply to Al Billings [:abillings] from comment #29)
> Does this need to be back ported to ESR31?

This part of a large body of slaughterhouse work that would probably have to all be uplifted together (on the order of 100 patches). We can consider that at some point, but I want to get everything smoothed out and shipping on release first.

Comment 31

[Matt Wobensmith [:mwobensmith][:matt:]]

Is there a test, or other way to both reproduce the problem and verify that it's been fixed? 

I see that Bobby's away til 10/12, but that still gives us a chance to verify pre-release if we have more info on his return.

Comment 32

[Matt Wobensmith [:mwobensmith][:matt:]]

Actually, on second thought, I'm going to mark this [qe-verify-] in the interim. If there are tests to be run that can verify this bug fix, just get in touch and I'm happy to look at them. Thanks.