Do a subsumes check before passing objects through a COW membrane

RESOLVED FIXED in Firefox 33, Firefox OS v2.1

Status

()

Core
XPConnect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

({sec-other})

unspecified
mozilla33
x86
Mac OS X
sec-other
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox33 fixed, firefox-esr31 wontfix, b2g-v2.1 fixed)

Details

(Whiteboard: [adv-main33-])

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Blocks: 928415
(Assignee)

Comment 4

4 years ago
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.
No longer blocks: 928415
Depends on: 856067
(Assignee)

Comment 5

4 years ago
Created attachment 822902 [details] [diff] [review]
Part 1 - Add opt-out mechanism for SpecialPowers. v1
(Assignee)

Comment 6

4 years ago
Created attachment 822903 [details] [diff] [review]
Part 2 - Check objects at COW membranes. v1
(Assignee)

Comment 7

4 years ago
Created attachment 822904 [details] [diff] [review]
Part 3 - Tests. v1
Is this ready for review?  It has sat here for a month, and is blocking a sec-high bug.  Thanks!
(Assignee)

Comment 9

4 years ago
(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.
(Assignee)

Comment 10

4 years ago
The right fix for this is bug 856067.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
(Assignee)

Comment 11

4 years ago
Actually, bug 856067 doesn't cover all the cases here. We need this as the COW analog of bug 1036214.
Blocks: 928415
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Assignee)

Updated

4 years ago
Depends on: 1038844
(Assignee)

Comment 12

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

This moved into bug 1038844.
Attachment #822902 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
Created attachment 8458439 [details] [diff] [review]
Check objects at COW membranes. v2

The deps for this should now be in place. Hopefully we can get it in before
the merge.
Attachment #822903 - Attachment is obsolete: true
Attachment #822904 - Attachment is obsolete: true
Attachment #8458439 - Flags: review?(gkrizsanits)
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...
Attachment #8458439 - Flags: review?(gkrizsanits) → review+
(Assignee)

Comment 16

4 years ago
(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.
(Assignee)

Comment 18

4 years ago
Created attachment 8458843 [details] [diff] [review]
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.
Attachment #8458843 - Flags: review?(mdas)
(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...
(Assignee)

Comment 20

4 years ago
(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.
(Assignee)

Comment 21

4 years ago
Created attachment 8458876 [details] [diff] [review]
Clone marionette args into the sandbox rather than using a COW. v2

Talking to mdas, it sounds like we need wrapReflectors: true here.
Attachment #8458843 - Attachment is obsolete: true
Attachment #8458843 - Flags: review?(mdas)
Attachment #8458876 - Flags: review?(mdas)
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.
Attachment #8458876 - Flags: review?(mdas) → review+
oops, I mean r+ provided you add it to the second instance of that line as well. Thanks!
https://hg.mozilla.org/mozilla-central/rev/030adb40ad19
https://hg.mozilla.org/mozilla-central/rev/2eddd81bb167
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
status-b2g-v2.1: --- → fixed
status-firefox33: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
(Assignee)

Updated

4 years ago
Blocks: 1042824

Updated

4 years ago
Depends on: 1051224
Does this need to be back ported to ESR31?
(Assignee)

Comment 30

4 years ago
(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.
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.
Flags: needinfo?(bobbyholley)
Whiteboard: [adv-main33-]
status-firefox-esr31: --- → wontfix
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.
Flags: needinfo?(bobbyholley) → qe-verify-

Updated

2 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.