843829 - (CVE-2013-1711) XBL scopes can be fooled by invoking XBL functions with non-native arguments

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Bobby Holley (:bholley)]

This is the root cause of bug 842255. Kudos for moz_bug_r_a4 for the clever test case. TL;DR - This allows clever pages to potentially circumvent the security advantages of XBL scopes and apply some of the same attacks that were possible on trunk before XBL scopes landed.

Generally when privileged code gets its hands on a raw content JS object, it's the result of waiving Xray, and the object has an Xray waiver around it. However, there are cases where privileged code can get its hands on a content JS object without a waiver. The most obvious way to do this is to just do

var withWaiver = contentWindow.wrappedJSObject.someExpando;
var withoutWaiver = XPCNativeWrapper(withWaiver);

In this case, we have |withWaiver != withoutWaiver|, because they're separate object identities. The former transitively applies Xray waivers, while the latter is just biding its time until it finds a native object, at which point Xray vision kicks back in, and content JS objects should theoretically be unreachable again.

One could make the argument that we should have safer behavior here, and that objects without waivers should be opaque. I'm pretty sure that ship has sailed though (if it was ever even in port), and that changing that would break too much. Moreover, we actually rely on it with the XBL scope machinery: when an XBL scope accesses a field off of an bound node with Xray vision, we grab the underlying JS object (since there's not really much else to grab), but specifically avoid waiving Xray, so that Xray vision returns as soon as the caller traverses back to a node. One could make the argument taht this isn't very important behavior to preserve, since clever content can always circumvent it once the caller enters JS.

Anyway. This normally isn't a security problem, because the Xray layer between chrome and content is generally complete modulo instances of waiving. However, in the XBL case we put the wrapped XBL functions on the content-accessible prototype, which content can then invoke. And it can invoke these function with arbitrary JS objects for |this| and the associated arguments, which the XBL generally does not expect.

I can think of two solutions here:

1 - For XBL scopes, make non-waived JS object use an Opaque wrapper rather than a CrossCompartmentWrapper. This would mean that we'd have to deep-waive for field access, but maybe that's ok.

2 - If we decide it's not OK, we could implement a two-tiered system of waiving. One would be deep waiving, which we have right now, and the other would be "just waive until I get back to a native object", which is currently the no-waiver behavior. This would allow us to make the no-waiver behavior opaque. This is probably a fair amount more code than option (1).

Blake, Boris - Can you guys weigh in? Do you think it's ok to deep-waive for field access?

Comment 1

[Boris Zbarsky [:bzbarsky]]

I have to admit to not understanding most of comment 0... I clearly need to read some sort of primer on what this "waiver" stuff is.  Do we have one?

Comment 2

[Bobby Holley (:bholley)]

Per discussion with bz, I realized that we can _also_ fix bug 842255 by only checking the XBL bit if XBL scopes are turned off. We should probably do both. I'll file another bug.

Comment 3

[Bobby Holley (:bholley)]

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

Comment 4

[Bobby Holley (:bholley)]

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

Comment 5

[Bobby Holley (:bholley)]

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

Comment 6

[Bobby Holley (:bholley)]

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

Comment 7

[Bobby Holley (:bholley)]

So, this is now all green, except for one reftest orange: layout/reftests/xul/accesskey.xul

This test instantiates some XUL <label control=""> elements in content, which end up picking up the label-control binding and calling wrapChar in its constructor, here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/text.xml#189

Long story short is that the XBL binding attempts to use constants off DOM interface objects (NodeFilter.SHOW_TEXT), which we rely on inheriting from content via the sandboxPrototype. It would appear that we don't have useful Xrays in this case, so we're relying on non-Xrayed interface objects being visible to the XBL scope, which is exactly what we're trying to turn off here.

I think we need a solution for interface objects and DOM prototype before landing this. Peterv, what's the status on all that stuff? How close are we to having proper Xray prototypes to all DOM stuff?

Comment 8

[Peter Van der Beken [:peterv]]

(In reply to Bobby Holley (:bholley) from comment #7)
> How close are we
> to having proper Xray prototypes to all DOM stuff?

We already have it. You're runing into a bug related to callbacks (which NodeFilter is). We use a generic JSObject (js::ObjectClass) for callbacks and so we can't hang the NativePropertyHooks off of that, so the Xray can't look up the native properties. I'm not sure why we don't use a DOMIfaceAndProtoJSClass for them. Bz?

Comment 9

[Boris Zbarsky [:bzbarsky]]

According to the blame, we stopped doing that when we started doing the hasInstance magic, since for callbacks there is no prototype object.  See http://hg.mozilla.org/mozilla-central/rev/9a12a0f8c8be

We might be able to undo that change as long as we ensure that these objects get a null hasInstance hook in the JSClass.

Comment 10

[Bobby Holley (:bholley)]

Rehydrated these patches and pushed to try:

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

Comment 11

[Bobby Holley (:bholley)]

Woohoo, this is now green! Uploading patches and flagging for review.

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 1 - Fix incorrect test. v1

This wasn't actually testing anything, because |is| is defined in the scope of
the content, so by passing the objects as arguments, we end up re-wrapping them
in the content scope, where the distinction between waived and non-waived
objects doesn't exist.

We're actually just about to remove this test in the next patch, but I wanted
to make it correct first. :-)

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 2 - Apply transitive waivers for nativeCall. v1

Comment 14

[Bobby Holley (:bholley)]

Attachment: Part 3 - Stop using IsTransparent for XBL field access, and explicitly waive instead. v1

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 4 - Explicitly add a waiver in FieldGetter and FieldSetter. v1

Comment 16

[Bobby Holley (:bholley)]

Attachment: Part 5 - Wrap unwaived content JS objects in opaque wrappers for XBL scopes. v2

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 730936 [details] [diff] [review]
Part 2 - Apply transitive waivers for nativeCall. v1

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

::: js/xpconnect/wrappers/WaiveXrayWrapper.cpp
@@ +85,5 @@
>             WrapperFactory::WaiveXrayAndWrap(cx, rval.address());
>  }
>  
> +bool
> +WaiveXrayWrapper::nativeCall(JSContext *cx, JS::IsAcceptableThis test,

I would like a comment here explaining that this is the other side of a "secret" handshake with FieldGetter, it's pretty subtle only looking at this code.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 730938 [details] [diff] [review]
Part 4 - Explicitly add a waiver in FieldGetter and FieldSetter. v1

Whew indeed. Thanks for the detailed comment, it made understanding this much, much nicer.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 730939 [details] [diff] [review]
Part 5 - Wrap unwaived content JS objects in opaque wrappers for XBL scopes. v2

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

::: js/xpconnect/wrappers/AccessCheck.h
@@ +67,5 @@
> +    static bool allowNativeCall(JSContext *cx, JS::IsAcceptableThis test, JS::NativeImpl impl)
> +    {
> +        // We allow nativeCall here because the alternative is throwing (which
> +        // happens in SecurityWrapper::nativeCall), which we don't want. There's
> +        // unlikely to be too much harm to letting this through.

I agree with the last sentence because allowing a native call from privileged code to less privileged code can only *drop* privileges, so I'd call that out in the comment here. Otherwise, this looks like a security hole waiting to happen.

Comment 20

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/97d16e7beb27
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2e889cd77a48
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/57652d8f0827
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/64f001fe04fb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1df3bdadb7ce

Comment 21

[(no longer active)]

Backed out because of mochitest-5 failures:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e60919ded783
https://tbpl.mozilla.org/php/getParsedLog.php?id=21369981&tree=Mozilla-Inbound

Comment 22

[Bobby Holley (:bholley)]

Just a stupid osx-only XUL test that should have been a chrome test. Fixed up and repushed.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0f69c8e92e3a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e4c0126aa316
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dbcbbf02fcbd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5b1b1d6792e0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/984df1da7b15
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7c0a42b907d6

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0f69c8e92e3a
https://hg.mozilla.org/mozilla-central/rev/e4c0126aa316
https://hg.mozilla.org/mozilla-central/rev/dbcbbf02fcbd
https://hg.mozilla.org/mozilla-central/rev/5b1b1d6792e0
https://hg.mozilla.org/mozilla-central/rev/984df1da7b15
https://hg.mozilla.org/mozilla-central/rev/7c0a42b907d6

Comment 24

[Bobby Holley (:bholley)]

(Tests contained in part 5)

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Should this be marked FIXED?

Comment 26

[Bobby Holley (:bholley)]

(In reply to Jonas Sicking (:sicking) from comment #25)
> Should this be marked FIXED?

I think so, yeah.