Closed Bug 379644 Opened 13 years ago Closed 13 years ago

let's kill xbl for untrusted content

Categories

(Core :: XBL, defect, P2)

defect

Tracking

()

RESOLVED DUPLICATE of bug 546857

People

(Reporter: guninski, Assigned: sicking)

References

Details

Attachments

(1 file)

xbl is constantly causing troubles and it is not clear if it is manageable with current resources:

fork()ed from Bug 378518 Comment #4  

G30rgi   2007-05-03 06:38:13 PDT

what about an option |disallow xbl| from userland?

probably the web will be usable and chrome will be unaffected.

Comment #5 [reply] Boris Zbarsky  2007-05-03 07:45:05 PDT

That's an interesting question.  Until recently, this hasn't so much been an
option.  At this point we _could_ do it.  The only question is how many apps it
would break.

Comment #6 [reply] G30rgi  2007-05-03 08:38:20 PDT

(In reply to comment #5)
> The only question is how many apps it would break.
> 

probably not exactly "apps" but "unprivileged web stuff".

not many people cry that "extends" in xbl causes SEGV.

Comment #7 [reply] G30rgi  2007-05-03 08:48:58 PDT

(In reply to comment #6)
> (In reply to comment #5)
> > The only question is how many apps it would break.
> > 
> 
> "unprivileged web stuff".
> 

"disable xbl option" is less than clever.

to put it this way:

for standard compliance reasons and the unlimited power of XBL usage of XBL
from luserland requires universalbrowserwrite privilege.



Comment #8 [reply] Boris Zbarsky  2007-05-03 09:20:40 PDT

Unfortunately, there's no way to request that privilege in declarative content
(e.g. CSS).

And I do mean "apps" as in "intranet apps"...

Yes, I would not have made XBL available to untrusted content up front, but the
cat is mostly out of the bag now.  Can we please take discussion about that
aspect to a separate bug and stop adding things to this one?
Does this mean "Continue to allow content to bind to XBL bindings which are available in chrome, but not arbitrary bindings"?

I don't think we have the luxury of breaking remote XUL.
So what is currently possible is to determine exactly where the -moz-binding declaration came from.  Which gives us three pieces of data:

1)  The principal of the source of the -moz-binding declaration (e.g. xul.css,
    etc).
2)  The binding URI.
3)  The principal of the node the binding is being applied to.

At the moment, we allow binding attachment if:

a)  The binding URI is a chrome:// URI
or
b)  The principal of the -moz-binding declaration source can link to the
    binding URI (as represented by CheckLoadURI).

I'd like to know what G30rgi's exact proposal is for changes to this setup.
Note also that we should consider deployment of:

* http://dean.edwards.name/moz-behaviors/usage/

* http://www.activewidgets.com/ (based on http://www.activewidgets.com/javascript.forum.13633.1/firefox-1-5-and-data.html comment)

I stopped on page 2 of the Google listing.
Blocks: killsbabies
Does this bug need to be security-sensitive?  I don't see anything in this bug that sounds like it needs to be, and non-security people should be part of this discussion...
I don't think it should, no.

Of course I also think it's in the wrong product...
Assignee: nobody → general
Group: security
Component: Security → XBL
Product: Firefox → Core
QA Contact: firefox → ian
I don't think we can do this. If we allowed binding to chrome bindings (in order to not break marquee and xul) we would still be exposable to all the issues that we currently have.
neither blame nor flame BZ, just feel for him for having to fix the
current state of XBL.

the current state of XBL is known and interesting kludges are starting
to emerge - Bug 378518

i am no xbl expert and the purpose of this bug is to disallow as much
xbl as possible while not breaking chrome and pissing as low
people/companies as possible (unless XBL is your hidden ace card).

afaict this potentially may be done by an option disabling untrusted
bindings or requiring a privilege for untrusted bindings - that is,
untrusted content doing |-moz-binding: P0Wned.com| and similar
equivalent stuff.

can't comment how much developers this will piss off, but the fact that
developers aren't noticing |extends| causes SEGV makes me bet
`negligible' number of developers may be pissed off.

sure this proposal is unpopular, but i support the following:

making *all* users happy is mistake leading to unmanageable
features/bugs/bloat.

imho the goal should be to implement the *minimal functionality that
will make about 80%-90% of all browser users happy* (basically this
seems the opera approach)

https://wiki.ubuntu.com/WinningTheDesktop 

BZ's examples show some developers will be pissed off - it is unclear
what percentage of current users will be pissed off.

feel free to resolve this bug as you wish.
(In reply to comment #3)
> 
> * http://www.activewidgets.com/ (based on
> http://www.activewidgets.com/javascript.forum.13633.1/firefox-1-5-and-data.html
> comment)
> 
[ActiveWidgets]$ grep -rniI binding *
[ActiveWidgets]$ grep -rniI moz-binding *
[ActiveWidgets]$
tried to measure the popularity of "-moz-binding" this way:

+"moz-binding" +url -chrome -resource -mBinding -NS_LITERAL_STRING -xss -nsCAutoString
http://www.google.com/codesearch?hl=en&q=+%2B%22moz-binding%22+%2Burl+-chrome+-resource+-mBinding+-NS_LITERAL_STRING+-xss+-nsCAutoString&start=90&sa=N

+"-moz-binding" -chrome -resource -xss -vulnerability -marquee
http://www.google.com/search?q=%2B%22-moz-binding%22+-chrome+-resource+-xss+-vulnerability+-marquee&hl=en&start=30&sa=N
You also need to search for uses of XUL since this would break remote XUL.
And this would break at least custom controls in XForms, if not whole XForms.
Which "this" do comment 10 and comment 11 refer to?  That is, what modifications are people thinking about to the model in comment 2?

And as a second question, what modifications would actually help the security situation?
(In reply to comment #11)
> And this would break at least custom controls in XForms

if -moz-binding: url(http://url/to/some/content/xbl) is disabled in normal content

> if not whole XForms.
if -moz-binding: url(chrome://something) is disabled in normal content
(this is the same as remote XUL). The .css file used for binding is in this case also in chrome://
 

One possibility is:
* Disallow content pages from introducing new bindings, but allow chrome and user style sheets to introduce bindings to content pages.
* Disallow content pages from using getAnonymousNodes, so they can't exploit insertion-point bugs.
That second bullet point would mean that a binding attached to an untrusted node also couldn't call getAnonymousNodes (since it's running with the permissions of the node it's attached to).  Would that cause problems?
Disabling GetAnonymousNodesFor is the 'this' I was referring to. We would need to do it, but that would break all XUL elements that call it, which I bet is a lot.
This site uses xbl (and getAnonymousNodes):
http://webfx.eae.net/dhtml/xblmarquee/xblmarquee.html
It's basically where I learned how to use xbl.

I used it myself a lot on http://www.kantjils.nl/ when I was the webmaster there.
I would be extremely unhappy, if xbl would be disabled.
(In reply to comment #12)
> 
> And as a second question, what modifications would actually help the security
> situation?
> 

stealing bindings and/or xml?

same origin checks?
> stealing bindings and/or xml?

Meaning what?

I'm not sure how same origin checks would help.  Checks where?
(In reply to comment #19)
> > stealing bindings and/or xml?
> 
> Meaning what?
> 
> I'm not sure how same origin checks would help.  Checks where?
> 

meaning:

one visits http://evil which does '-moz-binding: url(http://secret/secret.xml)'

if secret.xml is secret binding may its source be read?
if secret.xml is just (valid) xml (not necessarily binding) may it be read?

does the spec say something about it?

Ah, I see.

I think the answer is mostly no to those questions (though you can get your hands on the anon content the binding clones and installs, if any).
so in the future the binding's source shouldn't be exposed?
Right.
Bug 371694 mentions "implement nsISecurityCheckedComponent"
the above bug is not very clear but is this possible from xbl?
implements="..."
So it sounds like this bug is drifting into "make xbl suck less" rather than "kill xbl". If so we should close this bug and open bugs on specific issues?
(In reply to comment #26)
> So it sounds like this bug is drifting into "make xbl suck less" rather than
> "kill xbl". If so we should close this bug and open bugs on specific issues?
> 

sure, close as you wish.

from my point of view |implements| is dead, the future of |extends| is unclear and  |binding| is still alive :)
feel free to resolve as anyone wishes
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
i dare propose a weaker version:

let disable user xbl/bindings in thunberbird - both in preview pane and in editor.
I really think we should remove support for -moz-binding from untrusted content.  It's a vector to script execution that _no_ sanitizer expects to find, though of course someone out there uses it; someone uses everything, even our bugs.  I think we should be willing to break the _tiny_ number of people using them in order to remove this attack surface.  (Similarly for linking to chrome: from content, though that's another bug.)

A lesser alternative would be to restrict -moz-binding in content to strictly same origin loads (no data:, for example), which would reduce the threat of a -moz-binding being inserted against the site operator's knowledge.
Status: RESOLVED → REOPENED
Flags: blocking1.9?
OS: Linux → All
Hardware: PC → All
Resolution: WONTFIX → ---
I'm all for restricting it to same-origin by default... I really don't think we should neuter remote XBL altogether.
See also bug 324253, "Do something about the XSS issues -moz-binding introduces".  But like Shaver, I'm more worried about crashes and memory safety than XSS.  Rumor has it that it would be very hard to fix the known memory safety issues with XBL 1.

I don't think the compatibility argument holds water.  We're going to get rid of XBL 1 at some point anyway when we replace it with the saner XBL 2, right?
> I really don't think we should neuter remote XBL altogether.

This needs to be backed up with some reasoning. We will take a Firefox-compat hit on a few sites if we do this--is that the concern?
Most of the sites I'm concerned about are not public-web but intranet: one of the lasting impressions I got from the Paris devday was how much XBL was in use on non-chrome intranet sites.

It might be sufficient for this purpose to disable remote-XBL by default and enable it via pref. But I really don't want to say that Le Monde just can't use Firefox 3 for their online editorial staff any more.

And boy, I wish we had brought this up sooner because I believe without good data that there's at least as much if not more XBL out on the web as MathML.
Can intranet sites not sign and elevate?  Do we still support SSL-as-implicit-signature?  A default pref off would be ok, site-specific for turn-on would be better.  I really don't want to say that Le Monde published a front-page lolcat because they did use Firefox 3 with XBL enabled.

If you're not worried about the public web, then why worry about how much MathML there is?  (I'd rather see us support MathML than XBL for content, though; at least there's a specification and I believe something resembling a test suite.)
(In reply to comment #33)
> I don't think the compatibility argument holds water.  We're going to get rid
> of XBL 1 at some point anyway when we replace it with the saner XBL 2, right?

Going from XBL1 to *nothing* and then to XBL2 is a lot harsher than from XBL1 to XBL2.
> Can intranet sites not sign and elevate? 

Not really, no, unless there happens to be JS on the stack while the XBL load is kicking off (so during frame construction, say).

I suppose we could add a caps thing to turn this on by hostname.  But it might make sense to allow same-origin no matter what.  It would be _really_ good to get some actual info on the uses.  :(

> Do we still support SSL-as-implicit-signature?

I'm not sure what you mean, but I suspect the answer is "no" no matter what it is...
i continue to think that no version of XBL should be accessible from userland.

(the sanitizers arguments is not correct imo - sanitizers should use whitelist normalization, so they should not be vulnerable to constructs they don't know of).

sure some apps will break. so what? some users and developers will be pissed off. later they'll get over it. they will be more pissed off if they get owned constantly because of xbl features.

you can't please everyone.
Since it sounds like XBL is actually used out there I don't think we should remove it. Signing and things like that is a major headache, and not really something that I think we should encourage anyway. And if we're starting to have to put in a lot of effort to re-enabling XBL in the cases where we think it should still be permitted, we might as well put that effort into fixing the bugs we have with XBL instead.
(In reply to comment #41)
> Since it sounds like XBL is actually used out there I don't think we should
> remove it.

I don't understand this -- of course XBL is used out there, it's in a browser.  boxObject is used out there too, and more widely, to say nothing of cross-document node movement, but we were willing to make authors update for those cases only for spec purity.  Why would you blanket object to disabling content XBL on the basis of there being _some_ use?

(All the reasons in favour of boxObject and WRONG_DOCUMENT_ERR are ten times more applicable here, _plus_ security issues.)

We break things in _minor_ updates for security, like jar:, and expect people to cope, because it's for the greater good.

> Signing and things like that is a major headache, and not really
> something that I think we should encourage anyway.

Why don't you think we should encourage signing of code?  (If we treat SSL-delivered content as signed, it gets a lot easier, though we would need some way to deal with bz's issue -- they could perhaps add the binding rules through the CSSOM from privileged script?)

> And if we're starting to
> have to put in a lot of effort to re-enabling XBL in the cases where we think
> it should still be permitted, we might as well put that effort into fixing the
> bugs we have with XBL instead.

I don't think we have a good handle on what bugs we do have, and the code is (very) hard to test.

I'm not talking about a lot of effort, I'm talking about disabling -moz-binding processing from content, maybe with a (site-specific) pref override.  Le Monde can ship an extension or non-default pref, or whatever -- after they get their stuff working again from the constructor and other XBL changes we made on the trunk.  Everyone else can sleep easier.  (Do Le Monde have any Windows 98 machines?  X with pseudocolour displays?  Panther?)
I fully agree with shaver, even though TomTom HOME 2 *does* use XBL in content. I'd be happy to adapt to whatever is there, or even get rid of the current solution altogether, if that's improving Firefox security.

In fact, if we think it's a security hazard that cannot get fixed in the near future, I think it's mandatory to disable it.
Introducing a same-origin check will not help the surfer in that case either. I think a pref (ideally per site) that intranet users can enable would be a good compromise.
(In reply to comment #42)
> (In reply to comment #41)
> > Since it sounds like XBL is actually used out there I don't think we should
> > remove it.
> 
> I don't understand this -- of course XBL is used out there, it's in a
> browser. 
> boxObject is used out there too, and more widely, to say nothing of
> cross-document node movement, but we were willing to make authors update for
> those cases only for spec purity.

For both boxObject and node movement, there are simple local changes that authors can apply to fix their content. If we just disable XBL there are no simple changes that will get that content working again (other than policy changes that reenable XBL for that content, such as signing the content).

FWIW we haven't disabled boxObject for Web content in FF3, to give authors a transition period during which they can migrate to new APIs, especially getBoundingClientRect which we introduced in FF3 to handle the common use-case for boxObject.
We need to figure out something here
Assignee: xbl → jonas
Status: REOPENED → NEW
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
please at least give me a preference to disable remote xbl globally
Please see comment 45.  Doing that is not enough to solve the problems.  It just gives a false sense of security.
(In reply to comment #48)
> Please see comment 45.

someone has rm-ed this comment so fast it is not even in my mail
If you really do just want that, writing an extension that does it via content policy is pretty easy...
Comment 45 is marked as private.  georgi, you're not in the security group on Bugzilla?
(In reply to comment #51)
> Comment 45 is marked as private.  georgi, you're not in the security group on
> Bugzilla?
> 

not in this group.
I'm injecting XBL into content documents with my Image Toolbar extension in order to capture image hover events and raise a custom event (by binding to images), which worked a lot better than trying to add event listeners all over the place.

If you're disabling XBL for untrusted content, presumably for 3.0, am I suddenly going to be faced with a large rewrite before the RCs, or will my method still work because it was added from chrome? If it has any relevance, currently my code fails if JavaScript is disabled due to bug 236839.
That's one of the use cases we're trying hard to not break.
Ben: Specifically how are you adding those bindings? Are you inserting CSS rules somehow or are you using .addBinding? Or some other way?
I'm currently inserting CSS bindings using nsIStyleSheetService loadAndRegisterSheet().
Jonas says this is WONTFIX.
Status: NEW → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → WONTFIX
Can you please elaborate why? Last state of comments here is still the investigation.
So there are two categories of concerns:

1. XBL allows CSS to execute script.
2. We have exploitable crash bugs in the implementation.

So I realized 1 is an issue in IE too, which makes it slightly less bad. We're also going to tighten the same-origin policy better which will require that people not only that people can insert CSS rules, but also upload XBL files with an XML mimetype to the same server.

2 turned out to be not so bad. We have it mostly under control and have gotten pretty good testing on it. There is a set of bugs that needs to be fixed, but that should be possible to happen pretty quickly.

Additionally, it's turned out that XBL is in fact used in intranet apps. This makes me very reluctant to disable it. Especially with no real alternative. 
The intranet-app case is why we proposed per-site pref machinery, no?  I'm glad to hear that our XBL testing situation is in better shape than I'd thought.
So the main reason i was for the per-site opt-in was to deal with the issue 1 in comment 59. But given that IE also allows scripting via CSS, and we'd be able to block only using CSS to insert scripts (you'd have to upload the XBL as a separate file as well) i'm less worried.
(In reply to comment #61)
> But given that IE also allows scripting via CSS

I don't think that should be a factor in our decision.
> We're also going to tighten the same-origin policy better which will require that people not only that people can insert CSS rules, but also upload XBL files with an XML mimetype to the same server.

Does that mean we're going to break using data: URLs for XBL?  I rely on that feature for testing XBL, so I'll want some kind of workaround.  Some tests will need changes too: http://lxr.mozilla.org/mozilla/search?string=data%3Atext%2Fxml

(I think that feature was added in bug 366770, fwiw.)
Yes we will break data: urls, which means that it'll affect a pile of mochitests.

Would you be ok with a simple pref that'll enable data-urls again? Or would you be vary of turning that on since you could expose yourself to XSS attacks?
I'd be fine with using a pref.
btw, trunk allows bindings in the same file via data uri's and probably other ways  in xhtml - there were some bugzilla testcases with the binding in the same xhtml, not sure if they still work
(In reply to comment #64)
> Yes we will break data: urls, which means that it'll affect a pile of
> mochitests.

Are you talking about breaking them completely, or just for "content"?
Why do you need it for chrome? Seems like most of the time it's useful for testing and nothing else, and I plan to add a pref that enables it for testing.
how does data: uris differ from this inline xbl?

<div style="-moz-binding: url(#randomxbl);">test</div>
#randomxbl is declared in the same file.

testcase to follow.
Yes, that'll work too, but only if the mimetype of the document is text/xml or application/xml (I plan on not making it work for application/xhtml+xml) which is a very rare type for content insertion attacks.
(In reply to comment #71)
> Yes, that'll work too, but only if the mimetype of the document is text/xml or
> application/xml (I plan on not making it work for application/xhtml+xml) which
> is a very rare type for content insertion attacks.
> 

imho this is a kludge. so you are protecting text/html but not protecting text/xml ?
so i am completely missing why content insertion matters in this bug.

just make clear and document that "-moz-binding" allows content insertion just like "iframe" does and let CMS deal with their problems - they should strip "-moz-binding" the way they they strip "iframe" and "script" if their primary concern is injecting js/content.
I'm with georgi on this one. Do we need a new bug?

/be
Georgi's example relies on allowing users to upload XML files and then run script from them, right?  You can already do that trivially: just stick an <html:script> in the XML.  I don't think the XBL adds much to that vector...  if you're allowing people to upload arbitrary XML and putting it on the web, you're allowing those scripts to run, period.
>2 turned out to be not so bad. We have it mostly under control and have gotten
>pretty good testing on it.

being bug spammed by Bug 378518 i conjecture the above statement is completely wrong.
This was WONTFIXed, but eventually done as part of bug 546857.
http://mxr.mozilla.org/mozilla-central/search?string=allowxulxbl
Resolution: WONTFIX → DUPLICATE
Duplicate of bug: kill-remote-xul
You need to log in before you can comment on or make changes to this bug.