Right now, content can attach arbitrary XBL bindings from chrome://global to arbitrary nodes. All of the interesting stuff from these bindings runs in the XBL scope, but this behavior still exposes a wide and untested set of funny behavior, which good defense-in-depth requires us to eliminate. I'm going to try implementing an attribute on <binding> called bindToUntrustedContent, defaulting to false, and stick it on the handful of XBL controls that we actually intend to expose to content.
https://tbpl.mozilla.org/?tree=Try&rev=a4814d18557d
https://tbpl.mozilla.org/?tree=Try&rev=ef8021c53aec
https://tbpl.mozilla.org/?tree=Try&rev=f5ad093a9c9f
https://tbpl.mozilla.org/?tree=Try&rev=afcc3c005cf2 If this try push is green, this should be ready for review. Adding various flags now before I forget. Release Note Request (optional, but appreciated) [Why is this notable]: This changes the semantics of XBL bindings such that they need a special annotation in order to be bound to untrusted content. This has the potential to break addons (like flashblock), though the fix required is minimal. [Suggested wording]: [Links (documentation, blog post, etc)]: Please ping me back on both of these closer to the release.
Created attachment 8470486 [details] [diff] [review] Part 1 - Whitelist bindings that we actually expect to use in content. v1
Created attachment 8470487 [details] [diff] [review] Part 2 - Implement bindToUntrustedContent attribute restriction. v1
Created attachment 8470488 [details] [diff] [review] Part 3 - Tests. v1
(In reply to Bobby Holley (:bholley) from comment #0) > Right now, content can attach arbitrary XBL bindings from chrome://global to > arbitrary nodes. How? haven't we disabled -moz-binding from non-chrome css files + from non-chrome js?
Comment on attachment 8470486 [details] [diff] [review] Part 1 - Whitelist bindings that we actually expect to use in content. v1 >+ <binding id="browser" bindToUntrustedContent="true"> Why this >+ <binding id="editor" bindToUntrustedContent="true"> and this
Comment on attachment 8470487 [details] [diff] [review] Part 2 - Implement bindToUntrustedContent attribute restriction. v1 It is not clear to me how prototype binding inheritance should work here. I think all the bindings in the inheritance list should have the attribute in order to let the binding to be attached to untrusted content. Otherwise the attribute doesn't really protect much. (ChromeOnlyContent, as an example, is a bit different kind of flag, and there we search for the proto binding which anon content will be used, and check ChromeOnlyContent flag of that proto)
Comment on attachment 8470486 [details] [diff] [review] Part 1 - Whitelist bindings that we actually expect to use in content. v1 (with the question about browser and editor answered, r+)
Comment on attachment 8470488 [details] [diff] [review] Part 3 - Tests. v1 Please make sure we have a bug open to disable and hopefully hide MozBinding from web content. (Use of Promise and bind hurt my eyes, but fine.)
(In reply to Olli Pettay [:smaug] from comment #9) > Comment on attachment 8470486 [details] [diff] [review] > Part 1 - Whitelist bindings that we actually expect to use in content. v1 > > >+ <binding id="browser" bindToUntrustedContent="true"> > Why this > > >+ <binding id="editor" bindToUntrustedContent="true"> > and this Isn't editor bound to text boxes and whatnot? Or did I misunderstand? Anyway, I'm happy to strip them out of that makes sense. (In reply to Olli Pettay [:smaug] from comment #10) > Comment on attachment 8470487 [details] [diff] [review] > Part 2 - Implement bindToUntrustedContent attribute restriction. v1 > > It is not clear to me how prototype binding inheritance should work here. > I think all the bindings in the inheritance list should > have the attribute in order to let the binding to be attached to untrusted > content. Otherwise the attribute doesn't really protect much. Isn't that exactly what this patch does? We invoke GetBinding recursively for base bindings, so we'll do the security check at each binding.
> (In reply to Olli Pettay [:smaug] from comment #10) > > Comment on attachment 8470487 [details] [diff] [review] > > Part 2 - Implement bindToUntrustedContent attribute restriction. v1 > > > > It is not clear to me how prototype binding inheritance should work here. > > I think all the bindings in the inheritance list should > > have the attribute in order to let the binding to be attached to untrusted > > content. Otherwise the attribute doesn't really protect much. > > Isn't that exactly what this patch does? We invoke GetBinding recursively > for base bindings, so we'll do the security check at each binding. Aha, that is not clear from the patch, but looks like we do that. Looking the patch again.
(In reply to Bobby Holley (:bholley) from comment #14) > (In reply to Olli Pettay [:smaug] from comment #9) > > Comment on attachment 8470486 [details] [diff] [review] > > Part 1 - Whitelist bindings that we actually expect to use in content. v1 > > > > >+ <binding id="browser" bindToUntrustedContent="true"> > > Why this > > > > >+ <binding id="editor" bindToUntrustedContent="true"> > > and this > > Isn't editor bound to text boxes and whatnot? Or did I misunderstand? > Anyway, I'm happy to strip them out of that makes sense. As far as I know and see those are the magical global keyboard bindings. Only platformHTMLBindings.xml#inputFields and platformHTMLBindings.xml#textAreas are actually bound to some elements.
Comment on attachment 8470487 [details] [diff] [review] Part 2 - Implement bindToUntrustedContent attribute restriction. v1 looks ok then.
https://tbpl.mozilla.org/?tree=Try&rev=97bc2cc96071
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ce94fbaec83c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1358c1ad00b4 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9648b3e555db
(In reply to Olli Pettay from comment #8) > (In reply to Bobby Holley) > > Right now, content can attach arbitrary XBL bindings from chrome://global to > > arbitrary nodes. > How? haven't we disabled -moz-binding from non-chrome css files + from > non-chrome js? Did this question get answered?
Based on my regression testing this patch conflicts with the add-on FlashStopper v1.2.2. With FS enabled videos do not play at all. https://addons.mozilla.org/en-US/firefox/addon/flashstopper/ Good: 20140811200055 https://hg.mozilla.org/integration/mozilla-inbound/rev/d78a39f01102 Bad: 20140811200755 https://hg.mozilla.org/integration/mozilla-inbound/rev/9648b3e555db Mozilla/5.0 (Windows NT 6.3; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
The add-on FlaskBlock https://addons.mozilla.org/en-US/firefox/addon/flashblock/?src=ss has similar problems as FlashStopper.
https://hg.mozilla.org/mozilla-central/rev/ce94fbaec83c https://hg.mozilla.org/mozilla-central/rev/1358c1ad00b4 https://hg.mozilla.org/mozilla-central/rev/9648b3e555db
Submitted new bug report --> https://bugzilla.mozilla.org/show_bug.cgi?id=1052480
(In reply to neil@parkwaycc.co.uk from comment #20) > > > Right now, content can attach arbitrary XBL bindings from chrome://global to > > > arbitrary nodes. > > How? haven't we disabled -moz-binding from non-chrome css files + from > > non-chrome js? > Did this question get answered? It did, yes. My original comment wasn't entirely accurate, but there are good reasons to do this anyway as defense-in-depth.
Bobby, could you propose a wording for this bug?
(In reply to Sylvestre Ledru [:sylvestre] from comment #26) > Bobby, could you propose a wording for this bug? Something like: For improved security, XBL bindings must now have a "bindToUntrustedContent" attribute set to "true" in order to be attached to untrusted elements. Check the terminal output of a debug build to determine if a binding was vetoed based on this restriction.
Bobby, I wonder if that it is not too specific to be part of the release notes. Am I wrong?
(In reply to Sylvestre Ledru [:sylvestre] from comment #28) > Bobby, I wonder if that it is not too specific to be part of the release > notes. Am I wrong? Quite possibly - I don't have a sense of what's appropriate for them. If not, it should at least go on the addon compat blog.
For your information, this change has possibly broken NoScript Anywhere on Firefox for Android too. https://forums.informaction.com/viewtopic.php?f=25&t=20186
or
