Closed Bug 816071 Opened 12 years ago Closed 11 years ago

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

Categories

(Core :: Security, defect)

17 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox-esr17 --- wontfix
b2g18 --- fixed
b2g-v1.1hd --- wontfix
b2g-v1.2 --- unaffected

People

(Reporter: codycrews00, Assigned: bholley)

References

Details

(Keywords: sec-high, testcase, Whiteboard: [embargo until b2g18 EOL][part 2 fixed by bug 818716 and 820666])

Attachments

(6 files, 1 obsolete file)

Attached file 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.
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.
Component: Untriaged → Security
Product: Firefox → Core
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.
Keywords: testcase
(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.
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....
(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.)
Attachment #686079 - Attachment mime type: text/plain → text/html
(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.
test 1 directly accesses a SOW, and test 2 accesses the SOW with the XBL eval trick.
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.
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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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.
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.
(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...
(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|.
Bobby we need help sec rating this one.
Flags: needinfo?(bobbyholley+bmo)
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.
Assignee: nobody → bobbyholley+bmo
Flags: needinfo?(moz_bug_r_a4)
Flags: needinfo?(bobbyholley+bmo)
Flags: in-testsuite?
Keywords: sec-high
Summary: Content provided by XBL uses prototypes available in the context of the current window allowing theft of otherwise completely anonymous content. → Content can impersonate XBL script using eval() and Function(), bypassing SOWs
Thanks for the update, Bobby!
Whiteboard: [to be fixed by bug 818716 and 820666]
Attached patch Tests. v1 r=meSplinter Review
Here are moz_bug_r_a4's testcases converted into Mochitests.
Attachment #691573 - Flags: review+
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.
Summary: Content can impersonate XBL script using eval() and Function(), bypassing SOWs → Content can confuse XBL with prototypes and can impersonate XBL script using eval() and Function() (bypassing SOWs)
Whiteboard: [to be fixed by bug 818716 and 820666] → [part 2 fixed by bug 818716 and 820666]
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.
(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.
(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.
Flags: needinfo?(moz_bug_r_a4)
Bug 818716 and bug 820666 do not fix this.
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.
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.
testcase 3 works on fx20, but does not work on fx10,17-19.  This works on fx10,17-20.
Attachment #692192 - Attachment is obsolete: true
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.
On fx20, test 2 does not show "Permission denied to access property" errors.
> 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.
(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?
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....
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.
(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. ;-)
Flags: sec-bounty? → sec-bounty+
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 chofmann@mozilla.org when you get a chance and I'll send you bounty details.
Ill definitely be in touch by tomorrow morning, probably later tonight.  It's just been one of those days.
Bobby, is this fixed by bug 821850 and thus we can close it?
Flags: needinfo?(bobbyholley+bmo)
(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.
Flags: needinfo?(bobbyholley+bmo)
Thanks!
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [part 2 fixed by bug 818716 and 820666] → [embargo until ESR17 EOL][part 2 fixed by bug 818716 and 820666]
Depends on: 834697, XBL-scopes
Bug 834697 landed for Firefox 21.
Is this wontfix for b2g18? Looks like a pretty ugly backport.
Flags: needinfo?(bobbyholley+bmo)
Yes, we're not going to backport XBL scopes to b2g18.
Flags: needinfo?(bobbyholley+bmo)
Whiteboard: [embargo until ESR17 EOL][part 2 fixed by bug 818716 and 820666] → [embargo until b2g18 EOL][part 2 fixed by bug 818716 and 820666]
The APIs that this testcase use when away in bug 912322.
Flags: in-testsuite? → in-testsuite-
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: