Closed Bug 1321420 Opened 7 years ago Closed 7 years ago

Add a pref to restrict content in browser requests

Categories

(Chat Core :: Twitter, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Instantbird 53

People

(Reporter: arlolra, Assigned: arlolra)

Details

Attachments

(1 file, 2 obsolete files)

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
Attachment #8815941 - Attachment is patch: true
Attachment #8815941 - Attachment mime type: text/x-patch → text/plain
Component: Other → Twitter
Product: Instantbird → Chat Core
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?
Flags: needinfo?(arlolra)
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
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.
Flags: needinfo?(arlolra)
Doesn't the twitter auth page rely on JS?
> 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.
Summary: Restrict content in Twitter's auth browser → Add a pref to restrict content in browser requests
Attachment #8815941 - Attachment is obsolete: true
Attachment #8816233 - Flags: review?(aleth)
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
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 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.
(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).
> 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 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 ;)
Attachment #8816233 - Attachment is obsolete: true
Attachment #8816233 - Flags: review?(aleth)
Attachment #8816291 - Flags: review?(aleth)
Attachment #8816291 - Flags: review?(aleth) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/a1ff30b2483661bb2eede5649de61b5501e3063f
Assignee: nobody → arlolra
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 53
You need to log in before you can comment on or make changes to this bug.