1321420 - Add a pref to restrict content in browser requests

Status: RESOLVED FIXED

(Chat Core :: Twitter, defect)

Description

[arlolra]

Attachment: 0001-Restrict-content-in-browser-when-authenticating-w-Tw.patch

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.98 Safari/537.36

Comment 1

[Patrick Cloke [:clokep]]

arlolra: Please include a description when filing bugs. There's no context about what this is trying to fix or what the issue is. Why was this change made? What is it fixing?

Comment 2

[arlolra]

Sorry for not providing context.

This was in response to yesterday's exploit,
https://blog.mozilla.org/security/2016/11/30/fixing-an-svg-animation-vulnerability/

I was trying to reduce the surface in which arbitrary JS can be executed in Instantbird.

Comment 3

[aleth [:aleth]]

Doesn't the twitter auth page rely on JS?

Comment 4

[arlolra]

> Doesn't the twitter auth page rely on JS?

Unless I did something wrong, I built with this patch and was able to successfully authenticate.  It's possible certain client side validation would be disabled, but that doesn't prevent it from working.

Comment 5

[arlolra]

Attachment: 0001-Bug-1321420-Add-a-pref-to-restrict-content-in-browse.patch

Comment 6

[aleth [:aleth]]

Comment on attachment 8816233 [details] [diff] [review]
0001-Bug-1321420-Add-a-pref-to-restrict-content-in-browse.patch

Review of attachment 8816233 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/content/browserRequest.js
@@ +137,5 @@
>    let browser = document.getElementById("requestFrame");
> +
> +  if (Services.prefs.getBoolPref("chat.browserRequest.restrictContent")) {
> +    browser.docShell.allowJavascript = false;
> +    browser.docShell.allowPlugins = false;

Do you need this? Aren't plugins disabled app-wide already?

Comment 7

[arlolra]

Comment on attachment 8816233 [details] [diff] [review]
0001-Bug-1321420-Add-a-pref-to-restrict-content-in-browse.patch

Review of attachment 8816233 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/content/browserRequest.js
@@ +137,5 @@
>    let browser = document.getElementById("requestFrame");
> +
> +  if (Services.prefs.getBoolPref("chat.browserRequest.restrictContent")) {
> +    browser.docShell.allowJavascript = false;
> +    browser.docShell.allowPlugins = false;

https://github.com/mozilla/releases-comm-central/blob/master/im/app/profile/all-instantbird.js#L264-L272

I'm not sure what plugins in xpis are, but they seem to be enabled by default.

Comment 8

[aleth [:aleth]]

(In reply to arlolra from comment #7)
> I'm not sure what plugins in xpis are, but they seem to be enabled by
> default.

Plugins in addons. I think we'd probably accept a patch to disable these (r? flo).

Comment 9

[arlolra]

> I think we'd probably accept a patch to disable these

Happy to supply it, if desired.

Can we leave `allowPlugins = false` as defence in depth?

Comment 10

[aleth [:aleth]]

Comment on attachment 8816233 [details] [diff] [review]
0001-Bug-1321420-Add-a-pref-to-restrict-content-in-browse.patch

Review of attachment 8816233 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/chat-prefs.js
@@ +90,5 @@
>  pref("chat.prpls.prpl-yahoo.disable", true);
>  // Whether to disable SRV lookups that use the system DNS library.
>  pref("chat.dns.srv.disable", false);
> +// Restricting what documents that contain untrusted data can do.
> +pref("chat.browserRequest.restrictContent", false);

chat.browserRequest.disableJavascript would be clearer.

::: chat/content/browserRequest.js
@@ +137,5 @@
>    let browser = document.getElementById("requestFrame");
> +
> +  if (Services.prefs.getBoolPref("chat.browserRequest.restrictContent")) {
> +    browser.docShell.allowJavascript = false;
> +    browser.docShell.allowPlugins = false;

If you really want to keep this, move it outside the if clause, I see no use case for plugins in chat browserRequests ;)

Comment 11

[arlolra]

Attachment: 0001-Bug-1321420-Add-a-pref-to-disable-JavaScript-in-brow.patch from comment 10

Comment 12

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/a1ff30b2483661bb2eede5649de61b5501e3063f