Closed Bug 866823 Opened 7 years ago Closed 7 years ago

Xray Waivers can be used to bypass COWs

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox20 --- wontfix
firefox21 --- wontfix
firefox22 + fixed
firefox23 + fixed
firefox24 --- fixed
firefox-esr17 22+ fixed
b2g18 22+ fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- affected

People

(Reporter: bholley, Assigned: bholley)

References

Details

(Keywords: sec-critical, Whiteboard: [qa-][adv-main22-][adv-esr1707-])

Attachments

(6 files, 2 obsolete files)

1.62 KB, patch
gkrizsanits
: review+
mrbkap
: review+
Details | Diff | Splinter Review
4.79 KB, patch
gkrizsanits
: review+
mrbkap
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
3.14 KB, patch
mrbkap
: review+
gkrizsanits
: 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
Attachment #744024 - Flags: review?(mrbkap)
Attachment #744024 - Flags: review?(gkrizsanits)
Attached patch Tests. v1 (obsolete) — Splinter Review
Attachment #744025 - Flags: review?(mrbkap)
Attachment #744025 - Flags: review?(gkrizsanits)
+            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.
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)
Attached patch Tests. v2Splinter Review
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)
(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. :-)
Attachment #744023 - Flags: review?(mrbkap) → review+
Attachment #744207 - Flags: review?(mrbkap) → review+
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?
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+
Note to self - these patches are in |cow_proto_fix|.
Flags: in-testsuite?
Flags: sec-bounty+
Keywords: sec-highsec-critical
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
(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?
Keywords: qawanted, verifyme
QA Contact: mwobensmith
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?
Patch 1 should apply everywhere. I'll create branch version for patch 2.
These patches were written against what is now aurora, so there shouldn't be any conflicts there.
Attachment #751422 - Flags: review+
Depends on: 874083
(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-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.