Closed Bug 1017257 Opened 6 years ago Closed 5 years ago

Need to add CSP restrictions to Loop client context

Categories

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

defect

Tracking

(firefox34 verified, firefox35 verified)

VERIFIED FIXED
mozilla35
Tracking Status
firefox34 --- verified
firefox35 --- verified

People

(Reporter: abr, Assigned: abr)

References

Details

(Whiteboard: [p=1, 1.5:p1, ft:webrtc, est:1][gecko?][other team][loop-uplift][fig:wontverify])

Attachments

(2 files, 9 obsolete files)

We need to add a restrictive CSP policy for the Loop client context, to prevent third-party libraries from loading in remote javascript or CSS, which could open us up to security exploits.

It would be ideal if we could use a well-tested code path, such as the use of <meta> tags, to implement this. Doing so relies on Bug 663570 landing, however, and the timeframe for that set of changes isn't currently obvious.

An alternative would be emulating the approach used by https://addons.mozilla.org/en-US/firefox/addon/newusercspdesign/

I'm putting this down as a p=1, since it should be pretty straightforward if things "just work" with a null origin; if we run into the same issues as we did with cookies, though, it might take significantly more research.
Sid, can you suggest an appropriate CSP document here.
Flags: needinfo?(sstamm)
Do you want a policy or guidelines for writing a good policy?

https://developer.mozilla.org/en-US/docs/Web/Security/CSP/Using_Content_Security_Policy

A good starter CSP is "default-src 'self'".  What is the list of resource origins you want to trust? What type of thing is a "loop context" (nsDocument or...)?

If you are writing native code in gecko, you can append a CSP to a document's principal whenever you want.  See nsDocument::InitCSP for an example.  Also, FYI we are in the middle of replacing the CSP backend with a more efficient one; this work shouldn't affect how CSP is enforced or interpreted, but we are replacing the default implementation of nsIContentSecurityPolicy with a new one and there will be a bunch of refactoring code churn in CSP.  For more details, see bug 925004 and its dependency graph.
Flags: needinfo?(sstamm)
Whiteboard: [p=1, 1.5:p1, ft:webrtc, est:1] → [p=1, 1.5:p1, ft:webrtc, est:1][gecko?]
Target Milestone: --- → mozilla33
reach out to Song at Tokbox... aim for Aurora
Target Milestone: mozilla33 → mozilla34
Whiteboard: [p=1, 1.5:p1, ft:webrtc, est:1][gecko?] → [p=1, 1.5:p1, ft:webrtc, est:1][gecko?][other team]
I'll reach out to someone on the sec team to see who can implement this for us
Flags: needinfo?(mreavy)
Priority: P1 → P3
Sid -- Is there someone you could recommend to take this?  I know it's not complicated, but my team is out of cycles.  And I think we need it for Fx34.
Flags: needinfo?(mreavy) → needinfo?(sstamm)
The security engineering folk are committed to our Q3 goals and short staffed at the moment, sorry.  If someone writes (1) the CSP policy you want to use and (2) automated tests for expected behavior, I can probably pitch in the implementation or at least help out with it.
Flags: needinfo?(sstamm)
NOTE: Comment 3 has nothing to do with this bug.  It was meant for a different bug.
Regarding the actual CSP policy we want to use, here's my proposal,
rationale, and a list of open issues that we need feedback from the
security team on.

Observations:

- Most of the third party libraries we are using are simple, self-contained
  modules that do not attempt to do anything that might run afoul any CSP
  policies. The one exception is sdk.js, which (by its nature) interacts with
  the network.

- sdk.js does not call eval; however, it does have an OTHelpers.template
  function that is equivalent to calling eval. It is currently unused. We
  should let TB know that using this will cause our security model to break,
  as this is a key security issue we are attempting to address through CSP.

- sdk.js does accept base64 images, which are rendered via "data:". This seems
  safe.

- We do not use frames, and the only places sdk.js uses them should never
  happen in the desktop client (situations like "your browser does not support
  WebRTC")

- sdk.js does attempt to load fonts (from fonts.googleapis.com!), but this
  should not happen in the desktop client (and never will, due to TB having
  a very different l10n policy than Mozilla). Our current implementation does
  not load fonts either, so we will start out with a policy of "none". We can
  relax this to "self" in the future, if our design calls for it. At the moment,
  I want to set the most restrictive policy that still allows things to function.

- sdk.js requires the ability to connect WebSockets to various tokbox-operated
  hosts, all of which reside in the opentok.com domain.


Based on the foregoing, I believe the initial CSP we want is as follows
(however, see the note below about media-src):

> default-src 'self'; img-src: 'self' data:; child-src: 'none'; font-src: 'none'; connect-src: https://*.opentok.com

OPEN ISSUES (NEED FEEDBACK FROM SECURITY TEAM):

1. Our content loads in via about: URIs. It's not clear whether CSP is yet
   equipped to deal with this situation (much of it is focused on hostnames and
   port numbers, both of which are not applicable to 'about:' URIs).

2. We need to be attach  media from peer connections and getUserMedia to
   <video> and <audio> elements. It is not clear which media-src policy allows
   doing so, as this situation does not seem to have been contemplated in the
   CSP specification.

3. It is also not clear what impact CSP has on PeerConnections. The Data
   Channel is equivalent, security-wise, to a WebSockets connection. However,
   applying connect-src restrictions to PeerConnections would completely
   prevent peer-to-peer PeerConnections (which defeats much of their purpose).

4. We don't really need the functionality of report-uri; however, we should be
   able to detect and report violations during testing of new libraries. Some
   hook to provide similar functionality to a local function (or even just by
   logging to console) would be of great assistance.

I'm setting needinfo to Sid for feedback on this list of four open issues. Sid: feel free to lateral the information request to anyone else who could help clear these up for us. Thanks!
Flags: needinfo?(sstamm)
1) you may need to specify "about:" in your policy. That's as fine-grained as it gets, unfortunately. (actually, you may be able to specify entire about: urls to enumerate your specific ones).

2) I don't know. It's possible that functionality bypasses the CSP checks since you're hooking up a stream rather than loading a URI. It sounds equivalent to other 'self'-like items, though, such as blob: and data:. Maybe we need to propose a 'stream' keyword.

3) CSP doesn't know about PeerConnections and shouldn't interfere with them at this time. There's an open item in the working group for the future, but probably the best we can do is either allow or disallow them since you generally don't know what address you're going to connect to ahead of time.

4) the spec includes error events fired at the page for violations. I don't think we have implemented that but if you're going to write code for it do that.

Leaving needinfo? for Sid to sanity check my answers since I've not had a chance to look at the CSP code since it's been rewritten and some "level 2" features added.
(In reply to Daniel Veditz [:dveditz] from comment #9)
> 1) you may need to specify "about:" in your policy. That's as fine-grained
> as it gets, unfortunately. (actually, you may be able to specify entire
> about: urls to enumerate your specific ones).

Would not hurt to add "about:" to the policy, but right now everything from about: is allowed everywhere.  That may change in the future.
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPService.cpp#84

> 3) CSP doesn't know about PeerConnections and shouldn't interfere with them
> at this time. There's an open item in the working group for the future, but
> probably the best we can do is either allow or disallow them since you
> generally don't know what address you're going to connect to ahead of time.

That's kind of what I'm thinking too.  It could *just work*, but flaggin n-i?mt, he's been thinking about p2p connections and CSP.  Martin: do you have any thoughts on this?

> 4) the spec includes error events fired at the page for violations. I don't
> think we have implemented that but if you're going to write code for it do
> that.

Yep, it's on the docket, but it is not high-priority.  You can monitor the web console during manual or automated testing, but there's no clean API until the thing in the spec.
Flags: needinfo?(martin.thomson)
(In reply to Adam Roach [:abr] from comment #8)
> - sdk.js does not call eval; however, it does have an OTHelpers.template
>   function that is equivalent to calling eval. It is currently unused. We
>   should let TB know that using this will cause our security model to break,
>   as this is a key security issue we are attempting to address through CSP.

Sounds right.  Arbitrary Strings -> Executing Code = dangerous.
 
> default-src 'self'; img-src: 'self' data:; child-src: 'none'; 
>    font-src: 'none'; connect-src: https://*.opentok.com

You'll want to remove colons from the various directive names.  (e.g., "font-src:").  I think this is a fine policy if you want to block all font loading and prohibit subframes, which it sounds like you want.
Flags: needinfo?(sstamm)
The CSP tweak that will likely be necessary here - if the proposal I made gets implemented - is '...; connect-src https://*.opentok.com webrtc-data; ...'.

The trick being that the rule you have would prohibit the use of data channels, which are a potential source of script.

That feature is unlikely to make CSP level 2, but I don't know how you want to track that to avoid breakage later.  The connection between this code and CSP changes isn't strong, so unless you have a test that verifies that the CSP policy in use is effective, some way of tracking it might be wise.  Either that or you could speculatively modify the directive.
Flags: needinfo?(martin.thomson)
Taking Martin's proposed datachannel name speculatively, fixing the syntax error Sid pointed out, and taking Daniel's suggestion to future-proof about: checking; I believe the policy we'll be using is:

default-src 'self' about:; img-src 'self' data: about:; child-src 'none'; font-src 'none'; connect-src: https://*.opentok.com 'webrtc-data'
In terms of getting this implemented, I've poked at this a little bit. Some thoughts about three different potential approaches:

1. I'll reiterate that using <meta> tags would probably be ideal here, but this makes us wait for  	Bug 663570.

2. Except for one minor (heh) hitch, implementing this functionality might be as easy as adding the following to the end of injectLoopAPI in MozLoopAPI.jsm:

> let csp = Cc["@mozilla.org/cspcontext;1"].
>           createInstance(Components.interfaces.nsIContentSecurityPolicy);
> 
> csp.appendPolicy("default-src 'self' about:; img-src 'self' data: about:; " +
>                  "child-src 'none'; font-src 'none'; connect-src: " +
>                  "https://*.opentok.com 'webrtc-data'", false);
> 
> targetWindow.document.nodePrincipal.csp = csp;

The fly in this ointment is that nsIPrincipal.csp is marked [noscript], so the final step here is impossible. I'm not sufficiently familiar with IDL to know whether there's some kind of workaround we can apply to address this problem for privileged JavaScript code.

3. Alternately, we could consider an approach that generically allows defining a CSP to go with "about:" URLs. This would look something like:

  a. In netwerk/base/public/nsIChannel.idl:
     - add new CSP string attribute

  b. In browser/components/about/AboutRedirector.cpp:
     - Add "const char *CSP;" field to RedirEntry
     - In kRedirMap, set CSP for looppanel to desired policy (see
       comment 13)
     - in AboutRedirector::NewChannel(), set CSP for tempchanel to
       kRedirMap[i].CSP (using the new CSP attribute from (a), above)

  c. In source/content/base/src/nsDocument.cpp:
     - Add logic to InitCSP to check aChannel for non-null CSP string
     - If non-null, csp->AppendPolicy(cspString, false); and then apply
       csp to the principal.
Assignee: nobody → adam
It's probably worth noting that approaches 1 and 2 from Comment 14 probably won't set eval permissions correctly due to the caching introduced in Bug 614137. That certainly points towards something shaped kind of like #3, at least in the short term.
abr: is there a specific about: URI that all Loop sessions come from or a way in nsDocument to identify that it is a loop document without having to shove a CSP into the channel?  

What I'm getting at is maybe adding a branch in nsDocument::InitCSP for "about:loop" or something like what we do for default App CSPs... this is much like what you said in 3, but without the modifications to the channels.
Flags: needinfo?(adam)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #16)
> abr: is there a specific about: URI that all Loop sessions come from or a
> way in nsDocument to identify that it is a loop document without having to
> shove a CSP into the channel?  
> 
> What I'm getting at is maybe adding a branch in nsDocument::InitCSP for
> "about:loop" or something like what we do for default App CSPs... this is
> much like what you said in 3, but without the modifications to the channels.

I thought about doing something like that, but it seems inelegant to hardcode the loop URIs into InitCSP. It certainly is less code, though, so I see the appeal.

Presently, we have only two "about:" URIs that we load from (about:loopconversation and about:looppanel). That's manageable. I'll see about getting a POC patch up based on this approach shortly.
Flags: needinfo?(adam)
Actually reading back through this, I'd be interested in getting Adam's take on media-src directives and WebRTC, re: necessity/value.  Our initial conclusion was that this wasn't that interesting.
Attached patch WIP: Attach Loop CSP policy (obsolete) — Splinter Review
I would think something like this should work; but it doesn't appear to cause any restrictions to be imposed. (When I tweak the loop.CSP property to set connect-src to 'none', the wss connections to TB's server still come up). I'll have to troubleshoot it later, but wanted to put it up here to verify that, structurally, it's what Sid had in mind.
abr: that looks like what I had in mind, yes.  It's possible the connections to TB's server are not being enforced if there are bypasses in CSP when the document is "about:".  Can you check with nspr logging and "PR_LOG_MODULES=csp:5" to see what's happening (or if anything at all is being checked through the CSP)?

Another option we could use is flags on the document kind of like we do with apps for appstatus...
(In reply to Martin Thomson [:mt] from comment #18)
> Actually reading back through this, I'd be interested in getting Adam's take
> on media-src directives and WebRTC, re: necessity/value.  Our initial
> conclusion was that this wasn't that interesting.

Tricky. The purported reason to have CSP is to prevent injection attacks, but restrictions on things without the ability to execute scripts like fonts, images[1], and media don't really serve that purpose (unless there's some security exploit in our handling of those items).

I guess it comes down to how we see people using things like restrictions on video tags. If the idea is to prevent pulling in media from locations that we don't want them from, then allowing peerconnection-sourced media is an unmitigated end-run around every remote restriction that could possibly be specified. From that perspective, it seems to make sense that we would at least want to have an explicit opt-in for PeerConnection-sourced media when CSP is in effect. The story for gUM is different, since there's no way to abuse it to stream media from an otherwise unapproved remote source. However, for sake of consistency, we probably want to go ahead and allow CSP to restrict its use as well -- probably as a separate token.

Which is to say that I think we *probably* want two new possible values that can be used in media-src: one for PC, and a different one for gUM.
____
[1] Except, perhaps SVG. I haven't kept up on our what we do with <script> tags in SVG, though, so this may or may not be an issue.
That seems like sound logic to me.  The end-run thing bothered me a little too.  I'll forward this on to the webappsec group and let them ponder the implications.

From an implementation perspective, we'd need custom code for this at both entry points.  Good thing that MediaStream/Track isn't Transferable or we would have a ton of hurt on our hands.
(In reply to Martin Thomson [:mt] from comment #22)

> From an implementation perspective, we'd need custom code for this at both
> entry points.  Good thing that MediaStream/Track isn't Transferable or we
> would have a ton of hurt on our hands.

We have discussed making MediaStream/Track possible to send using postMessage (but structurally cloned rather than transfered). I'm quite ignorant in this area - would cloning also give us "a ton of hurt on our hands"? And is that ton implementation specific or general?
(In reply to steppohak from comment #24)
> We have discussed making MediaStream/Track possible to send using
> postMessage (but structurally cloned rather than transfered). I'm quite
> ignorant in this area - would cloning also give us "a ton of hurt on our
> hands"? And is that ton implementation specific or general?

The general point is that there are lots of ways for streams/tracks to get shunted around once they are available to script.  So guarding the entry points (RTCPeerConnection, getUserMedia) is easy.  If we talk about cloning or transferring tracks, that could be regarded as another entry point, but the problem there is that you would need to trace the origin of the media back to be able to make the right authorization choice for any given stream.  I don't know if the general problem has been solved, but media principals aren't stable in the way that other resources are.

Incidentally, transfer for MediaStream/Track isn't high on our priority list right now, see bug 976814.
Ah, I see why it's not working: by the time we get to CSP loading, the about redirection has already taken place -- the URI I'm comparing against is no longer "about:loopconversation" -- it's "file:///.../browser/components/loop/content/conversation.html"

It would be really ugly to have to check for that kind of thing, so I'm going to look at the alternate approach Sid mentions of attaching information to the document principal.
Attached patch POC: Attach Loop CSP policy (obsolete) — Splinter Review
I spent a long time chasing window instantiation around the tree, but couldn't
quite figure out an easy way to get additional information attached to the
principal.

But I did discover that the original URI is preserved in the channel, so this
patch uses that and does appear to sucessfully apply CSP.

And then there's a hitch:

  JavaScript error: chrome://browser/content/loop/shared/libs/lodash-2.4.1.js,
  line 6305: call to Function() blocked by CSP

Which is the kind of thing we're trying to prevent from happening with the CSP.
So we need to figure out how we want to handle lodash.
Attachment #8484563 - Attachment is obsolete: true
Attachment #8486459 - Attachment is obsolete: true
Attached patch Attach Loop CSP policy (obsolete) — Splinter Review
Attachment #8486553 - Attachment is obsolete: true
Attachment #8486558 - Attachment description: Attach Loop CSP policy, fix violations of desired policy → Attach Loop CSP policy
Comment on attachment 8486559 [details] [diff] [review]
Make loop code CSP-friendly (remove all inline script)

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

Mark:

After activating CSP on Loop, I ran into a number of places where we were doing inline scripts, which runs afoul the CSP restrictions we want to have in place. This patch moves the uses of lodash's "template" function to use precompiled templates (which will be faster anyway), and makes a few tweaks to move inlined javascript (including things like onload="javascript here") into external files. I think these changes are all pretty straightforward, but ping me if you have any questions.
Attachment #8486559 - Flags: review?(standard8)
Comment on attachment 8486558 [details] [diff] [review]
Attach Loop CSP policy

Sid -- not sure if I should mark this "f?" or "r?". It's functionally complete, and seems to serve the purpose just fine. If you'd like to see something else, let me know. If you think is is ready to go, feel free to r+ it. :)
Attachment #8486558 - Flags: review?(sstamm)
Priority: P3 → P1
Whiteboard: [p=1, 1.5:p1, ft:webrtc, est:1][gecko?][other team] → [p=1, 1.5:p1, ft:webrtc, est:1][gecko?][other team][loop-uplift]
Target Milestone: mozilla34 → mozilla35
Depends on: 1065608
Comment on attachment 8486559 [details] [diff] [review]
Make loop code CSP-friendly (remove all inline script)

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

I took a look at this, and realised we should just port those last few, simple, views from backbone to react, as then we wouldn't need the template functionality at all. They were also in the wrong place, being in shared, when they are conversation window specific. Hence, I've filed bug 1065608 for that and stuck up a patch.

The rest of the changes look fine to me, I just didn't think it was work landing the template stuff just to switch away from needing it.
Attachment #8486559 - Flags: review?(standard8) → feedback+
Comment on attachment 8486558 [details] [diff] [review]
Attach Loop CSP policy

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

So this is probably okay, but introduces extra URI allocations and comparisons for all documents (not just loop documents).  Is there a way during loop document construction (before nsDocument::Init()) to tag this document as a loop doc so that non-loop docs won't have to do the URI creation/comparison dance you do here?  It would be faster to store a bool in nsDocument that is set on construction based on whether this is a loop doc or not.

Please check into this and re-flag me for review once you've exhausted this possibility (and fixed my minor comments below).

::: browser/app/profile/firefox.js
@@ +1612,5 @@
>  pref("loop.retry_delay.limit", 300000);
>  pref("loop.feedback.baseUrl", "https://input.mozilla.org/api/v1/feedback");
>  pref("loop.feedback.product", "Loop");
>  pref("loop.debug.websocket", false);
> +pref("loop.CSP", "default-src 'self' about: file: chrome:; img-src 'self' data: about: file: chrome:; font-src 'none'; connect-src wss://*.tokbox.com https://*.opentok.com https://*.tokbox.com wss://*.mozilla.com wss://*.mozaws.net 'webrtc-data'");

Is this a firefox-specific feature or will it end up in FxOS and other gecko things?  If others, consider adding the pref to modules/libpref/init/all.js instead.

::: content/base/src/nsDocument.cpp
@@ +2747,5 @@
> +      rv = chanURI->EqualsExceptRef(loopURI, &applyLoopCSP);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +    }
> +  }
> +

I think this is fine, but it would be ever so slightly nicer to have this encapsulated in a "IsLoopDocument() function on the nsDocument.  That way if we can figure out a much better way to mark/identify a document as part of loop, we can swap out these URI scans for something faster and more precise.

@@ +2756,4 @@
>        cspHeaderValue.IsEmpty() &&
>        cspROHeaderValue.IsEmpty()) {
>  #ifdef PR_LOGGING
> +    rv = aChannel->GetURI(getter_AddRefs(chanURI));

If you assign rv here, please also check for failure.  Since this is for PR_LOGGING, I'd just drop the assignment and do a null check before using chanURI below to avoid a different control flow in debug builds.

@@ +2836,5 @@
> +  // ----- if the doc is part of Loop, apply the loop CSP
> +  if (applyLoopCSP) {
> +    nsAdoptingString loopCSP;
> +    loopCSP = Preferences::GetString("loop.CSP");
> +    NS_ASSERTION(loopCSP, "Missing loop.CSP preference");

What is the right thing to do here in opt builds?  If it's a loop document and we can't find the CSP, should we stop trying to load loop?  Do you want to fall back to weak security?
Attachment #8486558 - Flags: review?(sstamm)
Now without the templating stuff
Attachment #8486559 - Attachment is obsolete: true
Comment on attachment 8488207 [details] [diff] [review]
Make loop code CSP-friendly (remove all inline script)

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

Yep, this is a much smaller set of changes now that you've removed the templates. Thanks!
Attachment #8488207 - Flags: review?(standard8)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #34)
> Comment on attachment 8486558 [details] [diff] [review]
> Attach Loop CSP policy
> 
> Review of attachment 8486558 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So this is probably okay, but introduces extra URI allocations and
> comparisons for all documents (not just loop documents).

Yeah, that's pretty ugly.

> Is there a way
> during loop document construction (before nsDocument::Init()) to tag this
> document as a loop doc so that non-loop docs won't have to do the URI
> creation/comparison dance you do here?  It would be faster to store a bool
> in nsDocument that is set on construction based on whether this is a loop
> doc or not.
> 
> Please check into this and re-flag me for review once you've exhausted this
> possibility (and fixed my minor comments below).

I'm still looking into this. I thought I'd hit a dead end, but after more poking at XBL and webidl, I think there might be a path here.
 
> ::: browser/app/profile/firefox.js
> @@ +1612,5 @@
> >  pref("loop.retry_delay.limit", 300000);
> >  pref("loop.feedback.baseUrl", "https://input.mozilla.org/api/v1/feedback");
> >  pref("loop.feedback.product", "Loop");
> >  pref("loop.debug.websocket", false);
> > +pref("loop.CSP", "default-src 'self' about: file: chrome:; img-src 'self' data: about: file: chrome:; font-src 'none'; connect-src wss://*.tokbox.com https://*.opentok.com https://*.tokbox.com wss://*.mozilla.com wss://*.mozaws.net 'webrtc-data'");
> 
> Is this a firefox-specific feature or will it end up in FxOS and other gecko
> things?  If others, consider adding the pref to modules/libpref/init/all.js
> instead.

This is definitely not part of FFxOS, where it will be a separate app rather than integrated with gecko.
 
> ::: content/base/src/nsDocument.cpp
> @@ +2747,5 @@
> > +      rv = chanURI->EqualsExceptRef(loopURI, &applyLoopCSP);
> > +      NS_ENSURE_SUCCESS(rv, rv);
> > +    }
> > +  }
> > +
> 
> I think this is fine, but it would be ever so slightly nicer to have this
> encapsulated in a "IsLoopDocument() function on the nsDocument.  That way if
> we can figure out a much better way to mark/identify a document as part of
> loop, we can swap out these URI scans for something faster and more precise.

Sounds reasonable.
 
> @@ +2756,4 @@
> >        cspHeaderValue.IsEmpty() &&
> >        cspROHeaderValue.IsEmpty()) {
> >  #ifdef PR_LOGGING
> > +    rv = aChannel->GetURI(getter_AddRefs(chanURI));
> 
> If you assign rv here, please also check for failure.  Since this is for
> PR_LOGGING, I'd just drop the assignment and do a null check before using
> chanURI below to avoid a different control flow in debug builds.

Sorry; cut-and-paste. I'm going to return this to its previous behavior, which was to discard the result from GetURI.

> @@ +2836,5 @@
> > +  // ----- if the doc is part of Loop, apply the loop CSP
> > +  if (applyLoopCSP) {
> > +    nsAdoptingString loopCSP;
> > +    loopCSP = Preferences::GetString("loop.CSP");
> > +    NS_ASSERTION(loopCSP, "Missing loop.CSP preference");
> 
> What is the right thing to do here in opt builds?  If it's a loop document
> and we can't find the CSP, should we stop trying to load loop?  Do you want
> to fall back to weak security?

I think we fall back to having no CSP in that case. When this is working, there's no way to load in code other than what ships with the browser. So, as long as this is active in our testing, running with it disabled for some small fraction of our users doesn't really pose a threat. What we're really guarding against here is a situation where we update one of our third-party libraries and discover that the new version is doing something unexpected. As long as we catch this before it rolls out on the train, we'll be good.
Attached patch Attach Loop CSP policy (obsolete) — Splinter Review
Sid:

This patch shows what I had in mind: the "isLoop" attribute on document (of
course, I would make isLoop in socialchat.xml predicated on the value of a
parameter), but it's not working. If I set the this.contentDocument.isLoop
flag directly in the constructor, then it's setting it on some document other
than the one that eventually gets loaded (and I'm not familiar enough with
this process to track down why in a sane amount of time). If I instead wait
for a readystatechange to "loading", it's too late: the CSP setup code has
already fired. So I think the short answer is that there's not a clean way to
get a flag on the document. Let me know if you think I'm overlooking a
possibility here.

(Ignore the fact that the current URL-based detection code is still in there;
I was using that as a convenient place to hang debugger breakpoints)

I'm pretty sure that I *do* have a clean shot at the channel in
AboutRedirector (as I discuss in comment 14), so we need to pick our poision:
either we hang a flag off the channel and do a simple boolean check at
document load, or we leave channel alone, and do this little dance with extra
URL construction. Which do you think is preferable?
Attachment #8486558 - Attachment is obsolete: true
Flags: needinfo?(sstamm)
Comment on attachment 8488294 [details] [diff] [review]
Attach Loop CSP policy

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

::: content/base/src/nsDocument.cpp
@@ +2801,5 @@
> +  }
> +
> +  if (applyLoopCSP) {
> +    PR_LOG(gCspPRLog, PR_LOG_DEBUG, ("This is loop."));
> +  }

Ignore everything from line 2799 to here -- it was just in there to help figure out what was going on in the debugger.
Attachment #8488207 - Flags: review?(standard8) → review+
Mark -- I ran the mocha tests before landing this as a double-check, and it
turns out that they get unhappy if the conversation and panel objects have
been initialized. This is a minor tweak to suppress calling init() in the
mocha tests.
Attachment #8488630 - Flags: review?(standard8)
Attachment #8488207 - Attachment is obsolete: true
Attached patch Attach Loop CSP policy (obsolete) — Splinter Review
Attachment #8488294 - Attachment is obsolete: true
Flags: needinfo?(sstamm)
Comment on attachment 8488798 [details] [diff] [review]
Attach Loop CSP policy

Sid: Here's a proposed "minimal impact" approach that uses the nsIFileChannel
to keep track of whether the Loop CSP should be applied. I'll note that this
is at least vaguely congruent with HTTP, where CSP-related inforation is
retreived from the nsIHttpChannel object. I've also moved the check for
loopyness off into its own function, as you suggested.

Given the challenges involved in trying to set this directly on the document,
I believe this is a good compromise that provides good performance.
Attachment #8488798 - Flags: review?(sstamm)
Comment on attachment 8488630 [details] [diff] [review]
Make loop code CSP-friendly (remove all inline script)

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

::: browser/components/loop/test/desktop-local/index.html
@@ +49,5 @@
>    <script src="conversation_test.js"></script>
>    <script src="panel_test.js"></script>
>    <script>
> +    document.removeEventListener('DOMContentLoaded', loop.panel.init);
> +    document.removeEventListener('DOMContentLoaded', loop.conversation.init);

I think it would be useful to add a comment here along the lines of "Stop the default init functions running to avoid conflicts in tests".
Attachment #8488630 - Flags: review?(standard8) → review+
Keywords: leave-open
Comment on attachment 8488798 [details] [diff] [review]
Attach Loop CSP policy

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

Yeah, I like this very much.  You should get review on the browser/components and netwerk/ changes too... flagging mcmanus and ttaubert here for nsFileChannel and AboutRedirector changes.

r=me on the CSP bits (with or without the IsLoopDocument function in nsDocument).

::: content/base/src/nsDocument.cpp
@@ +2735,5 @@
> +    fileChannel->GetIsLoop(&isLoop);
> +  }
> +  return isLoop;
> +}
> +

I think you should skip adding this method to nsDocument and inline its body in InitCSP(), which is the only consumer of IsLoopDocument.  Unless, of course, you expect to use this check somewhere else too...
Attachment #8488798 - Flags: review?(ttaubert)
Attachment #8488798 - Flags: review?(sstamm)
Attachment #8488798 - Flags: review?(mcmanus)
Attachment #8488798 - Flags: review+
Comment on attachment 8488798 [details] [diff] [review]
Attach Loop CSP policy

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

::: netwerk/protocol/file/nsFileChannel.cpp
@@ +241,5 @@
>  
>  //-----------------------------------------------------------------------------
>  
> +nsFileChannel::nsFileChannel(nsIURI *uri) :
> +  mIsLoop(false)

Not my realm but adding this to nsIFileChannel looks really wrong. This is a generic interface and we should look for another solution to apply CSP in the document.

We should at least add a CSP string to nsIFileChannel that nsDocument could then read - the nsIFileChannel shouldn't know about Loop. But it actually shouldn't even know about CSP.

Don't have a great idea right now, maybe someone else more familiar with the code.
Attachment #8488798 - Flags: review?(ttaubert) → review-
Tim: I flagged you for your thoughts mainly on aboutredirector stuff since you were all over the hg blame for those files.  You didn't comment on that part; does abr's approach look like it's going in the right direction (if we can figure out the channel thing)?  Maybe we can hook that directly into a flag on the document?
Flags: needinfo?(ttaubert)
Well that part is very dependent on what the final solution will look like. If we keep going that direction then we should probably implement it like the |idbOriginPostfix| instead of having something that works only for Loop. Storing the CSP rules in a pref complicates the situation somewhat though because we'll probably not want to specify a pref name but the plain CSP rules in the RedirEntry.
Flags: needinfo?(ttaubert)
Comment on attachment 8488798 [details] [diff] [review]
Attach Loop CSP policy

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

I'm not up to date on loop, but tim's comment makes sense to me..

::: netwerk/protocol/file/nsIFileChannel.idl
@@ +9,5 @@
>  
>  /**
>   * nsIFileChannel
>   */
>  [scriptable, uuid(06169120-136d-45a5-b535-498f1f755ab7)]

uuid change
Attachment #8488798 - Flags: review?(mcmanus)
(In reply to Tim Taubert [:ttaubert] from comment #49)
> Well that part is very dependent on what the final solution will look like.
> If we keep going that direction then we should probably implement it like
> the |idbOriginPostfix| instead of having something that works only for Loop.

The issue with that approach is that it runs into the issue that Sid raised earlier: it imposes the weight of constructing an additional URL in the path of every page load (see the first couple of lines of Comment 34). He didn't seem to like that. If we're willing to take that hit, then I think the solution of just checking for Loop URLs directly inside the CSP application method is less intrusive and perfectly adequate.

If, however, we want to take Sid's recommendation that we don't instantiate new URLs in this code path, then copying the |idbOriginPostfix| approach isn't viable.

> Storing the CSP rules in a pref complicates the situation somewhat though
> because we'll probably not want to specify a pref name but the plain CSP
> rules in the RedirEntry.

If push comes to shove, we can hardcode this into the redirector. Leaving it in a pref makes debugging CSP-related issues a whole lot easier, though, so I'd like to preserve this property if possible. Once we settle on an overall data flow, I'm happy to figure out whether it's compatible with putting the policy in a pref. If not, I don't think it's a showstopper.

If Tim and Patrick are okay with an approach of simply attaching a CSP to the nsIFileChannel (which is basically approach 3 from Comment 14), I'm happy to respin the patch to make that happen.

ni? to Tim and Patrick for their opinion.
Flags: needinfo?(ttaubert)
Flags: needinfo?(mcmanus)
Can someone please advise the testing approach for this bug? I've been asked to make sure this gets the appropriate coverage against the fig branch.
Flags: needinfo?(ttaubert) → qe-verify+
QA Contact: anthony.s.hughes
Sorry, I inadvertently removed Adam's needinfo to Tim re comment 51.
Flags: needinfo?(ttaubert)
I'm re-tagging Tim so he appears in my list of outstanding requests.

Once this lands, you'd test it by running through a call and making sure you don't get any CSP errors on the Javascript console. Warnings would be an indication that something that previously worked isn't working any more. Ideally, this check should be made any time any of our third-party libraries are updated.
Flags: needinfo?(ttaubert)
Flags: needinfo?(ttaubert)
(In reply to Adam Roach [:abr] from comment #51)

> The issue with that approach is that it runs into the issue that Sid raised
> earlier: it imposes the weight of constructing an additional URL in the path
> of every page load (see the first couple of lines of Comment 34). He didn't
> seem to like that. If we're willing to take that hit, then I think the
> solution of just checking for Loop URLs directly inside the CSP application
> method is less intrusive and perfectly adequate.

the patch in comment 34 is my preference here.

all I see there in the non-about common case is an addref and a missed strcmp on the scheme check. That seems pretty tame compared to all that happens on a page load - if talos tolerates it I think it should be fine.. and the patch is much more focused that way, don't you think?

that being said I can actually live with either approach if need be.
Flags: needinfo?(mcmanus)
(In reply to Patrick McManus [:mcmanus] from comment #55)
> the patch in comment 34 is my preference here.
> 
> all I see there in the non-about common case is an addref and a missed
> strcmp on the scheme check. That seems pretty tame compared to all that
> happens on a page load - if talos tolerates it I think it should be fine..
> and the patch is much more focused that way, don't you think?

After much debate, I'm inclined to agree.  The alternative seems a bit hairier than I expected; sorry for bouncing you around, abr.
Attached patch Attach Loop CSP Policy (obsolete) — Splinter Review
Attachment #8488798 - Attachment is obsolete: true
Comment on attachment 8497624 [details] [diff] [review]
Attach Loop CSP Policy

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

I believe this patch reflects our agreed-upon approach. I've tweaked the CSP
policy itself to eliminate a console warning, and to accommodate Bug 1069962.

I'm taking Comment 34 Sid's advice [1] in preference to Comment 46 Sid's
advice [2] because it makes more sense to me.

____
[1] "It would be ever so slightly nicer to have this encapsulated in a
    IsLoopDocument() function on the nsDocument."

[2] "I think you should skip adding this method to nsDocument and inline its
    body in InitCSP(), which is the only consumer of IsLoopDocument."
Attachment #8497624 - Flags: review?(sstamm)
Flags: needinfo?(ttaubert)
(In reply to Patrick McManus [:mcmanus] from comment #55)
> if talos tolerates it I think it should be fine..

Let's check...

https://tbpl.mozilla.org/?tree=Try&rev=9d7ccd993742
Comment on attachment 8497624 [details] [diff] [review]
Attach Loop CSP Policy

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

looks good to me.
Attachment #8497624 - Flags: review?(sstamm) → review+
This bug will not be fixed in time for QE to verify in Fig pre-uplift so I'm tagging it as such. It's still marked to track for testing once it lands/uplifts.
Whiteboard: [p=1, 1.5:p1, ft:webrtc, est:1][gecko?][other team][loop-uplift] → [p=1, 1.5:p1, ft:webrtc, est:1][gecko?][other team][loop-uplift][fig:wontverify]
Comment on attachment 8488630 [details] [diff] [review]
Make loop code CSP-friendly (remove all inline script)

Approval Request Comment
Uplift request for patches staged and tested on Fig
Attachment #8488630 - Flags: approval-mozilla-aurora?
Comment on attachment 8488630 [details] [diff] [review]
Make loop code CSP-friendly (remove all inline script)

I worked with Randell and Maire on uplifting a large number of Loop bugs at once. All of the bugs have been staged on Fig and tested by QE before uplift to Aurora. As well, all of the bugs are isolated to the Loop client. Randell handled the uplift with my approval. I am adding approval to the bug after the fact for bookkeeping.
Attachment #8488630 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
After discussing with dmose, we'd like the setup for development to be lower-friction, so I'm going to make a small tweak to firefox.js when we're running a debug build.
Keywords: leave-open
Dan: The only unreviewed changes in this patch are the bits of
firefox.js and browser_devices_get_user_media_about_urls.js.
It's about eight lines in total.

Because of the surprise on the last landing, I've pushed this patch
out to try:

  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d08bc6758913
  https://tbpl.mozilla.org/?tree=Try&rev=d08bc6758913
Attachment #8501244 - Flags: review?(dmose)
Attachment #8497624 - Attachment is obsolete: true
Comment on attachment 8501244 [details] [diff] [review]
Attach Loop CSP Policy

r=dmose, conditional on filing a bug suggesting that we need to at least modify browser/components/loop/run-all-tests.sh if not just move the test.

I'll owe you a beer if just fix the shell script as part of this patch.  :-)
Attachment #8501244 - Flags: review?(dmose) → review+
(In reply to Dan Mosedale (:dmose) - not reading bugmail; needinfo? for response from comment #69)
> I'll owe you a beer if just fix the shell script as part of this patch.  :-)

Challenge accepted.

https://hg.mozilla.org/integration/fx-team/rev/547ad4574166
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/547ad4574166
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
abr: rock on!  Please remind me of the beer owed in PDX!
(In reply to Adam Roach [:abr] from comment #54)
> Once this lands, you'd test it by running through a call and making sure you
> don't get any CSP errors on the Javascript console. Warnings would be an
> indication that something that previously worked isn't working any more.
> Ideally, this check should be made any time any of our third-party libraries
> are updated.
No CSP errors/warnings in the browser console.
Verified fixed FF 35.0a1 (2014-10-09) Win 7, Ubuntu 13.04, OS X 10.9.5.
Status: RESOLVED → VERIFIED
Comment on attachment 8501244 [details] [diff] [review]
Attach Loop CSP Policy

Approval Request Comment
Part of the staged Loop aurora second uplift set
Attachment #8501244 - Flags: approval-mozilla-aurora?
Paul, this has been uplifted to Aurora. Can you please test this to confirm it's working as expected?
Flags: needinfo?(paul.silaghi)
QA Contact: anthony.s.hughes → paul.silaghi
(In reply to Paul Silaghi, QA [:pauly] from comment #73)
> No CSP errors/warnings in the browser console.
> Verified fixed FF 35.0a1 (2014-10-09) Win 7, Ubuntu 13.04, OS X 10.9.5.
Verified fixed FF 34b1 OS X 10.9.5
Flags: needinfo?(paul.silaghi)
Comment on attachment 8501244 [details] [diff] [review]
Attach Loop CSP Policy

Approval previously granted to jesup to land this change as part of the second batch of Loop fixes to land on Aurora. Marking the approval after the fact.
Attachment #8501244 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Is there test coverage to make sure Loop CSP is being respected?
Flags: in-testsuite?
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #79)
> Is there test coverage to make sure Loop CSP is being respected?

Not presently. This would be a really good thing to add.
(In reply to Adam Roach [:abr] from comment #80)
> (In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #79)
> > Is there test coverage to make sure Loop CSP is being respected?
> 
> Not presently. This would be a really good thing to add.

Do you think that's something we can add to one of the Engineering suites?
You need to log in before you can comment on or make changes to this bug.