Closed Bug 1050049 Opened 6 years ago Closed 6 years ago

Whitelist XBL bindings that may be applied to untrusted content

Categories

(Core :: XBL, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
relnote-firefox --- 34+

People

(Reporter: bholley, Assigned: bholley)

References

Details

(Keywords: addon-compat, dev-doc-needed)

Attachments

(3 files)

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=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.
Attachment #8470488 - Flags: review?(bugs)
(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)
Attachment #8470487 - Flags: review?(bugs) → review-
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+)
Attachment #8470486 - Flags: review?(bugs) → review+
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.)
Attachment #8470488 - Flags: review?(bugs) → review+
(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.
Attachment #8470487 - Flags: review- → review+
Blocks: 1052307
(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.
Blocks: 1052480
No longer blocks: 1052480
Depends on: 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.
Depends on: 1053398
Depends on: 1054162
Bobby, could you propose a wording for this bug?
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
Depends on: 1062035
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
You need to log in before you can comment on or make changes to this bug.