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

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Cody Crews]

Attachment: 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.

Comment 1

[Cody Crews]

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.

Comment 2

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

This testcase looks legit.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Cody Crews]

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.)

Comment 5

[Cody Crews]

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

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 7

[Cody Crews]

(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.

Comment 8

[Cody Crews]

Attachment: revised testcase to fix my badness with the head tags

Sorry I *may* have been a little drunk last night.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 11

[Cody Crews]

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.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Does that mean that this bug can be marked as INVALID then?

Comment 13

[Cody Crews]

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.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 15

[Cody Crews]

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.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 17

[Cody Crews]

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.)

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 19

[Cody Crews]

(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 :)

Comment 20

[Bobby Holley (:bholley)]

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.

Comment 21

[Cody Crews]

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 :(

Comment 22

[Daniel Veditz [:dveditz]]

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.

Comment 23

[Cody Crews]

Attachment: 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.

Comment 24

[Cody Crews]

Attachment: hmm strange the original doesnt work i think its a problem with setTimeout.

Comment 25

[Cody Crews]

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.

Comment 26

[Bobby Holley (:bholley)]

(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.

Comment 27

[Cody Crews]

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.

Comment 28

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

Just adding a reminder flag for codyc - hoping for new testcase. (Thanks!)

Comment 29

[Cody Crews]

Attachment: 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.

Comment 30

[Bobby Holley (:bholley)]

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.

Comment 31

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

Assigning to Bobby since he'll check this out post Bug 821850. (Thanks)

Comment 32

[Daniel Veditz [:dveditz]]

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.

Comment 33

[Bobby Holley (:bholley)]

(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.

Comment 34

[Cody Crews]

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.

Comment 35

[Bobby Holley (:bholley)]

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).

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

bug 986730, bug 825392, and bug 990290 have all been FIXED. Does that mean that this one is too?

Comment 38

[Bobby Holley (:bholley)]

(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.

Comment 39

[Cody Crews]

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.

Comment 40

[Andrew McCreight [:mccr8]]

(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.