Last Comment Bug 379959 - (CVE-2008-5503) loadBindingDocument doesn't do any security checks
(CVE-2008-5503)
: loadBindingDocument doesn't do any security checks
Status: RESOLVED FIXED
[sg:high] high assuming you can read ...
: dev-doc-complete, fixed1.8.1.19
Product: Core
Classification: Components
Component: XBL (show other bugs)
: Trunk
: x86 Linux
: P2 critical (vote)
: ---
Assigned To: Jonas Sicking (:sicking) No longer reading bugmail consistently
: Hixie (not reading bugmail)
Mentors:
: 408922 417397 (view as bug list)
Depends on: 204140 324253 387971 388597 470049
Blocks:
  Show dependency treegraph
 
Reported: 2007-05-07 09:30 PDT by Boris Zbarsky [:bz]
Modified: 2015-02-23 17:58 PST (History)
22 users (show)
pavlov: blocking1.9+
dveditz: blocking1.8.1.19+
dveditz: wanted1.8.1.x+
caillon: blocking1.8.0.next+
dveditz: wanted1.8.0.x+
bent.mozilla: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to fix [landed] (20.93 KB, patch)
2007-06-07 18:29 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
jst: review+
jst: superreview+
Details | Diff | Splinter Review
mochitests (4.81 KB, patch)
2007-06-07 18:32 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
no flags Details | Diff | Splinter Review
WIP (15.83 KB, patch)
2008-04-18 17:15 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
no flags Details | Diff | Splinter Review
Patch to fix (16.97 KB, patch)
2008-04-21 16:49 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
no flags Details | Diff | Splinter Review
Same as above without -w (19.15 KB, patch)
2008-04-21 16:50 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
no flags Details | Diff | Splinter Review
Patch with tests (-w) (26.90 KB, patch)
2008-04-22 15:28 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
no flags Details | Diff | Splinter Review
Fix reftests [checked in] (13.65 KB, patch)
2008-04-23 14:36 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
dholbert: review+
mbeltzner: approval1.9+
Details | Diff | Splinter Review
Fix comments (with -w) (27.25 KB, patch)
2008-04-24 19:56 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
no flags Details | Diff | Splinter Review
Fix comments (with -w) (27.25 KB, patch)
2008-04-24 19:58 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
Fix comments (without -w) (29.53 KB, patch)
2008-04-24 19:58 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
no flags Details | Diff | Splinter Review
Final patch [checked in] (16.37 KB, patch)
2008-04-28 12:03 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
dsicore: approval1.9+
Details | Diff | Splinter Review
Branch patch -w (5.80 KB, patch)
2008-11-18 18:24 PST, Jonas Sicking (:sicking) No longer reading bugmail consistently
no flags Details | Diff | Splinter Review
Branch patch 2 -w (7.07 KB, patch)
2008-11-18 18:41 PST, Jonas Sicking (:sicking) No longer reading bugmail consistently
bzbarsky: review+
bzbarsky: superreview+
samuel.sidler+old: approval1.8.1.19+
asac: approval1.8.0.next+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2007-05-07 09:30:47 PDT
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?
Comment 1 Boris Zbarsky [:bz] 2007-05-07 09:33:21 PDT
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.
Comment 2 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-05-15 17:20:17 PDT
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.
Comment 3 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-05-30 18:46:30 PDT
Same thing applies to AddBinding, it lets you add bindings from any URI, which you can then dig into using document.getAnonymousNodesFor(...)
Comment 4 Boris Zbarsky [:bz] 2007-05-30 20:48:04 PDT
Do normal -moz-binding loads do these checks??
Comment 5 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-06-07 18:29:46 PDT
Created attachment 267663 [details] [diff] [review]
Patch to fix [landed]

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.
Comment 6 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-06-07 18:32:54 PDT
Created attachment 267666 [details] [diff] [review]
mochitests

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.
Comment 7 Boris Zbarsky [:bz] 2007-06-07 20:21:05 PDT
It might take me a bit to get to this.  Swamped with bug 279703.
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2007-06-12 14:10:12 PDT
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.
Comment 9 Boris Zbarsky [:bz] 2007-06-15 23:41:59 PDT
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.  :(
Comment 10 Boris Zbarsky [:bz] 2007-06-16 00:01:03 PDT
Also, doesn't this pass a null URI to content policy if aLoadingPrincipal happens to be system?  I'm not sure that's desirable.
Comment 11 Boris Zbarsky [:bz] 2007-06-16 00:35:05 PDT
And I think this does the security check twice when coming through LoadBindings()....
Comment 12 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-06-20 10:58:46 PDT
Pushing the remaining fixes to beta 1
Comment 13 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-06-22 02:49:25 PDT
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.
Comment 14 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-06-22 09:54:34 PDT
Yeah, unfortunately this is intended...
Comment 15 Daniel Veditz [:dveditz] 2007-07-09 16:05:11 PDT
Since this hasn't landed on trunk yet I don't think we can take it in 1.8.1.5
Comment 16 Boris Zbarsky [:bz] 2007-07-09 18:40:58 PDT
This landed on trunk back in mid-June...  But yes, I don't think we want to take that on branch as-is.
Comment 17 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-11-08 17:11:40 PST
Note to self: When fixing this, try to make failed-to-load-bindings not completely disable rendering. See bug 387971
Comment 18 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-12-18 15:50:33 PST
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.
Comment 19 Jesse Ruderman 2007-12-18 16:05:35 PST
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.
Comment 20 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-12-18 16:41:48 PST
Filed bug 408922
Comment 21 Boris Zbarsky [:bz] 2007-12-18 17:01:02 PST
> as there should no longer be any security issues with the code

I don't think that's true.  See comment 9.
Comment 22 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-12-18 17:20:36 PST
Aww crud, sorry, missed that part :(
Comment 23 chris hofmann 2008-01-10 15:06:33 PST
bumping priority back to p2 so this is on fx3 radar.  dan is also updating priority and sg: markings on bug 408922
Comment 24 chris hofmann 2008-01-15 14:48:44 PST
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
Comment 25 Brandon Sterne (:bsterne) 2008-02-08 15:43:28 PST
Not sure if this work is being tracked here or in bug 408922.  Are we on-track to make fx3 b3?
Comment 26 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-02-08 17:10:03 PST
Aiming for b4 for this one.
Comment 27 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-02-12 14:56:51 PST
*** Bug 408922 has been marked as a duplicate of this bug. ***
Comment 28 Jeff Walden [:Waldo] (remove +bmo to email) 2008-02-13 20:18:35 PST
*** Bug 417397 has been marked as a duplicate of this bug. ***
Comment 29 Daniel Veditz [:dveditz] 2008-03-05 11:40:59 PST
Didn't make trunk yet, we'll wait for 1.8.1.14.
Comment 30 Mike Schroepfer 2008-03-31 16:27:13 PDT
(In reply to comment #29)
> Didn't make trunk yet, we'll wait for 1.8.1.14.
> 

Should we land this on trunk?
Comment 31 Boris Zbarsky [:bz] 2008-03-31 20:37:49 PDT
It needs a patch first...
Comment 32 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-04-18 17:15:48 PDT
Created attachment 316513 [details] [diff] [review]
WIP

I think this'll work, but I haven't tested or even compiled yet.
Comment 33 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-04-21 16:49:42 PDT
Created attachment 316909 [details] [diff] [review]
Patch to fix

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
Comment 34 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-04-21 16:50:37 PDT
Created attachment 316911 [details] [diff] [review]
Same as above without -w
Comment 35 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-04-22 15:28:25 PDT
Created attachment 317112 [details] [diff] [review]
Patch with tests (-w)

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.
Comment 36 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-04-23 14:36:23 PDT
Created attachment 317379 [details] [diff] [review]
Fix reftests [checked in]

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 37 Daniel Holbert [:dholbert] 2008-04-23 14:43:35 PDT
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 38 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-04-23 14:52:05 PDT
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 39 Mike Beltzner [:beltzner, not reading bugmail] 2008-04-23 17:55:30 PDT
Comment on attachment 317379 [details] [diff] [review]
Fix reftests [checked in]

a=beltzner, though not needed for tests
Comment 40 Boris Zbarsky [:bz] 2008-04-24 15:23:34 PDT
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.
Comment 41 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-04-24 16:42:51 PDT
(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
Comment 42 Boris Zbarsky [:bz] 2008-04-24 18:46:09 PDT
> 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.
Comment 43 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-04-24 19:56:53 PDT
Created attachment 317655 [details] [diff] [review]
Fix comments (with -w)
Comment 44 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-04-24 19:58:12 PDT
Created attachment 317656 [details] [diff] [review]
Fix comments (with -w)
Comment 45 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-04-24 19:58:42 PDT
Created attachment 317657 [details] [diff] [review]
Fix comments (without -w)
Comment 46 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-04-24 21:02:27 PDT
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 47 Boris Zbarsky [:bz] 2008-04-24 21:33:56 PDT
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.
Comment 48 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-04-27 18:37:22 PDT
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?
Comment 49 Boris Zbarsky [:bz] 2008-04-27 20:28:29 PDT
For non-chrome documents, yes.  We probably need to adjust the documentation accordingly...
Comment 50 Ben Turner (not reading bugmail, use the needinfo flag!) 2008-04-28 12:03:22 PDT
Created attachment 318209 [details] [diff] [review]
Final patch [checked in]

(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.
Comment 51 Boris Zbarsky [:bz] 2008-04-28 12:50:17 PDT
For what it's worth, the "Final patch" looks good to me.  Ben, thanks for picking this up!
Comment 52 Damon Sicore (:damons) 2008-04-28 15:50:58 PDT
Comment on attachment 318209 [details] [diff] [review]
Final patch [checked in]

a1.9+=damons
Comment 53 Ben Turner (not reading bugmail, use the needinfo flag!) 2008-04-28 17:22:35 PDT
Comment on attachment 318209 [details] [diff] [review]
Final patch [checked in]

Landed on trunk.
Comment 54 Ben Turner (not reading bugmail, use the needinfo flag!) 2008-04-28 17:22:56 PDT
Fixed.
Comment 55 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-04-28 23:22:01 PDT
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.
Comment 56 Eric Shepherd [:sheppy] 2008-04-29 08:01:23 PDT
Docs have been updated.
Comment 57 Boris Zbarsky [:bz] 2008-05-18 09:37:22 PDT
It looks like we don't document the same-origin policy anywhere, unless I missed it...  That needs to be fixed, per comment 55.
Comment 58 Jeff Walden [:Waldo] (remove +bmo to email) 2008-05-18 13:30:18 PDT
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.
Comment 59 Boris Zbarsky [:bz] 2008-05-18 16:23:59 PDT
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.
Comment 60 Boris Zbarsky [:bz] 2008-05-18 16:24:27 PDT
For example, the XBL request origin is never affected by document.domain setting.
Comment 61 Eric Shepherd [:sheppy] 2008-05-19 11:05:09 PDT
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.
Comment 62 Boris Zbarsky [:bz] 2008-05-19 11:30:12 PDT
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.
Comment 63 Eric Shepherd [:sheppy] 2008-05-19 11:59:05 PDT
Does this policy work the same way as for JavaScript content, or does XBL have a different policy?
Comment 64 Boris Zbarsky [:bz] 2008-05-19 12:36:17 PDT
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.
Comment 65 Eric Shepherd [:sheppy] 2008-05-19 12:54:27 PDT
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.
Comment 66 Boris Zbarsky [:bz] 2008-05-19 13:50:59 PDT
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.
Comment 67 Eric Shepherd [:sheppy] 2008-05-19 14:46:09 PDT
OK... I should probably document both of these.  Where can I get info on the JavaScript policy?
Comment 68 Boris Zbarsky [:bz] 2008-05-19 15:01:56 PDT
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...
Comment 69 Eric Shepherd [:sheppy] 2008-05-20 13:37:46 PDT
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).
Comment 70 Boris Zbarsky [:bz] 2008-05-20 14:35:20 PDT
That looks great.  Thanks!
Comment 71 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-05-20 16:50:53 PDT
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.
Comment 72 Daniel Veditz [:dveditz] 2008-06-02 12:04:23 PDT
With Jonas out it doesn't look like a branch fix is coming any time soon.
Comment 73 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-06-03 01:29:37 PDT
And since this is a compat change I think we should get some FF3 baking before putting this on branch.
Comment 74 Samuel Sidler (old account; do not CC) 2008-08-26 23:27:11 PDT
Jonas didn't get to this in time for 1.8.1.17.
Comment 75 Samuel Sidler (old account; do not CC) 2008-10-16 16:54:57 PDT
Jonas, any update here? Code freeze is in about a week...
Comment 76 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-11-18 18:24:24 PST
Created attachment 348889 [details] [diff] [review]
Branch patch -w

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.
Comment 77 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-11-18 18:41:27 PST
Created attachment 348890 [details] [diff] [review]
Branch patch 2 -w

Use TYPE_XBL when doing content-policy checks, and call CheckLoadURI even though you can't get hold of the data.
Comment 78 Boris Zbarsky [:bz] 2008-11-18 18:43:02 PST
Comment on attachment 348890 [details] [diff] [review]
Branch patch 2 -w

r+sr=bzbarsky
Comment 79 Samuel Sidler (old account; do not CC) 2008-11-18 19:50:23 PST
Comment on attachment 348890 [details] [diff] [review]
Branch patch 2 -w

Approved for 1.8.1.19. a=ss
Comment 80 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-11-18 19:52:58 PST
Checked in to 1.8 branch! Yay!
Comment 81 Al Billings [:abillings] 2008-11-25 17:15:37 PST
Jonas, is there a simple way to test this in the 1.8 branch?
Comment 82 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-12-03 12:11:03 PST
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 83 Alexander Sack 2008-12-16 00:40:24 PST
Comment on attachment 348890 [details] [diff] [review]
Branch patch 2 -w

a=asac for 1.8.0 branch
Comment 84 Mike Beltzner [:beltzner, not reading bugmail] 2008-12-17 15:01:49 PST
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
Comment 85 Samuel Sidler (old account; do not CC) 2008-12-17 15:19:39 PST
(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 86 Daniel Veditz [:dveditz] 2008-12-17 15:26:03 PST
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.
Comment 87 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-12-17 15:33:53 PST
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.
Comment 88 Mike Beltzner [:beltzner, not reading bugmail] 2008-12-17 15:41:49 PST
(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 89 Alexander Sack 2008-12-18 05:30:12 PST
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.
Comment 90 Boris Zbarsky [:bz] 2008-12-18 18:48:50 PST
> 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.
Comment 91 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-12-18 19:54:02 PST
And I when writing it :(
Comment 92 :Gijs Kruitbosch (away 26-29 incl.) 2015-02-23 07:48:56 PST
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?)
Comment 93 Boris Zbarsky [:bz] 2015-02-23 17:58:32 PST
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...

Note You need to log in before you can comment on or make changes to this bug.