Closed Bug 843829 (CVE-2013-1711) Opened 7 years ago Closed 7 years ago

XBL scopes can be fooled by invoking XBL functions with non-native arguments


(Core :: XBL, defect)

Not set



Tracking Status
firefox22 --- wontfix
firefox23 --- fixed
firefox24 --- fixed
firefox-esr17 --- wontfix
b2g18 --- wontfix


(Reporter: bholley, Assigned: bholley)



(Keywords: sec-moderate, Whiteboard: [adv-main23+] Embargo until EOL ESR17/B2G18)


(5 files)

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?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(mrbkap)
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?
Flags: needinfo?(bzbarsky)
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.
Depends on: 845862
Flags: needinfo?(mrbkap) → needinfo?
Depends on: 848939
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:

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?
Flags: needinfo?(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?
Flags: needinfo?(peterv) → needinfo?(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

We might be able to undo that change as long as we ensure that these objects get a null hasInstance hook in the JSClass.
Flags: needinfo?(bzbarsky)
Rehydrated these patches and pushed to try:
Woohoo, this is now green! Uploading patches and flagging for review.
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. :-)
Attachment #730935 - Flags: review?(mrbkap)
Blocks: 856067
Attachment #730935 - Flags: review?(mrbkap) → review+
Attachment #730936 - Flags: review?(mrbkap) → review+
Attachment #730937 - Flags: review?(mrbkap) → review+
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 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.
Attachment #730938 - Flags: review?(mrbkap) → review+
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.
Attachment #730939 - Flags: review?(mrbkap) → review+
(Tests contained in part 5)
Flags: in-testsuite? → in-testsuite+
(In reply to Jonas Sicking (:sicking) from comment #25)
> Should this be marked FIXED?

I think so, yeah.
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [adv-main23+]
Alias: CVE-2013-1711
Whiteboard: [adv-main23+] → [adv-main23+] Embargo until EOL ESR17/B2G18
Group: core-security
You need to log in before you can comment on or make changes to this bug.