Add a pref to restrict content in browser requests

RESOLVED FIXED in Instantbird 53

Status

Chat Core
Twitter
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: arlolra, Assigned: arlolra)

Tracking

trunk
Instantbird 53

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

9 months ago
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
(Assignee)

Updated

9 months ago
Attachment #8815941 - Attachment is patch: true
Attachment #8815941 - Attachment mime type: text/x-patch → text/plain
(Assignee)

Updated

9 months ago
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
Last Resolved: 9 months ago
Resolution: --- → INVALID
(Assignee)

Comment 2

9 months ago
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)

Comment 3

9 months ago
Doesn't the twitter auth page rely on JS?
(Assignee)

Comment 4

9 months ago
> 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.
(Assignee)

Updated

9 months ago
Summary: Restrict content in Twitter's auth browser → Add a pref to restrict content in browser requests
(Assignee)

Comment 5

9 months ago
Created attachment 8816233 [details] [diff] [review]
0001-Bug-1321420-Add-a-pref-to-restrict-content-in-browse.patch
Attachment #8815941 - Attachment is obsolete: true
Attachment #8816233 - Flags: review?(aleth)
(Assignee)

Updated

9 months ago
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---

Comment 6

9 months ago
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?
(Assignee)

Comment 7

9 months ago
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

9 months ago
(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).
(Assignee)

Comment 9

9 months ago
> 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

9 months ago
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 ;)
(Assignee)

Comment 11

9 months ago
Created attachment 8816291 [details] [diff] [review]
0001-Bug-1321420-Add-a-pref-to-disable-JavaScript-in-brow.patch from comment 10
Attachment #8816233 - Attachment is obsolete: true
Attachment #8816233 - Flags: review?(aleth)
Attachment #8816291 - Flags: review?(aleth)

Updated

9 months ago
Attachment #8816291 - Flags: review?(aleth) → review+

Updated

9 months ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/a1ff30b2483661bb2eede5649de61b5501e3063f
Assignee: nobody → arlolra
Status: UNCONFIRMED → RESOLVED
Last Resolved: 9 months ago9 months 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.