The default bug view has changed. See this FAQ.

Xray Waivers can be used to bypass COWs

RESOLVED FIXED in Firefox 22

Status

()

Core
XPConnect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

({sec-critical})

unspecified
mozilla24
sec-critical
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +
in-testsuite +

Firefox Tracking Flags

(firefox20 wontfix, firefox21 wontfix, firefox22+ fixed, firefox23+ fixed, firefox24 fixed, firefox-esr1722+ fixed, b2g1822+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 affected)

Details

(Whiteboard: [qa-][adv-main22-][adv-esr1707-])

Attachments

(6 attachments, 2 obsolete attachments)

1.62 KB, patch
krizsa
: review+
mrbkap
: review+
Details | Diff | Splinter Review
4.79 KB, patch
krizsa
: review+
mrbkap
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
3.14 KB, patch
mrbkap
: review+
krizsa
: review+
Details | Diff | Splinter Review
7.34 KB, patch
bholley
: review+
Details | Diff | Splinter Review
5.13 KB, patch
bholley
: review+
Details | Diff | Splinter Review
5.03 KB, patch
bholley
: review+
Details | Diff | Splinter Review
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.
Blocks: 742444
Created attachment 744023 [details] [diff] [review]
Part 1 - Don't create waivers in WaiveXrayAndWrap if the caller has no business waiving. v1
Attachment #744023 - Flags: review?(mrbkap)
Attachment #744023 - Flags: review?(gkrizsanits)
Created attachment 744024 [details] [diff] [review]
Part 2 - Handle standard prototype remapping in PrepareForWrapping. v1
Attachment #744024 - Flags: review?(mrbkap)
Attachment #744024 - Flags: review?(gkrizsanits)
Created attachment 744025 [details] [diff] [review]
Tests. v1
Attachment #744025 - Flags: review?(mrbkap)
Attachment #744025 - Flags: review?(gkrizsanits)
https://tbpl.mozilla.org/?tree=Try&rev=cd0c5a3c5145
+            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.
(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.
(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.
Created attachment 744207 [details] [diff] [review]
Part 2 - Handle standard prototype remapping in PrepareForWrapping. v2
Attachment #744023 - Attachment is obsolete: true
Attachment #744023 - Flags: review?(mrbkap)
Attachment #744023 - Flags: review?(gkrizsanits)
Attachment #744207 - Flags: review?(mrbkap)
Attachment #744207 - Flags: review?(gkrizsanits)
Created attachment 744208 [details] [diff] [review]
Tests. v2
Attachment #744025 - Attachment is obsolete: true
Attachment #744025 - Flags: review?(mrbkap)
Attachment #744025 - Flags: review?(gkrizsanits)
Attachment #744208 - Flags: review?(mrbkap)
Attachment #744208 - Flags: review?(gkrizsanits)
https://tbpl.mozilla.org/?tree=Try&rev=60bcb69eb133
(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 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.
Attachment #744023 - Attachment is obsolete: false
Attachment #744023 - Flags: review?(mrbkap)
Attachment #744023 - Flags: checkin?(gkrizsanits)
Attachment #744024 - Attachment is obsolete: true
Attachment #744024 - Flags: review?(mrbkap)
Attachment #744024 - Flags: review?(gkrizsanits)
This is green.
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...
Attachment #744023 - Flags: review?(mrbkap)
Attachment #744023 - Flags: review+
Attachment #744023 - Flags: checkin?(gkrizsanits)
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.
Attachment #744207 - Flags: review?(gkrizsanits) → review+
Attachment #744208 - Flags: review?(gkrizsanits) → review+
(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. :-)

Updated

4 years ago
Attachment #744023 - Flags: review?(mrbkap) → review+

Updated

4 years ago
Attachment #744207 - Flags: review?(mrbkap) → review+

Updated

4 years ago
Attachment #744208 - Flags: review?(mrbkap) → review+
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.
Attachment #744207 - Flags: sec-approval?
status-b2g18: --- → affected
status-firefox20: --- → affected
status-firefox21: --- → affected
status-firefox22: --- → affected
status-firefox23: --- → affected
status-firefox-esr17: --- → affected
tracking-b2g18: --- → ?
tracking-firefox22: --- → +
tracking-firefox23: --- → +
tracking-firefox-esr17: --- → +
Whiteboard: [checkin after 5/14]
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. :-)
Attachment #744207 - Flags: sec-approval? → sec-approval+
status-firefox20: affected → wontfix
status-firefox21: affected → wontfix
tracking-b2g18: ? → +
tracking-firefox-esr17: + → 22+
Note to self - these patches are in |cow_proto_fix|.
Flags: in-testsuite?
Flags: sec-bounty+
Keywords: sec-high → sec-critical
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/35ff2341e914
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/338af3238e84
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
Attachment #744207 - Flags: approval-mozilla-esr17?
Attachment #744207 - Flags: approval-mozilla-beta?
Attachment #744207 - Flags: approval-mozilla-b2g18?
Attachment #744207 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/35ff2341e914
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → affected
status-firefox24: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
https://hg.mozilla.org/mozilla-central/rev/338af3238e84
Whiteboard: [checkin after 5/14]
(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?
tracking-b2g18: + → 22+
Keywords: qawanted, verifyme
QA Contact: mwobensmith

Updated

4 years ago
Attachment #744207 - Flags: approval-mozilla-esr17?
Attachment #744207 - Flags: approval-mozilla-esr17+
Attachment #744207 - Flags: approval-mozilla-beta?
Attachment #744207 - Flags: approval-mozilla-beta+
Attachment #744207 - Flags: approval-mozilla-b2g18?
Attachment #744207 - Flags: approval-mozilla-b2g18+
Attachment #744207 - Flags: approval-mozilla-aurora?
Attachment #744207 - Flags: approval-mozilla-aurora+
(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).
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?
Keywords: branch-patch-needed
Patch 1 should apply everywhere. I'll create branch version for patch 2.
Created attachment 751420 [details] [diff] [review]
Part 2 - Handle standard prototype remapping in PrepareForWrapping on esr17. r=gabor,mrbkap
Attachment #751420 - Flags: review+
Created attachment 751421 [details] [diff] [review]
part 2 - Handle standard prototype remapping in PrepareForWrapping on b2g18. r=gabor,mrbkap
Attachment #751421 - Flags: review+
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.
Attachment #751422 - Flags: review+
Keywords: branch-patch-needed
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
status-b2g18: affected → fixed
status-firefox22: affected → fixed
status-firefox23: affected → fixed
status-firefox-esr17: affected → fixed
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
status-firefox-esr17: fixed → affected
Depends on: 874083
https://hg.mozilla.org/releases/mozilla-esr17/rev/a76cdc2f5171
https://hg.mozilla.org/releases/mozilla-esr17/rev/dd4b5caa371c
status-b2g18-v1.0.1: affected → fixed
status-b2g18-v1.0.1: fixed → affected
status-firefox-esr17: affected → fixed

Comment 36

4 years ago
(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
(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.
Marking this bug [qa-] as per comment 37.
Keywords: qawanted, verifyme
Whiteboard: [qa-]
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.
Whiteboard: [qa-] → [qa-][adv-main22-]
Whiteboard: [qa-][adv-main22-] → [qa-][adv-main22-][adv-esr1707-]
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b8cf7425dfe
https://hg.mozilla.org/mozilla-central/rev/9b8cf7425dfe
status-firefox28: --- → fixed
Flags: in-testsuite? → in-testsuite+
status-firefox28: fixed → ---
Group: core-security
You need to log in before you can comment on or make changes to this bug.