1050049 - Whitelist XBL bindings that may be applied to untrusted content

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=a4814d18557d

Comment 2

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=ef8021c53aec

Comment 3

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=f5ad093a9c9f

Comment 4

[Bobby Holley (:bholley)]

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.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 1 - Whitelist bindings that we actually expect to use in content. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 2 - Implement bindToUntrustedContent attribute restriction. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 3 - Tests. v1

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

(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 9

[Olli Pettay [:smaug][[e-mail]]]

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 10

[Olli Pettay [:smaug][[e-mail]]]

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 11

[Olli Pettay [:smaug][[e-mail]]]

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 12

[Olli Pettay [:smaug][[e-mail]]]

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.)

Comment 14

[Bobby Holley (:bholley)]

(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.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

 
> (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.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

(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 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8470487 [details] [diff] [review]
Part 2 - Implement bindToUntrustedContent attribute restriction. v1

looks ok then.

Comment 18

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=97bc2cc96071

Comment 19

[Bobby Holley (:bholley)]

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

Comment 20

[[e-mail]]

(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?

Comment 21

[Gary [:streetwolf52]]

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

Comment 22

[Gary [:streetwolf52]]

The add-on FlaskBlock https://addons.mozilla.org/en-US/firefox/addon/flashblock/?src=ss has similar problems as FlashStopper.

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ce94fbaec83c
https://hg.mozilla.org/mozilla-central/rev/1358c1ad00b4
https://hg.mozilla.org/mozilla-central/rev/9648b3e555db

Comment 24

[Gary [:streetwolf52]]

Submitted new bug report --> https://bugzilla.mozilla.org/show_bug.cgi?id=1052480

Comment 25

[Bobby Holley (:bholley)]

(In reply to [e-mail] 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.

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Bobby, could you propose a wording for this bug?

Comment 27

[Bobby Holley (:bholley)]

(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.

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Bobby, I wonder if that it is not too specific to be part of the release notes. Am I wrong?

Comment 29

[Bobby Holley (:bholley)]

(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.

Comment 30

[Peter Wu]

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