Last Comment Bug 843829 - (CVE-2013-1711) XBL scopes can be fooled by invoking XBL functions with non-native arguments
(CVE-2013-1711)
: XBL scopes can be fooled by invoking XBL functions with non-native arguments
Status: RESOLVED FIXED
[adv-main23+] Embargo until EOL ESR17...
: sec-moderate
Product: Core
Classification: Components
Component: XBL (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla23
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
Mentors:
Depends on: 845862 848939 855025
Blocks: CVE-2013-1671 856067
  Show dependency treegraph
 
Reported: 2013-02-21 14:31 PST by Bobby Holley (:bholley) (busy with Stylo)
Modified: 2014-11-19 19:52 PST (History)
16 users (show)
bobbyholley: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
fixed
fixed
wontfix
wontfix


Attachments
Part 1 - Fix incorrect test. v1 (1.50 KB, patch)
2013-03-28 15:42 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 2 - Apply transitive waivers for nativeCall. v1 (2.00 KB, patch)
2013-03-28 15:42 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 3 - Stop using IsTransparent for XBL field access, and explicitly waive instead. v1 (8.74 KB, patch)
2013-03-28 15:42 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 4 - Explicitly add a waiver in FieldGetter and FieldSetter. v1 (4.97 KB, patch)
2013-03-28 15:42 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 5 - Wrap unwaived content JS objects in opaque wrappers for XBL scopes. v2 (13.11 KB, patch)
2013-03-28 15:43 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review

Description Bobby Holley (:bholley) (busy with Stylo) 2013-02-21 14:31:50 PST
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 [:bz] 2013-02-21 17:11:23 PST
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) (busy with Stylo) 2013-02-22 10:21:45 PST
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) (busy with Stylo) 2013-02-27 12:18:27 PST
https://tbpl.mozilla.org/?tree=Try&rev=1802c5e1b108
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2013-03-05 15:37:02 PST
https://tbpl.mozilla.org/?tree=Try&rev=e97bdb3fd796
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2013-03-06 13:03:51 PST
https://tbpl.mozilla.org/?tree=Try&rev=df34e0554ee6
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2013-03-07 13:40:07 PST
https://tbpl.mozilla.org/?tree=Try&rev=75b3aca33147
Comment 7 Bobby Holley (:bholley) (busy with Stylo) 2013-03-08 14:25:19 PST
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] - away till Aug 1st 2013-03-22 03:44:56 PDT
(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 [:bz] 2013-03-22 05:51:36 PDT
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) (busy with Stylo) 2013-03-28 10:34:43 PDT
Rehydrated these patches and pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=7092891b76c4
Comment 11 Bobby Holley (:bholley) (busy with Stylo) 2013-03-28 15:42:13 PDT
Woohoo, this is now green! Uploading patches and flagging for review.
Comment 12 Bobby Holley (:bholley) (busy with Stylo) 2013-03-28 15:42:21 PDT
Created attachment 730935 [details] [diff] [review]
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) (busy with Stylo) 2013-03-28 15:42:35 PDT
Created attachment 730936 [details] [diff] [review]
Part 2 - Apply transitive waivers for nativeCall. v1
Comment 14 Bobby Holley (:bholley) (busy with Stylo) 2013-03-28 15:42:46 PDT
Created attachment 730937 [details] [diff] [review]
Part 3 - Stop using IsTransparent for XBL field access, and explicitly waive instead. v1
Comment 15 Bobby Holley (:bholley) (busy with Stylo) 2013-03-28 15:42:58 PDT
Created attachment 730938 [details] [diff] [review]
Part 4 - Explicitly add a waiver in FieldGetter and FieldSetter. v1
Comment 16 Bobby Holley (:bholley) (busy with Stylo) 2013-03-28 15:43:11 PDT
Created attachment 730939 [details] [diff] [review]
Part 5 - Wrap unwaived content JS objects in opaque wrappers for XBL scopes. v2
Comment 17 Blake Kaplan (:mrbkap) 2013-04-02 16:36:58 PDT
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) 2013-04-02 16:40:58 PDT
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) 2013-04-02 16:57:44 PDT
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 24 Bobby Holley (:bholley) (busy with Stylo) 2013-04-03 20:34:35 PDT
(Tests contained in part 5)
Comment 25 Jonas Sicking (:sicking) PTO Until July 5th 2013-06-22 02:19:45 PDT
Should this be marked FIXED?
Comment 26 Bobby Holley (:bholley) (busy with Stylo) 2013-06-24 09:57:50 PDT
(In reply to Jonas Sicking (:sicking) from comment #25)
> Should this be marked FIXED?

I think so, yeah.

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