1225120 - Move Loop Client CSP policy to <meta> tag

Status: RESOLVED INCOMPLETE

(Hello (Loop) :: Client, defect, P2)

Description

[Adam Roach [:abr]]

Bug 1017257 added CSP to the built-in desktop client through an architecturally inelegant hack that requires the CSP code to be aware of the Loop feature. With the landing of Bug 663570, there is now a clean way to handle this situation: adding a <meta> tag to the Loop client documents.

This bug is to remove all Loop-specific code from nsDocument, and replace it by hardcoding the Loop CSP into Loop's "panel.html" and "conversation.html" files.

Comment 1

[Mark Banner (:standard8)]

This is something that will help reduce the Loop code that's outside of the loop directory, hence reducing code that's split into different places.

I think its worth doing soon because that will help the add-on, hence setting as a lowish P2.

Comment 2

[Mark Banner (:standard8)]

Adam, currently we have debug & release sets of preferences. Is it a concern if we use the debug one for release?

We've allowed at least our dev servers on the standalone side, so I'm wondering if we can do the same here, but with localhost as well?

If we are able to, this would remove the need for two different CSPs (and different processing).

Debug:

pref("loop.CSP", "default-src 'self' about: file: chrome: http://localhost:*; img-src * data:; font-src 'none'; connect-src wss://*.tokbox.com https://*.opentok.com https://*.tokbox.com wss://*.mozilla.com https://*.mozilla.org wss://*.mozaws.net http://localhost:* ws://localhost:*; media-src blob:");

Prod:

pref("loop.CSP", "default-src 'self' about: file: chrome:; img-src * data:; font-src 'none'; connect-src wss://*.tokbox.com https://*.opentok.com https://*.tokbox.com wss://*.mozilla.com https://*.mozilla.org wss://*.mozaws.net; media-src blob:");

Comment 3

[Adam Roach [:abr]]

I think this should be pretty safe -- the only thing you'd be opening up is the ability to connect to localhost. And if there's malicious code being served from localhost, then there's already a much larger exploit at play.

Tagging Richard for a sanity check on my logic here.

Comment 4

[Richard Barnes [:rbarnes]]

I'm not thrilled with opening up localhost access.  It's not that uncommon to have web interfaces for trusted stuff running locally (e.g., CUPS on port 631).  But I think I could live with it, given that you have to get malicious code into the page to start with.

ni=ckerschb in case there are any concerns with <meta> CSP

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Richard Barnes [:rbarnes] from comment #4)
> I'm not thrilled with opening up localhost access.  It's not that uncommon
> to have web interfaces for trusted stuff running locally (e.g., CUPS on port
> 631).  But I think I could live with it, given that you have to get
> malicious code into the page to start with.
> 
> ni=ckerschb in case there are any concerns with <meta> CSP

I suppose you are talking about removing all of [1] and serve the loop CSP through meta. I encourage that effort actually. Looking through the current CSP in comment 2, I don't see any objections that such a CSP could not be served using the meta tag.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#2718

Comment 6

[Mark Banner (:standard8)]

Attachment: WIP Bug 1225120 - Move Loop CSP policy to meta tag.

Initial WIP attempt. Possibly not working.

Comment 7

[Mark Banner (:standard8)]

So I've been trying the patch I've just attached here, and confirmed that the CSP appears to be loading from the meta tag, but it doesn't seem to be being applied.

I've a setup where the loop.server pref is set to localhost. When I've run this before the patch, it would block the loop server if I run in non-debug mode.

With the patch (running in non-debug), the server doesn't get blocked.

I'm wondering if something special here is going on with either about: or chrome: uris?

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Mark Banner (:standard8) from comment #7)
> So I've been trying the patch I've just attached here, and confirmed that
> the CSP appears to be loading from the meta tag, but it doesn't seem to be
> being applied.

As discussed on IRC, the CSP applied should work and it did throughout my experiments but let me summarize my findings:

1) What I've seen we are attaching a meta CSP to:
* about:looppanel
* about:loopconversation
I am not sure if there are other files where ApplyLoopDocument would return true and would apply a CSP where with this patch we are missing to apply a CSP. Probably it's fine, just making sure. Either way, we definitely should have a test, that tests that CSP is applied here.

2) In general, I don't think we need 'self' in the CSP [2] at all, because we also whitelist about:. So since the loopDocument is an about page anyway and we are also whitelisting about:, then no need for 'self'.

3) For further debugging. The web console might log useful CSP messages which potentially can help to figure possible errors early.

Other than that, looks great, looking forward to see more meta CSP usage.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPContext.cpp#344
[2] http://mxr.mozilla.org/mozilla-central/source/browser/extensions/loop/content/preferences/prefs.js#26

Comment 9

[Mark Banner (:standard8)]

Support for Hello/Loop has been discontinued.

https://support.mozilla.org/kb/hello-status

Hence closing the old bugs. Thank you for your support.