816071 - Content can confuse XBL with prototypes and can impersonate XBL script using eval() and Function() (bypassing SOWs)

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Cody Crews]

Attachment: stealanon.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0
Build ID: 20121119183901

Steps to reproduce:

I modified the prototype of XULElement for the current window to have a getter defined for setAttribute, then i included a video element to the document body.


Actual results:

Since the getter is called by the anonymous content its possible to grab a reference to the content using 'this'.  I also found it was possible to create a function at the time of execution to allow interaction with this content which otherwise would throw a security error.


Expected results:

The content provided by XBL should reference prototypes not available in the context of the current window when providing fully anonymous content.

Comment 1

[Cody Crews]

I haven't had time to go through what all could be done with this, but yeah i was hoping for the bug bounty.  This problem applies with any bindings that show up in the windows property names.  Ive also noticed another problem where when a binding is accessible through window property names (ie Object.getOwnPropertyNames(window)) that any functionality implemented by it is callable even though you should be dealing with a wrapped native prototype.  Ill file another bug for it later in the day if i have time, but it can be seen with any XML file where the XMLPrettyPrint.xml binding is applied.  If you access the binding through the above mentioned method you can call the observe function passing in "prettyprint-dom-created" as the topic to trigger the conditional "document.getAnonymousNodes(this).item(0).appendChild(aSubject);"  This would be considered a different bug altogether i would think, and by looking into the source the binding of XMLPrettyPrint.xml is done with system principal but again i haven't had a lot of free time lately to see how nasty this could all get.

Comment 2

[Boris Zbarsky [:bzbarsky]]

There's no sane way to use different protos for the XBL stuff.

What should happen, though, is that touching "this" in the getter should throw if it's anon content... I wonder why it doesn't.

Comment 3

[Cody Crews]

(In reply to Boris Zbarsky (:bz) from comment #2)
> There's no sane way to use different protos for the XBL stuff.
> 
> What should happen, though, is that touching "this" in the getter should
> throw if it's anon content... I wonder why it doesn't.

If you look at the approach i believe its because eval is being called binded to 'this', and since its a getter it then returns eval binded to whatever called the getter so its actually the anonymous content itself that is doing the work.  It calls the function returned from the getter thinking that function is setAttribute.  If there is no sane way to use different prototypes then having the prototypes exposed to the content window actually be wrappers might be a better idea.  This has all been a little hard to figure out exactly why it works this way without a proper way to debug things, firebug can only go so far.  Is there some tool that is used internally to see what things are, like when you are actually working with a wrapper etc?  Also i think it might be worth noting that if you do say Object.prototype.__defineGetter__("someFunction",function(){return this;}) that you can call someFunction on any object and be returned that very object(which was the basis for the above bug.)  Ive noticed quite a few things like this that i can't quite debug properly with firebug, and chromebug has been dead forever so its hard to figure out exactly what is going on.

Comment 4

[Boris Zbarsky [:bzbarsky]]

So the problem with using different prototypes is that it basically requires the XBL to use a separate global; otherwise instanceof and the like break.

That's a pretty big project, and likely to break various things.  :(

> Is there some tool that is used internally to see what things are, like when you are
> actually working with a wrapper etc? 

A C++ debugger....

Comment 5

[Cody Crews]

(In reply to Boris Zbarsky (:bz) from comment #4)
> So the problem with using different prototypes is that it basically requires
> the XBL to use a separate global; otherwise instanceof and the like break.
> 
> That's a pretty big project, and likely to break various things.  :(
> 
> > Is there some tool that is used internally to see what things are, like when you are
> > actually working with a wrapper etc? 
> 
> A C++ debugger....

I have visual studio setup with the symbols servers and even an alternate setup using vmware to replay but it all seems like overkill most of the time, i guess i was hoping for something a bit more contained with ff.  When it comes to working with these prototypes and XBL, there's other problems as i pointed out above, ill throw together a POC for that hopefully by this evening. The whole thing seems like a mess to me and I haven't dug deep enough yet to see exactly what bindings could be triggered by adding different types of elements to documents. As i said above any function implemented in the implementation section of the XBL seems to be callable.  After some progress is seen with all of this ill try to reproduce another issue ive seen recently where two references to the same document involved two different wrappers(not sure about anything bad that could be done with it yet but who knows.)

Comment 6

[moz_bug_r_a4]

(In reply to Boris Zbarsky (:bz) from comment #2)
> There's no sane way to use different protos for the XBL stuff.
> 
> What should happen, though, is that touching "this" in the getter should
> throw if it's anon content... I wonder why it doesn't.

Since eval'ed code inherits its caller's script filename, if content code tricks a chrome XBL code into calling eval, the eval'ed code inherits the chrome XBL code's script filename and thus the eval'ed code can pass the access check for SystemOnlyWrapper.

I'll attach a testcase, which does not use prototype.

Comment 7

[moz_bug_r_a4]

Attachment: testcase - videocontrols

test 1 directly accesses a SOW, and test 2 accesses the SOW with the XBL eval trick.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Hmm.  Can we somehow mark eval scripts so the SOW code will know about them?  I can't think of legitimate uses for eval from chrome code running in content, so ideally we would just skip the filename check altogether for eval code.

Comment 9

[Cody Crews]

A quick question, is an anonymous function created using Function considered eval'ed code?  If not then this problem will still exist even with the above change, but it may be crippled in the fact that you would have to append a new video element each time you wanted different code to execute.  The first way this worked involved Function but was revised to use only eval because the javascript stack showed an anonymous function being created and I assumed it was inheriting the content principal which i was trying to avoid.

Comment 10

[moz_bug_r_a4]

An anonymous function created using Function also inherits Function's caller's script filename.

I'll attach a testcase using Function so that we can test both eval and Function.

Comment 11

[moz_bug_r_a4]

Attachment: testcase 2 - videocontrols - using Function

Comment 12

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #8)
> Hmm.  Can we somehow mark eval scripts so the SOW code will know about them?
> I can't think of legitimate uses for eval from chrome code running in
> content, so ideally we would just skip the filename check altogether for
> eval code.

Yes, this sounds like the best solution to me.

Comment 13

[Bobby Holley (:bholley)]

Also note that the filename hack should go away. CanAccessNativeAnon should just be replaced with isCallerXBL, now that we don't have XBL from untrusted sources.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Bobby Holley (:bholley) from comment #13)
> Also note that the filename hack should go away. CanAccessNativeAnon should
> just be replaced with isCallerXBL, now that we don't have XBL from untrusted
> sources.

Can we do this sooner rather than later? Basing security decisions on filename has been a problem ever since we started doing it and the faster we stop, the better I'll sleep at night...

Comment 15

[Bobby Holley (:bholley)]

(In reply to Blake Kaplan (:mrbkap) from comment #14)
> Can we do this sooner rather than later? Basing security decisions on
> filename has been a problem ever since we started doing it and the faster we
> stop, the better I'll sleep at night...

I just filed bug 818716. It's on my list, but I'm just back from PTO so it'll take me a while of digging through bugmail before I even |git pull|.

Comment 16

[David Bolter [:davidb] (NeedInfo me for attention)]

Bobby we need help sec rating this one.

Comment 17

[Bobby Holley (:bholley)]

We don't have a great sense of the dangers of breaking into NAC, but I think sec-high is a fair rating here.

I've been working on the patches to avoid using the filename hack, which are now up for review in bug 818716 (please don't set the dep, since that would reveal that there's a known security issue involved). I also modified the propagation of the XBL bit over in bug 820666, so I _think_ we should be safe for both of the testcases here. We should verify that though, and check these testcases in (I'll take ownership of that).

moz_bug_r_a4: if we avoid propagating XBL permissions to eval() and Function(), can you think of any other variants of the trick here?

codyc, I'm also curious if you have any other thoughts.

Comment 18

[Andrew McCreight [:mccr8]]

Thanks for the update, Bobby!

Comment 19

[Bobby Holley (:bholley)]

Attachment: Tests. v1 r=me

Here are moz_bug_r_a4's testcases converted into Mochitests.

Comment 20

[Bobby Holley (:bholley)]

So really, I think there are two issues here. Bug 818716 and bug 820666 fix the eval/Function part, but there's still the issue that the XBL can get confused / phished-for-objects by manipulation of standard prototypes and DOM objects. I'm going to send some mail to figure out what we want to do here.

Comment 21

[Cody Crews]

The only concern I have left here is the fact that XBL *may* still run any code set as getters and such when accessing DOM objects and prototypes because that is how XBL code was originally tricked into calling Function/eval.  It would definitely cripple possible attacks related to this by XBL knowing if it was calling Function/eval, but in theory XBL code would still be susceptible to manipulation even if one had to add an element that triggered a binding each time new code was to be run.  Ill do some more testing after grabbing a nightly build to see the changes of the two bug fixes above and how it impacts this.  Ill also look into how this affects functions implemented totally by XBL since I still haven't done any additional work on that side of things.

Comment 22

[Bobby Holley (:bholley)]

(In reply to codyc from comment #21)
> The only concern I have left here is the fact that XBL *may* still run any
> code set as getters and such when accessing DOM objects and prototypes
> because that is how XBL code was originally tricked into calling
> Function/eval.  It would definitely cripple possible attacks related to this
> by XBL knowing if it was calling Function/eval, but in theory XBL code would
> still be susceptible to manipulation even if one had to add an element that
> triggered a binding each time new code was to be run.

Right, the concern here is that even if arbitrary code can't be run, content can still manipulate the XBL in unpredictable ways by messing with its environment. The two solutions here are to either make this OK (make there nothing to gain by attacking XBL), or protect the XBL better. Right now I'm looking into which is preferable.

Comment 23

[moz_bug_r_a4]

(In reply to Bobby Holley (:bholley) from comment #17)
> moz_bug_r_a4: if we avoid propagating XBL permissions to eval() and
> Function(), can you think of any other variants of the trick here?

It's possible to call DOM methods.  I'll attach a testcase.

Comment 24

[moz_bug_r_a4]

Attachment: testcase 3 - videocontrols - call methods

Bug 818716 and bug 820666 do not fix this.

Comment 25

[Bobby Holley (:bholley)]

Hm, yeah. It seems pretty clear at this point that content can waltz around in XBL. We either need to make this ok or move XBL into another compartment. I'm going to investigate that.

Comment 26

[Cody Crews]

I hopped on to attach a new testcase showing that it was still possible to obtain a cloned copy of anonymous XBL elements using cloneNode, but you guys are all over it :)  I'll wait to see what else can be done and put it all together, but this definitely isn't done yet.

Comment 27

[moz_bug_r_a4]

Attachment: testcase 3.1 - videocontrols - call methods

testcase 3 works on fx20, but does not work on fx10,17-19.  This works on fx10,17-20.

Comment 28

[moz_bug_r_a4]

There is a trunk-only problem with new DOM bindings.  Since WrapNewBindingObject does not call JS_WrapValue when scope and obj are in the same compartment, it's possible to bypass SOW.  I'll attach a testcase.

Comment 29

[moz_bug_r_a4]

Attachment: testcase 4 - DOM bindings

On fx20, test 2 does not show "Permission denied to access property" errors.

Comment 30

[Boris Zbarsky [:bzbarsky]]

> Since WrapNewBindingObject does not call JS_WrapValue when scope and obj are in the same
> compartment, it's possible to bypass SOW.

I believe bug 815149 will fix that.

Comment 31

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #30)
> > Since WrapNewBindingObject does not call JS_WrapValue when scope and obj are in the same
> > compartment, it's possible to bypass SOW.
> 
> I believe bug 815149 will fix that.

Wait, I'm confused. Did new bindings for Nodes land without SOWs?

Comment 32

[Boris Zbarsky [:bzbarsky]]

Effectively, yes.  Only an issue on trunk, because I didn't realize we had this SOW thing and Peter didn't realize we were doing a fast path in WrapNewBindingObject....

Comment 33

[Cody Crews]

Any progress on this?  I'm wondering because I have had a testcase for bug 817922 setting here for sometime, but been waiting to see if the changes resulting from this fix it as well.

Comment 34

[Bobby Holley (:bholley)]

(In reply to codyc from comment #33)
> Any progress on this?  I'm wondering because I have had a testcase for bug
> 817922 setting here for sometime, but been waiting to see if the changes
> resulting from this fix it as well.

I'm working on a comprehensive fix to run XBL code in a separate compartment with a special principal, in bug 821850.

FWIW I currently consider this bug and bug 817922 to be more or less the same issue from the perspective of a bug bounty: in the current world, content can muck with XBL to a significant enough degree as to cause it to do arbitrary things. Given that XBL can do arguably sketchy things (accessing anonymous content and potentially communicating with chrome over special channels), we want to make that no longer possible.

If all goes well, the only content->XBL interaction that will be possible will be invoking (and not accessing properties on) functions exposed as methods, getters, setters, and fields via the <implementation> in the XBL binding. If you have a testcase that can exploit such a function, I will consider that a separate bug.

So either way, you should feel free to post your testcase now. If it's an exploit via the API provided by an XBL binding <implementation>, you won't be risking your bug bounty. And even if my fix will quash it, it might still be bad enough to motivate us to move faster on this. ;-)

Comment 36

[chris hofmann]

codyyc,  I've tried to contact you in regards to a bounty payment but it looks like you might not be reachable for a while.  send mail to [e-mail] when you get a chance and I'll send you bounty details.

Comment 37

[Cody Crews]

Ill definitely be in touch by tomorrow morning, probably later tonight.  It's just been one of those days.

Comment 38

[Andrew McCreight [:mccr8]]

Bobby, is this fixed by bug 821850 and thus we can close it?

Comment 39

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #38)
> Bobby, is this fixed by bug 821850 and thus we can close it?

Well, more specifically by 834697. But yes, I think so. We should still embargo it until we're more confident in its effectiveness and until we figure out what to do for esr17.

Comment 40

[Andrew McCreight [:mccr8]]

Thanks!

Comment 41

[Al Billings [:abillings - ex-MoCo]]

Bug 834697 landed for Firefox 21.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Is this wontfix for b2g18? Looks like a pretty ugly backport.

Comment 43

[Bobby Holley (:bholley)]

Yes, we're not going to backport XBL scopes to b2g18.

Comment 45

[Bobby Holley (:bholley)]

The APIs that this testcase use when away in bug 912322.