Functions implemented by XBL are callable, this could be a serious security threat under the right circumstances.

RESOLVED FIXED

Status

()

Core
XBL
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: Cody Crews, Assigned: bholley)

Tracking

({sec-other})

17 Branch
x86_64
Windows 7
sec-other
Points:
---
Bug Flags:
sec-bounty -

Firefox Tracking Flags

(firefox-esr31 wontfix)

Details

(Whiteboard: waiting for a testcases with clearer evidence of abusability)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 688066 [details]
callxblfunction.html

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

Steps to reproduce:

Created an iframe with XML data and the type xhtml+xml, then called the observe function implemented by XMLPrettyPrint.xml.

please note with the attached testcase that sometimes the data URL used for the xml is cached(i believe) and it doesnt load the bindings from XMLPrettyPrint.xml


Actual results:

The observe function is called even though it is loaded from a chrome URL.


Expected results:

A security error should be thrown.
(Reporter)

Comment 1

5 years ago
Thought i should make a note here in the testcase I call the function with iframe.contentDocument.documentElement passed in as this, but the function is callable even without passing this in, I only did it this way so it could be visually seen when the new iframe is appened to the xml data.
Flags: sec-bounty?
Attachment #688066 - Attachment mime type: text/plain → text/html
This testcase looks legit.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: Untriaged → XBL
Product: Firefox → Core

Comment 3

5 years ago
It's loaded from a chrome URL, but compiled with the principals of the page.  This is why your script can call it, but also why it doesn't get much in the way of privileges your script doesn't already have.  Access to SOWs is the one obvious exception.
(Reporter)

Comment 4

5 years ago
Yes i understand that it has the principals of the page, but i was referring to the fact that im calling the function through the windows property of the binding and not through the documentElement that it is a part of.  Without passing in iframe.contentDocument.documentElement as the 'this' when calling the function it is still called.  My main concern here is that this could lead to access of other anonymous content as shown in the previous bug i filed(i mentioned this there.)  To look at this from another angle in the testcase from the other bug where the video utils are accessible, it should be just as possible to call functions implemented by the video utils through the bindings that show up as window properties as it is here, and that would involve no eval or Function trick.  I hope you understand what I mean(if not ill put together a better testcase).  On a side note i will still be around actually, things changed today( i was thinking i might not be around for a few months.)
(Reporter)

Comment 5

5 years ago
Also thought i should ask that is the principal that the bindings are originally applied with the principal that it runs as?  If you look at how XMLPrettyPrint.xml bindings are applied I remember seeing in the source over a week ago that it is actually applied with the system principal instead of the normal process of adopting the principal of the content it is being applied to(seems to be an odd case.)  This was with the 16.0 source at the time but I doubt it has changed.

Comment 6

5 years ago
> Also thought i should ask that is the principal that the bindings are originally applied
> with the principal that it runs as?

Not sure what you mean...

Jonas, does prettyprinting do something special here?
(Reporter)

Comment 7

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #6)
> > Also thought i should ask that is the principal that the bindings are originally applied
> > with the principal that it runs as?
> 
> Not sure what you mean...
> 
> Jonas, does prettyprinting do something special here?

in the 17.0 source nsXMLPrettyPrinter.cpp lines 128 - 132.  Here you will see pretty print bindings being applied with the system principal.  Other cases ive seen of bindings being applied seemed to reference the principal of the URI they were being applied to.
(Reporter)

Comment 8

5 years ago
Created attachment 688456 [details]
revised testcase to fix my badness with the head tags

Sorry I *may* have been a little drunk last night.
Attachment #688066 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #688456 - Attachment description: revised testcase to fixing my badness with the head tags → revised testcase to fix my badness with the head tags
Attachment #688456 - Attachment mime type: text/plain → text/html
The only special thing that the prettyprint code *needs* to do is to be called from gecko when we have the generated anonymous content ready. As I recall it this is done by having the XBL binding implement nsIObserver and then we call that through XPCOM.

Comment 10

5 years ago
> in the 17.0 source nsXMLPrettyPrinter.cpp lines 128 - 132.

That's passing in system for the origin principal, which is used for the security check for loading the binding at all.

The principal used for the function itself is set up when the function is cloned in nsXBLProtoImplMethod::InstallMember (for methods) which uses the principal of the element the binding is attached to.
(Reporter)

Comment 11

5 years ago
Ah thats my bad thanks for clearing that up.  Thats what i meant by the above that you didnt understand too.  Will take me quite some time to have an understanding of what happens and where in the source like you guys have.
Does that mean that this bug can be marked as INVALID then?
(Reporter)

Comment 13

5 years ago
The bug is completely valid i believe.  I just did some quick test and it is definitely possible to call functions provided by completely anonymous content in the same way as it was possible to call observe from XMLPrettyPrint.xml.  If another testcase is needed showing what I mean, ill prepare one by tomorrow morning.
Being able to call XBL functions have *always* been the case. It was designed such that that is safe.

The only thing a function implemented in XBL can do that a normal webpage can't do is to use SOWs as pointed out in comment 3. There might be other exceptions too, but none that are particularly critical.

I believe we already have bugs on making XBL functions uncallable, so if that's all this bug is, then it might be a dupe rather than invalid.

Bobby, can you confirm?
(Reporter)

Comment 15

5 years ago
In the above comment the functions i was talking about calling aren't provided by elements that can be retrieved using getAnonymousNodes or similar(at least not from content).  The functions are only found in the binding names that become window properties and there *should* be no way to interact with them since the content should never know they exist, but if this is a dup then mark it as so and ill just bring back a better testcase later leading to theft of anonymous content again.
Indeed, content *should* not know about them, and there *should* not be any way to interact with them. That doesn't mean that it's a security problem if content can.

XBL javascript functions were always designed with the mindset that they could be reached from content and that content calling them should be no different than if content had contained the same script contents.

I.e. we explicitly haven't relied on these functions not being reachable in order to keep things safe.
(Reporter)

Comment 17

5 years ago
Ah yeah I'm sorry to come across like that its just with security you could say I'm overly paranoid.  I understand what you mean its just that i can see nasty things being done with this, imagine functions in video Utils being overwritten so that clicking to play an embeded video also provides a click on a paid add in the same page or some such.  Ill do some testing before i bring this back up but mark it as invalid or a dup for now if thats the case, what im worried about is if these functions are rewritable, or even if a getter can be defined in the place of the function(once again allowing someone to run code as a SOW.)
Please do investigate. It's definitely not unlikely that we've made mistakes somewhere here and that once you are running as XBL code you can do evil things.

But yes, having a more concrete attack scenario would be needed for any real action here.
(Reporter)

Comment 19

5 years ago
(In reply to Jonas Sicking (:sicking) from comment #18)
> Please do investigate. It's definitely not unlikely that we've made mistakes
> somewhere here and that once you are running as XBL code you can do evil
> things.
> 
> But yes, having a more concrete attack scenario would be needed for any real
> action here.

I understand, as I had stated in this and the crash bug from yesterday I was under the assumption that I wouldn't be around for at least a few months starting today(with practically no access to a PC etc.)  So i admit the leg work wasnt done in either case but I thought it would be better to put these out there in case something bad could be done with them before I wouldn't have the chance to do further research.  Ill definitely be putting something together soon as I should have a little time to play this week :)
Whiteboard: waiting for a testcases with clearer evidence of abusability
Sometime soon, I plan on making the Components object visible only to XBL-scoped code. It's intended as a defense-in-depth measure, and it'll still have the relevant protections. But the goal here is to prevent content from QIing DOM objects to random interfaces, so it's not ideal if content can easily trick XBL into revealing the Components object.
Flags: needinfo?(codycrews00)
(Reporter)

Comment 21

5 years ago
Planning to do additional testing on this in relation to follow up stuff ill be doing for bug 816071.  Sorry to be slow on this :(
Flags: needinfo?(codycrews00)
Not a problem, but please leave the needinfo? flag on the bug until you've followed up so our queries recognize this as a pending, rather than active, bug.
Flags: needinfo?(codycrews00)
(Reporter)

Comment 23

5 years ago
Created attachment 696714 [details]
testcase showing why functions of XBL shouldnt be accessible through binding names.

Nothing major here, accessing native anonymous content again just like bug 816071 the only difference is the approach.  Meant to have some notes ready with this so hope I don't forget anything.  The original point with this bug was that XBL functions should not be accessible when mapped through binding names that are available as window properties.  When putting this together I realized that for some reason when XBL calls into a function that you can still do a trace through arguments.callee.caller, and that has been used to gain access to the video controls.  I'm not sure on the current spec/standard but I thought this was deprecated, and in any normal circumstance arguments.callee.caller would be null.  I also discovered that you can access anything defined as a field in XBL as a getter through the binding names, and used this to show off cloning the video utils on a random element.  I was gonna go as far as cloning each individual piece of the utils then rewriting getAnonymousElementByAttribute so that Utils.init could be called and come up with a completely independent set of video utils but I know this bug will most likely be taken care of by bug 816071 so yeah pointless.  Last thing to note is that I know Components is being deprecated soon, and I just think that needs to happen as quickly as possible because Components.lookupMethod can be used to call practically any function on anything that is protected by a SOW.
Flags: needinfo?(codycrews00)
(Reporter)

Updated

5 years ago
Attachment #696714 - Attachment mime type: text/plain → text/html
(Reporter)

Comment 24

5 years ago
Created attachment 696715 [details]
hmm strange the original doesnt work i think its a problem with setTimeout.
Attachment #696714 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #696715 - Attachment mime type: text/plain → text/html
(Reporter)

Comment 25

5 years ago
Funny this works on a local server, I'm sure its the timing causing this not to work, for some reason the cloned video utils property loves to vanish and thats causing this to error out.  Ill tweak and and try to access it remotely somewhere before i waste another upload.
(In reply to codyc from comment #23)
> Last thing to note is that I know Components is being deprecated
> soon, and I just think that needs to happen as quickly as possible because
> Components.lookupMethod can be used to call practically any function on
> anything that is protected by a SOW.

Ah! Clever. :-) That is very much a separate bug. Please file with a testcase and CC me.
(Reporter)

Comment 27

5 years ago
Ok I figured out my confusion here.  Apparently if you have called the cloneUtils function in these testcases even once in firebug it continues to work in additional loads even when bypassing cache using f5 during reload, this is what was causing me to see it working locally on 17.0.1 but it works on my admittedly dated nightly build all the time, not that its that important.  Had a major headache there.  The Components.lookupMethod issue doesnt work on 17.0.1 either but I have confirmed that it works in ff 19 and 20, not sure about 18.  I was thinking that it might be attributed to the lack of SOW's being discussed in bug 816071 but that wouldn't explain it working in ff 19.  Going to throw together a quick testcase and file a report before new years fun.
Just adding a reminder flag for codyc - hoping for new testcase. (Thanks!)
Flags: needinfo?(codycrews00)
(Reporter)

Comment 29

5 years ago
Created attachment 697098 [details]
testcase without cloning Utils

I removed the part about cloning the video control utils as that was just an additional thing that could be shown as a possible danger from this.  If you want to see that then check out one of the other testcases with firebug and execute the cloneUtils function.  It will return a generic element with an independent clone of the video control Utils, its not a reference to the existing Utils.  That concept no longer works on nightly and as of right now can only be seen from firebug(any ideas why?)  Something to note is that if you do that it can be very confusing afterwards because until you explicitly disable firebug for this site from its UI the testcase will function like its working without firebug, which led to me wasting a few hours insanely confused.  This is all closely tied into another bug and should be taken care of by the fix for it but I was just interested in pursuing the dangers that could result from these functions being accessible.  Let me know if you need anything else on it.
Flags: needinfo?(codycrews00)
(Reporter)

Updated

5 years ago
Attachment #697098 - Attachment mime type: text/plain → text/html
Thanks for the testcase codyc. I'm pretty sure this will be fixed by bug 821850 as mentioned in bug 816071 comment 34. I'll give it a test once I get those patches done.
Assigning to Bobby since he'll check this out post Bug 821850. (Thanks)
Assignee: nobody → bobbyholley+bmo
Depends on: 821850
Keywords: sec-other
No longer depends on: 821850
Depends on: 834697
If this depends on bug 834697 is it then "fixed"?

This bug does not meet the bar for a bug bounty. The components.lookupMethod issue was filed as a separate bug.
Flags: sec-bounty? → sec-bounty-
(In reply to Daniel Veditz [:dveditz] from comment #32)
> If this depends on bug 834697 is it then "fixed"?

Sort of, modulo the various holes in the XBL scope machinery that I've been fixing and patching. I'd like to keep these bugs hidden for the time being.

> This bug does not meet the bar for a bug bounty. The components.lookupMethod
> issue was filed as a separate bug.

I view this as more or less the same issue as bug 816071. So if we're paying a bounty for that then I believe that's correct.
(Reporter)

Comment 34

3 years ago
Would it bother anyone to now mark this bug as resolved or invalid or whatever that will keep it from being public?  This shouldn't be an issue at all from what I can tell as I am constantly gauging the possible impact of accessing anonymous content, and with everything that has finally played out even when I can gain a reference to anonymous content, I haven't found a single way to bypass the security restrictions in place now.
Yeah, I think we're in good shape here. Once bug 986730, bug 825392, and bug 990290 (all on FF31) make it release/ESR, I think we can open up all of these XBL bugs (assuming cody agrees, which it sounds like he does).
bug 986730, bug 825392, and bug 990290 have all been FIXED. Does that mean that this one is too?
(In reply to Jonas Sicking (:sicking) from comment #37)
> bug 986730, bug 825392, and bug 990290 have all been FIXED. Does that mean
> that this one is too?

Yes.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Reporter)

Comment 39

3 years ago
It's going to be nice to see all this XBL related stuff start to go public.  I know it's off topic, but what is the future of XBL looking like?  I remember that there was proposals for an XBLv2 type setup and then something else took prominence(with more browsers on board) but I haven't kept up.  Drop me an email if you have any idea and time to spare, and that goes for anyone reading.
(In reply to codyc from comment #39)
> I know it's off topic, but what is the future of XBL looking like?  I
> remember that there was proposals for an XBLv2 type setup and then something
> else took prominence(with more browsers on board) but I haven't kept up. 
> Drop me an email if you have any idea and time to spare, and that goes for
> anyone reading.

This is known as "web components".  See bug 811542 and dependencies.  One difference is that I believe they are not trying to 100% abstract out what is inside, partly so they don't have to worry about security problems in case somebody is able to figure out a way to get through it.  I don't know much about it beyond that.
status-firefox-esr31: --- → wontfix

Updated

2 years ago
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.