Last Comment Bug 866823 - Xray Waivers can be used to bypass COWs
: Xray Waivers can be used to bypass COWs
Status: RESOLVED FIXED
[qa-][adv-main22-][adv-esr1707-]
: sec-critical
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla24
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
: Matt Wobensmith [:mwobensmith][:matt:]
Mentors:
Depends on: 874083
Blocks: 742444 CVE-2013-1687
  Show dependency treegraph
 
Reported: 2013-04-29 11:12 PDT by Bobby Holley (:bholley) (busy with Stylo)
Modified: 2014-07-24 16:09 PDT (History)
13 users (show)
dveditz: sec‑bounty+
cbook: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
+
fixed
+
fixed
fixed
22+
fixed
22+
fixed
wontfix
affected


Attachments
Part 1 - Don't create waivers in WaiveXrayAndWrap if the caller has no business waiving. v1 (1.62 KB, patch)
2013-04-30 21:21 PDT, Bobby Holley (:bholley) (busy with Stylo)
gkrizsanits: review+
mrbkap: review+
Details | Diff | Splinter Review
Part 2 - Handle standard prototype remapping in PrepareForWrapping. v1 (7.53 KB, patch)
2013-04-30 21:21 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
Tests. v1 (2.82 KB, patch)
2013-04-30 21:22 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
Part 2 - Handle standard prototype remapping in PrepareForWrapping. v2 (4.79 KB, patch)
2013-05-01 11:29 PDT, Bobby Holley (:bholley) (busy with Stylo)
gkrizsanits: review+
mrbkap: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr17+
akeybl: approval‑mozilla‑b2g18+
abillings: sec‑approval+
Details | Diff | Splinter Review
Tests. v2 (3.14 KB, patch)
2013-05-01 11:30 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
gkrizsanits: review+
Details | Diff | Splinter Review
Part 2 - Handle standard prototype remapping in PrepareForWrapping on esr17. r=gabor,mrbkap (7.34 KB, patch)
2013-05-18 20:33 PDT, Bobby Holley (:bholley) (busy with Stylo)
bobbyholley: review+
Details | Diff | Splinter Review
part 2 - Handle standard prototype remapping in PrepareForWrapping on b2g18. r=gabor,mrbkap (5.13 KB, patch)
2013-05-18 20:34 PDT, Bobby Holley (:bholley) (busy with Stylo)
bobbyholley: review+
Details | Diff | Splinter Review
Handle standard prototype remapping in PrepareForWrapping on beta. r=gabor,mrbkap (5.03 KB, patch)
2013-05-18 20:34 PDT, Bobby Holley (:bholley) (busy with Stylo)
bobbyholley: review+
Details | Diff | Splinter Review

Description Bobby Holley (:bholley) (busy with Stylo) 2013-04-29 11:12:48 PDT
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) (busy with Stylo) 2013-04-30 21:21:35 PDT
Created attachment 744023 [details] [diff] [review]
Part 1 - Don't create waivers in WaiveXrayAndWrap if the caller has no business waiving. v1
Comment 2 Bobby Holley (:bholley) (busy with Stylo) 2013-04-30 21:21:50 PDT
Created attachment 744024 [details] [diff] [review]
Part 2 - Handle standard prototype remapping in PrepareForWrapping. v1
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2013-04-30 21:22:05 PDT
Created attachment 744025 [details] [diff] [review]
Tests. v1
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2013-04-30 21:26:30 PDT
https://tbpl.mozilla.org/?tree=Try&rev=cd0c5a3c5145
Comment 5 Gabor Krizsanits [:krizsa :gabor] 2013-05-01 04:52:55 PDT
+            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) (busy with Stylo) 2013-05-01 10:18:54 PDT
(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 [:krizsa :gabor] 2013-05-01 10:37:48 PDT
(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) (busy with Stylo) 2013-05-01 11:29:49 PDT
Created attachment 744207 [details] [diff] [review]
Part 2 - Handle standard prototype remapping in PrepareForWrapping. v2
Comment 9 Bobby Holley (:bholley) (busy with Stylo) 2013-05-01 11:30:12 PDT
Created attachment 744208 [details] [diff] [review]
Tests. v2
Comment 10 Bobby Holley (:bholley) (busy with Stylo) 2013-05-01 11:31:24 PDT
https://tbpl.mozilla.org/?tree=Try&rev=60bcb69eb133
Comment 11 Bobby Holley (:bholley) (busy with Stylo) 2013-05-01 11:34:27 PDT
(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) (busy with Stylo) 2013-05-01 11:45:34 PDT
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) (busy with Stylo) 2013-05-01 13:54:12 PDT
This is green.
Comment 14 Gabor Krizsanits [:krizsa :gabor] 2013-05-02 02:18:56 PDT
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 [:krizsa :gabor] 2013-05-02 02:20:41 PDT
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) (busy with Stylo) 2013-05-02 09:55:32 PDT
(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) (busy with Stylo) 2013-05-06 16:36:13 PDT
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] 2013-05-07 10:24:50 PDT
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) (busy with Stylo) 2013-05-07 13:47:37 PDT
Note to self - these patches are in |cow_proto_fix|.
Comment 23 Bobby Holley (:bholley) (busy with Stylo) 2013-05-15 21:15:47 PDT
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] 2013-05-16 14:12:00 PDT
https://hg.mozilla.org/mozilla-central/rev/35ff2341e914
Comment 25 Ryan VanderMeulen [:RyanVM] 2013-05-16 14:12:40 PDT
https://hg.mozilla.org/mozilla-central/rev/338af3238e84
Comment 26 Alex Keybl [:akeybl] 2013-05-17 15:32:51 PDT
(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) (busy with Stylo) 2013-05-17 16:23:17 PDT
(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] 2013-05-17 18:40:10 PDT
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) (busy with Stylo) 2013-05-18 15:27:45 PDT
Patch 1 should apply everywhere. I'll create branch version for patch 2.
Comment 30 Bobby Holley (:bholley) (busy with Stylo) 2013-05-18 20:33:31 PDT
Created attachment 751420 [details] [diff] [review]
Part 2 - Handle standard prototype remapping in PrepareForWrapping on esr17. r=gabor,mrbkap
Comment 31 Bobby Holley (:bholley) (busy with Stylo) 2013-05-18 20:34:00 PDT
Created attachment 751421 [details] [diff] [review]
part 2 - Handle standard prototype remapping in PrepareForWrapping on b2g18. r=gabor,mrbkap
Comment 32 Bobby Holley (:bholley) (busy with Stylo) 2013-05-18 20:34:39 PDT
Created attachment 751422 [details] [diff] [review]
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 34 Ryan VanderMeulen [:RyanVM] 2013-05-20 06:35:48 PDT
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 36 Ioana (away) 2013-05-23 07:27:08 PDT
(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) (busy with Stylo) 2013-05-28 11:18:05 PDT
(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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-05-30 13:30:17 PDT
Marking this bug [qa-] as per comment 37.
Comment 39 Al Billings [:abillings] 2013-06-18 15:39:08 PDT
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) (busy with Stylo) 2013-11-20 16:04:56 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b8cf7425dfe
Comment 41 Carsten Book [:Tomcat] 2013-11-21 05:48:46 PST
https://hg.mozilla.org/mozilla-central/rev/9b8cf7425dfe

Note You need to log in before you can comment on or make changes to this bug.