In fact, the only check it does is that the scheme of the document loadBindingDocument is called on matches the scheme of the document it's going to hand back. If they don't match, it hands back null. Consequences: 1) Web sites can use this method to load data from file:// (though they can't read it). This includes things like /dev/stdin, etc... 2) Web sites can use this to load XBL from other sites and get access to the full DOM of the binding document. The XML _does_ need to contain a <bindings> tag in the XBL namespace, so it's not quite as bad as allowing arbitrary XML fetches, but still pretty bad. Ideally we would do the following checks here: A) CheckLoadURI. B) Content policy check. C) Same-origin check against URI to load. D) Same-origin check on redirects. Basically, whatever XMLHttpRequest does... I really wish we didn't hand back the document here. I can't really see a use case for it, and we wouldn't need checks C and D above, probably, if all we did was to load the document somewhere internally. Maybe that's the way to go?

Also... we always load chrome bindings synchronously anyway. So I think all existing consumers of this in the tree are pointless. But I guess non-chrome bindings exist and all.

I actually had to use this for prettyprinting since back in the day that wasn't using chrome:// not sure if that's still the case though. I'll take this for now, if someone wants to step up feel free to.

Same thing applies to AddBinding, it lets you add bindings from any URI, which you can then dig into using document.getAnonymousNodesFor(...)

Do normal -moz-binding loads do these checks??

Turns out that loadBindingDocument was the only thing that didn't have security checks at all. However the security checks on -moz-binding, .addBinding etc didn't check for cross-origin. I bet this patch will break a few users of cross-site bindings, but I don't see that we can do much about that for now.

These rely on a patch that i'm about to attach to bug 383511. But we can't add these to mochitest until this bug is opened anyway.

It might take me a bit to get to this. Swamped with bug 279703.

Comment on attachment 267663 [details] [diff] [review] Patch to fix [landed] - In content/base/src/nsContentUtils.cpp +static PRBool SchemeIs(nsIURI* aURI, const char* aScheme) +{ + nsCOMPtr<nsIURI> baseURI = NS_GetInnermostURI(aURI); + NS_ENSURE_TRUE(baseURI, PR_FALSE); + + PRBool isChrome = PR_FALSE; + return NS_SUCCEEDED(baseURI->SchemeIs(aScheme, &isChrome)) && isChrome; isChrome seems like a bad name here. Maybe isScheme instead? r+sr=jst with that fixed.

I don't think this patch is quite right... It works for cases when the binding being attacked hasn't been loaded as a binding before, but not for cases when a binding extends something it wants to attack that was used as a binding on some other site. Maybe that's not an issue we want to worry about? In any case, I'll try to merge bug 204140 to this patch somehow, I guess. :(

Also, doesn't this pass a null URI to content policy if aLoadingPrincipal happens to be system? I'm not sure that's desirable.

And I think this does the security check twice when coming through LoadBindings()....

Pushing the remaining fixes to beta 1

This made the testcase from bug 71191 fail, becuase it uses cross-domain (more or less) xbl: https://bugzilla.mozilla.org/attachment.cgi?id=156917 I guess that's intended? I always thought the content part from other domains was allowed, but not the javascript part.

Yeah, unfortunately this is intended...

Since this hasn't landed on trunk yet I don't think we can take it in 1.8.1.5

This landed on trunk back in mid-June... But yes, I don't think we want to take that on branch as-is.

Note to self: When fixing this, try to make failed-to-load-bindings not completely disable rendering. See bug 387971

Lowering security flags on this one as there should no longer be any security issues with the code. Still keeping it as a blocker though so I won't loose track of it.

I'd prefer if you marked this as FIXED and filed a new bug for the remaining work. We don't want to lose track of the fact that we fixed an [sg:high] bug on trunk.

Filed bug 408922

> as there should no longer be any security issues with the code I don't think that's true. See comment 9.

Aww crud, sorry, missed that part :(

bumping priority back to p2 so this is on fx3 radar. dan is also updating priority and sg: markings on bug 408922

p1 so its on the fx3 b3 radar to help sort out any compat regressions in beta. It would be better to catch them then, than in an RC or final

Not sure if this work is being tracked here or in bug 408922. Are we on-track to make fx3 b3?

Aiming for b4 for this one.

Didn't make trunk yet, we'll wait for 1.8.1.14.

(In reply to comment #29) > Didn't make trunk yet, we'll wait for 1.8.1.14. > Should we land this on trunk?

It needs a patch first...

I think this'll work, but I haven't tested or even compiled yet.

This should fix things codewise, however I still need to do some testing magic before landing. So it looks like we were in a much better shape than I had thought, which makes me much less worried about this causing breakage or regressions. The original patch already addressed most of the critical concerns in comments 9 to 11. We do already block inheriting cross site as even inherited bindings are loaded through LoadBindingDocumentInfo. Comment 10 was addressed architecturally a long time ago since we no longer pass URIs but rather principals to the contentpolicy code. What this patch does: 1. Make us not load data-urls (unless the origin has chrome privileges) for bindings, this makes it significantly harder to perform XSS attacks using XBL since you have to not only upload a style-rule, but also an XBL binding document that contains evil XBL code. The patch does let you load data-urls if a pref is set, this is to allow for easier testing. Jesse has requested this, and I plan on turning this pref on for the mochitest profile so that we can still use dataurls in mochitests. 2. Make us not call nsContentUtils::CheckSecurityBeforeLoad twice when loading a binding from a new document. This is done by A) checking if the binding document is cached in the documents bindingmanager before calling nsContentUtils::CheckSecurityBeforeLoad. B) Making sure to cache the binding document in the bindingmanager always when a binding document is used. This happened most of the time already, the only instance where it doesn't is when the binding document already exists in the XUL cache. 2B makes us use a little bit more memory. Probably insignificantly more, but I still made us only do it for non-chrome documents, and made nsContentUtils::CheckSecurityBeforeLoad really fast for chrome-documents. What remains to be done: * Write a mochitest to make sure that the data-uri and cross-site uri blocking works as it should. (tested locally and it seems to). * Change the mochitest profile so that we still pass mochitest. * Change any reftests that use data-urls since we don't have a default profile for reftest yet

Same as the previous, but with tests as well as changes to mochitest profiles in order to let mochitests still use XBL with data-urls. The C++ changes are exactly the same as the previous patch. I still need to change reftests and crashtests to not use data-urls since those don't run in custom profiles yet. See bug 374458. However I'll create a separate patch for those as I want to back that out once bug 374458 is fixed.

Since we don't create a custom profile for reftests we can no longer use xbl with data-uris there :( Hopefully this will be fixed once bug 374458 is fixed and then we can revert this patch if we see fit

Comment on attachment 317379 [details] [diff] [review] Fix reftests [checked in] Just stripping out xbl data-uri bindings into their own files -- makes sense to me.

Comment on attachment 317379 [details] [diff] [review] Fix reftests [checked in] These are just changes to reftests and crashtests in preparation for landing the main patch.

Comment on attachment 317379 [details] [diff] [review] Fix reftests [checked in] a=beltzner, though not needed for tests

So in the new setup, if a UA stylesheet attaches a binding to some node in a content document, then after that the content document can attach that binding to arbitrary nodes in the document, since we hit the hashtable before doing the security check. I'm not at all sure we want that to happen. It looks like in the LoadBindings case we still do the security check at least twice, even with this patch. If we do the security check before the hashtable access, maybe the "binding is loaded" code just needs to pass in a boolean to skip the security check in that one case? Or maybe the binding manager hashtable should use a "uri and principal" key like the one at <http://lxr.mozilla.org/seamonkey/source/layout/style/nsCSSLoader.h#222> (just move that key out to nsContentUtils or whatever) so that we just don't get a match if the URI is the same but the loading principals are different? Doing that and putting the hashtable access before the security check would make me happy, I think... I'd prefer using IsSystemPrincipal() in nsContentUtils to direct comparisons against sSystemPrincipal.

(In reply to comment #40) > So in the new setup, if a UA stylesheet attaches a binding to some node in a > content document, then after that the content document can attach that binding > to arbitrary nodes in the document, since we hit the hashtable before doing > the security check. I'm not at all sure we want that to happen. Yeah, I was aware that this would be the case, but I'm not sure how big of a deal that is. I guess the uri+principal idea might be a good one. Will look into it. > It looks like in the LoadBindings case we still do the security check at least > twice, even with this patch. How so? I can't find any other than the one we do in LoadBindingDocumentInfo? > If we do the security check before the hashtable access, maybe the "binding is > loaded" code just needs to pass in a boolean to skip the security check in > that one case? Actually this is already happening, the "binding is loaded" code passes nsnull as aOriginPrincipal so we'll just skip the security check assuming that security has already been checked. > Or maybe the binding manager hashtable should use a "uri and principal" key > like the one at > <http://lxr.mozilla.org/seamonkey/source/layout/style/nsCSSLoader.h#222> (just > move that key out to nsContentUtils or whatever) so that we just don't get a > match if the URI is the same but the loading principals are different? Doing > that and putting the hashtable access before the security check would make me > happy, I think... Will look into that. > I'd prefer using IsSystemPrincipal() in nsContentUtils to direct comparisons > against sSystemPrincipal. Ok, will revert these changes then

> How so? I can't find any other than the one we do in LoadBindingDocumentInfo? Er... indeed. I think I got confused amongst the different blame versions I was looking at. :( > Ok, will revert these changes then To make it clear, I meant using nsContentUtils::IsSystemPrincipal(). I'm fine with adding that fast-path.

So I opted not to use the uri+originprincipal key. Turned out I would have had to muck around with the code quite a bit to get the principal correct everywhere. And the only real advantage is that we would do fewer security checks when content is using XBL, something that is very rare. If we really want to optimize that we can do that later. So this patch just gets rid of the sSystemPrincipal stuff, and moves the security checks to above the bindingmanager hashlookup. The bugzilla interdiff actually works this time! Please ignore the nsElementMap changes, they are not supposed to be there.

Comment on attachment 317656 [details] [diff] [review] Fix comments (with -w) >Index: content/base/public/nsContentUtils.h For what it's worth, I was OK with keeping sSystemPrincipal, as I said in comment 42. I just think that we should only have a "== sSystemPrincipal" in nsContentUtils::IsSystemPrincipal and use that call in the various nsContentUtils comparisons. >Index: content/xbl/src/nsXBLService.cpp >@@ -940,102 +944,99 @@ nsXBLService::LoadBindingDocumentInfo(ns >+ *aResult = nsnull; >+ nsCOMPtr<nsIXBLDocumentInfo> info; [etc] I don't see a reason to move this code up here.. >- // We've got a file. Check our XBL document cache. >+ // The second line of defense is the chrome cache. I think the line that was there is more correct now that the ordering change has been undone. >+ if (!info && useXULCache) { No need to check !info, since it's always true here. In general, maybe we just need to mostly revert the changes to this method? >+ static PRBool gAllowDataURIs; // Should we allow data urls in "Whether we should" So the only real changes here, as far as I can tell are the data: pref and the fast-path in CheckSecurityBeforeLoad, no? r+sr=bzbarsky on those, but the rest of the changes don't seem to be needed here and I'd rather we didn't make them just because. You're right that various other patches largely addressed the issues in comment 9 through comment 11.

So http://developer.mozilla.org/en/docs/Firefox_3_for_developers " Embedding XBL bindings You can now use the data: URL scheme to embed XBL bindings directly instead of having them in separate XML files. " has become untrue, unless a pref is switched?

For non-chrome documents, yes. We probably need to adjust the documentation accordingly...

(In reply to comment #47) Based on a phone conversation with sicking today and bz's comments this is the patch I'm going to land for sicking today. > For what it's worth, I was OK with keeping sSystemPrincipal Sicking said he wants to make those changes at a later time.

For what it's worth, the "Final patch" looks good to me. Ben, thanks for picking this up!

Comment on attachment 318209 [details] [diff] [review] Final patch [checked in] a1.9+=damons

Comment on attachment 318209 [details] [diff] [review] Final patch [checked in] Landed on trunk.

Fixed.

We need to change http://developer.mozilla.org/en/docs/Firefox_3_for_developers to say that data: is only available for chrome code, e.i. extension developers. We should also say that the normal same-origin policy is used for websites meaning that they can only link to XBL files on the same domain. XBL can also be loaded from chrome-urls. Ideal would be if we could link to the documentation for the new policy of linking to chrome-urls, which is the policy that is used here too.

Docs have been updated.

It looks like we don't document the same-origin policy anywhere, unless I missed it... That needs to be fixed, per comment 55.

I've definitely seen links to the following in various places, even from the security stuff in the Doctype thing Google's doing: http://www.mozilla.org/projects/security/components/same-origin.html Moving that to MDC and making it more prominent might not be a bad idea.

Sure, but that document doesn't actually describe the check that happens here, so much. In particular, it doesn't describe how one determines the origin the XBL request is coming from (which is somewhat important), nor does it describe the peculiarities with chrome:// XBL links, etc.

For example, the XBL request origin is never affected by document.domain setting.

OK, where can I get specific details about how the same-origin policy works, so I can make sure we have a thorough document on it? Alternately, if someone else that actually knows how it works would like to throw something together, I can copy-edit it once you're done.

Let me see if I can summarize. sicking, please correct me if I get something wrong. Whether XBL is allowed to be loaded depends on the nsIPrincipal originating the load and the nsIURI to be loaded. The principal originating the load is determined as follows: 1) External stylesheets (<link>, <?xml-stylesheet?>, user sheets, UA sheets): Depends on where the sheet is loaded from, just like it would for an HTML document loaded from such a source. 2) Inline stylesheets (<style>, style attributes): The principal of the Element node involved (the <style> element or the element whose style attribute we're looking at). 3) nsIDOMDocumentXBL.addBinding/loadBindingDocument: The principal of the script making the call, or the principal of the document the call is made on if there is no such script. The following checks are performed: a) If the principal originating the load is the system principal, the load is allowed. b) A CheckLoadURIWithPrincipal check to make sure the given principal can link to the given URI to start with. c) A content policy check. d) If the XBL-from-data: pref is set and the URI is a data: URI the load is allowed. e) If the URI is a chrome: URI the load is allowed. Note that step (b) already bailed out for cases when it's a chrome: URI that is not accessible to untrusted content. f) Finally, a CheckMayLoad check is performed on the originating principal and the given URI. If the originating principal is a NullPrincipal, this returns false. Otherwise, if the principal's codebase URI and the target URI are same-origin per the link in comment 58, it returns true. Otherwise, it has some fairly complicated special-casing for our fairly complicated same-origin policy for file:// URIs... Basically, the load is allowed if both the origin and target are file:// URIs, the target is not a directory, and is either a in a subdirectory of the origin (when the origin is a directory, which is pretty much never for XBL) or is in a subdirectory of the origin's parent (when the origin is a file). It might be worth documenting CheckMayLoad separately (since it's used for other things, like XMLHttpRequest), and referring to that documentation from the XBL doc.

Does this policy work the same way as for JavaScript content, or does XBL have a different policy?

Which "this policy"? The overall process is very different from, say, a check whether some JS is allowed to access some Node, starting with the fact that the "can JS access this node" check is something that takes two principals, not a principal and a URI.

I've seen the phrase "same origin policy" applied both to XBL and JavaScript. I'd like to know if the policy is the same for both.

Ah. No, it is not. The definition of "same origin" for two URIs is the same, but XBL and JavaScript use different methods of determining the relevant URIs to test, and both XBL and JavaScript have some exceptions that make the policies not quite pure "same origin" policies.

OK... I should probably document both of these. Where can I get info on the JavaScript policy?

I'm not sure it's written up anywhere, other than perhaps the HTML5 specification in broad strokes. The issues with the JS policy are when the check is done (the simple answer is "on any property access", maybe) and what it checks (that the two principals are "equal" in the sense that the origin URI (codebase unless .domain was set, adjusted for the domain set otherwise) is same-origin for the two, and either both have set .domain or neither has set .domain. But there are all kinds of exceptions to the above depending on exact situation...

I've documented the XBL policy here: http://developer.mozilla.org/en/docs/Same_origin_policy_for_XBL If this seems okay, please mark this as dev-doc-complete; otherwise, let me know what needs fixing (or fix it yourself if you prefer).

That looks great. Thanks!

I would prefer not to mention the XBL-from-data pref. We don't want people setting that since that can expose them to XSS attacks. I'll go ahead and make that change to the docs.

With Jonas out it doesn't look like a branch fix is coming any time soon.

And since this is a compat change I think we should get some FF3 baking before putting this on branch.

Jonas didn't get to this in time for 1.8.1.17.

Jonas, any update here? Code freeze is in about a week...

This should work on the 1.8 branch. I opted to keep the checks in LoadBindings rather than (the more efficient) way we do it on trunk where they live in LoadBindingDocumentInfo. This should be safe as I also make .loadBindingDocument always return null, so even if you can get arbitrary binding documents into the bindingmanager, you can't attach them anywhere.

Use TYPE_XBL when doing content-policy checks, and call CheckLoadURI even though you can't get hold of the data.

Comment on attachment 348890 [details] [diff] [review] Branch patch 2 -w r+sr=bzbarsky

Comment on attachment 348890 [details] [diff] [review] Branch patch 2 -w Approved for 1.8.1.19. a=ss

Checked in to 1.8 branch! Yay!

Jonas, is there a simple way to test this in the 1.8 branch?

The best way to test on 1.8 would be to try ensure that you can't load XBL cross site by using loadBindingDocument. I don't think we have any non-mochitest testcases.

Comment on attachment 348890 [details] [diff] [review] Branch patch 2 -w a=asac for 1.8.0 branch

This bug caused a regression on the 1.9 trunk (bug 387971) which was then fixed there by a fix to another bug (bug 204140). Sadly, however, since that regression was not marked here, we didn't realize that when taking this on 1.8.1 for Firefox 2.0.0.19, resulting in a regression there as well (bug 470049). Whew! Anyway, the point is: - the fix for this bug causes a regression that results in wellsfargo not working properly (as it restricts the ability to see the styling for the password field, and thus the password field). That's bug 387971 for 1.9 and bug 470049 for the 1.8.1 branch. - that regression was fixed on 1.9 in bug 204140 - we now need to fix that regression on the 1.8.1 branch

(In reply to comment #84) > - we now need to fix that regression on the 1.8.1 branch Fixing that regression involves fixing numerous (bug 204140 has a bunch of dependencies). It might take a day to get all that code ported and reviewed, to say nothing of the risk involved with taking that much code at the very last minute. I'm not sure what we should be doing here.

Comment on attachment 348890 [details] [diff] [review] Branch patch 2 -w asac: I don't think you want this patch for 1.8.0 given it breaks loging into wellsfargo.com. You'll need a new patch with regressions fixed.

The alternative is rolling out a simpler, but different, fix for 1.8. If we simply ignored any XBL that failed to load then I think we'd be golden. What happens currently is that if we fail to load an XBL we refuse to render anything, even the normal content.

(In reply to comment #87) > The alternative is rolling out a simpler, but different, fix for 1.8. If we > simply ignored any XBL that failed to load then I think we'd be golden. What > happens currently is that if we fail to load an XBL we refuse to render > anything, even the normal content. Meet me in bug 470049

Comment on attachment 348890 [details] [diff] [review] Branch patch 2 -w a=asac for 1.8.0 thanks Dan, distros already have this in their latest 1.8.0 build.

> Sadly, however, since that regression was not marked here It actually, was, in the dep list, on 2007-07-12... Sorry I missed that when reviewing the branch patch.

And I when writing it :(

Sooo... apologies for ressuscitating an old bug, but I'm poking about because of bug 1049199, where the first step would be having a list of the XBL-included stylesheets. I found the relevant DOMDocumentXBL interface with the void loadBindingDocument call, and a pointer to MDN. Then I read: https://developer.mozilla.org/en-US/docs/XBL/XBL_1.0_Reference/DOM_Interfaces#loadBindingDocument and I was very confused, because the method is clearly void and never returns anything. Are the docs just outdated? What is the use of calling it when it's void() ? (ie: can we just rip it out at this point?)

The docs are outdated, yes. The use of calling it is that it precaches the binding document in question, so that bindings from it can be applied synchronously. Not only that, but it will not return until the load is complete. In other words, you can call this method, then start using bindings from that binding document and not end up having to wait for them to load async. This is, of course, a pretty crappy thing to do in some ways... ;) Now the thing is, we don't use this method in our codebase, and addons mxr only shows me uses for chrome:// and resource:// URIs, which get XBL sync-loaded anyway. So maybe we can in fact remove it...