As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 1321420 - Add a pref to restrict content in browser requests
: Add a pref to restrict content in browser requests
Status: RESOLVED FIXED
:
Product: Chat Core
Classification: Components
Component: Twitter (show other bugs)
: trunk
: Unspecified Unspecified
: -- normal
: Instantbird 53
Assigned To: arlolra
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-11-30 16:36 PST by arlolra
Modified: 2016-12-01 15:43 PST (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
0001-Restrict-content-in-browser-when-authenticating-w-Tw.patch (1.08 KB, patch)
2016-11-30 16:36 PST, arlolra
no flags Details | Diff | Splinter Review
0001-Bug-1321420-Add-a-pref-to-restrict-content-in-browse.patch (2.90 KB, patch)
2016-12-01 11:39 PST, arlolra
no flags Details | Diff | Splinter Review
0001-Bug-1321420-Add-a-pref-to-disable-JavaScript-in-brow.patch from comment 10 (2.87 KB, patch)
2016-12-01 14:24 PST, arlolra
aleth: review+
Details | Diff | Splinter Review

Description User image arlolra 2016-11-30 16:36:19 PST
Created attachment 8815941 [details] [diff] [review]
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 User image Patrick Cloke [:clokep] 2016-12-01 04:53:09 PST
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 User image arlolra 2016-12-01 08:14:46 PST
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 User image aleth [:aleth] 2016-12-01 08:16:49 PST
Doesn't the twitter auth page rely on JS?
Comment 4 User image arlolra 2016-12-01 08:21:23 PST
> 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 User image arlolra 2016-12-01 11:39:34 PST
Created attachment 8816233 [details] [diff] [review]
0001-Bug-1321420-Add-a-pref-to-restrict-content-in-browse.patch
Comment 6 User image aleth [:aleth] 2016-12-01 13:51:25 PST
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 User image arlolra 2016-12-01 13:56:47 PST
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 User image aleth [:aleth] 2016-12-01 14:00:04 PST
(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 User image arlolra 2016-12-01 14:04:27 PST
> 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 User image aleth [:aleth] 2016-12-01 14:07:28 PST
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 User image arlolra 2016-12-01 14:24:48 PST
Created attachment 8816291 [details] [diff] [review]
0001-Bug-1321420-Add-a-pref-to-disable-JavaScript-in-brow.patch from comment 10

Note You need to log in before you can comment on or make changes to this bug.