866823 - Xray Waivers can be used to bypass COWs

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

This is the lynchpin of bug 863933.

Basically, the prototype remapping code in WrapperFatory::Rewrap uses js::Wrapper::wrappedObject instead of js::UncheckedUnwrap to determine if the underlying object is a standard prototype. This means that if there's another wrapper in the middle (like an Xray waiver), we can be fooled into thinking that the object is not a standard prototype. However, the code in ChromeObjectWrapper.cpp unconditionally lets accesses to standard-prototype properties through, under the assumption that they'll get remapped.

To fix this, we should do a couple of things:
* Definitely stop using js::Wrapper::wrappedObject here
* Move this whole thing from prototype calculation into WrapperFactory::PrepareForWrapping so that we unconditionally get the right object when we cross the boundary.

In the long term, we should really just kill this prototype remapping stuff entirely.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Don't create waivers in WaiveXrayAndWrap if the caller has no business waiving. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Handle standard prototype remapping in PrepareForWrapping. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Tests. v1

Comment 4

[Bobby Holley (:bholley)]

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

Comment 5

[Gabor Krizsanits (INACTIVE)]

+            if (!JS_GetClassPrototype(cx, key, homeProto.address()))
+                return NULL;

nullptr

+static bool
+NeedsCOW(JSCompartment *target, JSObject *obj)
+{
+    MOZ_ASSERT(!js::IsCrossCompartmentWrapper(obj));
+    bool targetIsChrome = AccessCheck::isChrome(target);
+    bool originIsChrome = AccessCheck::isChrome(obj);

So I really dislike the idea of using different wrapper for similar scenarios. We currently have 3 asymmetric cases. chrome->content, chrome->nsEP, nsEP->content and we use COW for the first 2 but not in the third case which makes no sense to me.

Because now I have weird questions like: content does Function.prototype = functionProtoFromAnNSEPscope. So it's an opaque wrapper and content does not have access to it. And then content gets a COW from some chrome scope, where in the proto chain we find Function.prototype and we just hook in the contents current Function.prototype which is an opaque wrapper right now. I kind of have the feeling that from this point on the COWs PropIsFromStandardPrototype will be tricked and will let content access the Function.prototype from the nsEP scope...

I have not tried this out at all just have a bad feeling about it.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5)
> So I really dislike the idea of using different wrapper for similar
> scenarios. We currently have 3 asymmetric cases. chrome->content,
> chrome->nsEP, nsEP->content and we use COW for the first 2 but not in the
> third case which makes no sense to me.

Well, we given that we're deprecating __exposedProps__, we really don't want to be introducing COWs into new places where we didn't have them before. In particular, I don't want jetpack authors to start using them and then have us snatch them away.

> Because now I have weird questions like: content does Function.prototype =
> functionProtoFromAnNSEPscope. So it's an opaque wrapper and content does not
> have access to it. And then content gets a COW from some chrome scope, where
> in the proto chain we find Function.prototype and we just hook in the
> contents current Function.prototype which is an opaque wrapper right now. I
> kind of have the feeling that from this point on the COWs
> PropIsFromStandardPrototype will be tricked and will let content access the
> Function.prototype from the nsEP scope...

Oh hm, that's an extremely clever idea. I _think_ we're safe from this because we don't allow munging the __proto__ on security wrappers (after bug 809652), but I think you're right that this is sketchy. gabor++

I think it probably makes sense to just always remap standard prototypes if the wrapper does not subsume the wrapee, regardless of whether or not we'll end up using a COW. This seems strictly safer, and I can't think of any reason why this would be a compat hit. I'll modify the patches to do this now.

Comment 7

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #6)
> Well, we given that we're deprecating __exposedProps__, we really don't want
> to be introducing COWs into new places where we didn't have them before. In
> particular, I don't want jetpack authors to start using them and then have
> us snatch them away.

I don't want to rely on __exposedProps__ in any shape or form, and nsEP are just about to be used in jetpack, so it's not like we introduce COWs where there weren't before. I just want to use the same js based object exporting helper functions in all 3 cases. And any differences there is likely will bite me sooner or later. But this is not the bug where we should discuss this, this should be a separate bug/patch/discussion if we decide to do it.

> I think it probably makes sense to just always remap standard prototypes if
> the wrapper does not subsume the wrapee, regardless of whether or not we'll
> end up using a COW. This seems strictly safer, and I can't think of any
> reason why this would be a compat hit. I'll modify the patches to do this
> now.

Sounds good to me let's see how our tests like it.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 2 - Handle standard prototype remapping in PrepareForWrapping. v2

Comment 9

[Bobby Holley (:bholley)]

Attachment: Tests. v2

Comment 10

[Bobby Holley (:bholley)]

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

Comment 11

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5)
> +            if (!JS_GetClassPrototype(cx, key, homeProto.address()))
> +                return NULL;
> 
> nullptr

I fixed this locally, but not in the uploaded patch. Sorry about that.

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 744023 [details] [diff] [review]
Part 1 - Don't create waivers in WaiveXrayAndWrap if the caller has no business waiving. v1

Gabor points out that I obsoleted the wrong patch.

Comment 13

[Bobby Holley (:bholley)]

This is green.

Comment 14

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 744023 [details] [diff] [review]
Part 1 - Don't create waivers in WaiveXrayAndWrap if the caller has no business waiving. v1

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

I was flagged as feedback?, now I hope I did the right thing with the flags...

Comment 15

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 744207 [details] [diff] [review]
Part 2 - Handle standard prototype remapping in PrepareForWrapping. v2

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

Please update the comment too.

Comment 16

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #14)
> I was flagged as feedback?, now I hope I did the right thing with the
> flags...

Oh, sorry. I meant to flag you for review, but I flagged you for checkin? instead. Anyway, looks like it's all sorted out. :-)

Comment 17

[Bobby Holley (:bholley)]

Comment on attachment 744207 [details] [diff] [review]
Part 2 - Handle standard prototype remapping in PrepareForWrapping. v2

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not easily at all. This is pretty complicated stuff, and the fix here is a general "move code around" that does very little to indicate what the real problem is and how it might be exploited.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

There's a comment explaining part 1 a bit, but that's not really the central flaw or piece of this patch.

Which older supported branches are affected by this flaw?

All

If not all supported branches, which bug introduced the flaw?

bug 760109 (landed on mozilla17)

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Should be straightforward.

How likely is this patch to cause regressions; how much testing does it need?

Not too likely, but this changes subtle Xray behavior that it would be good to test.

Comment 18

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

Comment on attachment 744207 [details] [diff] [review]
Part 2 - Handle standard prototype remapping in PrepareForWrapping. v2

sec-approval+ for M-C after 5/14. Please prepare branch patches and nominate at that time. Waaay too late to take in this cycle. :-)

Comment 19

[Bobby Holley (:bholley)]

Note to self - these patches are in |cow_proto_fix|.

Comment 22

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/35ff2341e914
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/338af3238e84

Comment 23

[Bobby Holley (:bholley)]

Comment on attachment 744207 [details] [diff] [review]
Part 2 - Handle standard prototype remapping in PrepareForWrapping. v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 760109
User impact if declined: security bugs
Testing completed: just pushed to inbound
Risk to taking this patch (and alternatives if risky): Not too likely, but this changes subtle Xray behavior that it would be good to test.
String or UUID changes made by this patch: None

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/35ff2341e914

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/338af3238e84

Comment 26

[Alex Keybl [:akeybl]]

(In reply to Bobby Holley (:bholley) from comment #23)
> Risk to taking this patch (and alternatives if risky): Not too likely, but
> this changes subtle Xray behavior that it would be good to test.

What's the best way to test?

Comment 27

[Bobby Holley (:bholley)]

(In reply to Alex Keybl [:akeybl] from comment #26)
> (In reply to Bobby Holley (:bholley) from comment #23)
> > Risk to taking this patch (and alternatives if risky): Not too likely, but
> > this changes subtle Xray behavior that it would be good to test.
> 
> What's the best way to test?

I just mean baking it for as long as possible (that is to say, uplift now).

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Bobby, I looked into uplifting this to b2g18 and it had some non-trivial conflicts. Presumably it's the same for esr17. Can you please prepare branch patches for uplift?

Comment 29

[Bobby Holley (:bholley)]

Patch 1 should apply everywhere. I'll create branch version for patch 2.

Comment 30

[Bobby Holley (:bholley)]

Attachment: Part 2 - Handle standard prototype remapping in PrepareForWrapping on esr17. r=gabor,mrbkap

Comment 31

[Bobby Holley (:bholley)]

Attachment: part 2 - Handle standard prototype remapping in PrepareForWrapping on b2g18. r=gabor,mrbkap

Comment 32

[Bobby Holley (:bholley)]

Attachment: Handle standard prototype remapping in PrepareForWrapping on beta. r=gabor,mrbkap

These patches were written against what is now aurora, so there shouldn't be any conflicts there.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f0ab73e4906a
https://hg.mozilla.org/releases/mozilla-aurora/rev/08ddc20e3a71

https://hg.mozilla.org/releases/mozilla-beta/rev/d625da8c0ee7
https://hg.mozilla.org/releases/mozilla-beta/rev/6f4773156c56

https://hg.mozilla.org/releases/mozilla-b2g18/rev/d298e1b4eaf7
https://hg.mozilla.org/releases/mozilla-b2g18/rev/0fedc9953d64

https://hg.mozilla.org/releases/mozilla-esr17/rev/45be8334f34f
https://hg.mozilla.org/releases/mozilla-esr17/rev/b23fc7520b99

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Backed out from esr17 for assertions/crashes. Looks like b2g18 is OK, though.
https://hg.mozilla.org/releases/mozilla-esr17/rev/8d56319d41df

https://tbpl.mozilla.org/php/getParsedLog.php?id=23154421&tree=Mozilla-Esr17

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/a76cdc2f5171
https://hg.mozilla.org/releases/mozilla-esr17/rev/dd4b5caa371c

Comment 36

[Ioana (away)]

(In reply to Bobby Holley (:bholley) from comment #27)
> (In reply to Alex Keybl [:akeybl] from comment #26)
> > What's the best way to test?
> 
> I just mean baking it for as long as possible (that is to say, uplift now).

How can QA bake this with manual testing? Any kind of guidelines, links etc would be useful

Comment 37

[Bobby Holley (:bholley)]

(In reply to Ioana Budnar, QA [:ioana] from comment #36)
> (In reply to Bobby Holley (:bholley) from comment #27)
> > (In reply to Alex Keybl [:akeybl] from comment #26)
> > > What's the best way to test?
> > 
> > I just mean baking it for as long as possible (that is to say, uplift now).
> 
> How can QA bake this with manual testing? Any kind of guidelines, links etc
> would be useful

I don't think there's all that much QA can do here aside from just generally making sure that addons behave as they should. I don't think the effort/reward ratio for manual QA is all that great here.

Comment 38

[u279076]

Marking this bug [qa-] as per comment 37.

Comment 39

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

Marking as a minus for advisories since it is part of the work for bug 863933, which is an external report getting an advisory and which got the bounty.

Comment 40

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b8cf7425dfe

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9b8cf7425dfe