612391 - CSP should use an HTTP channel instead of XHR for reporting violations

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

Currently, visiting a site that has a CSP violation such as AMO causes an assertion:
###!!! ASSERTION: This is unsafe! Fix the caller!: 'Error', file /home/mrbkap/work/main/mozilla/content/events/src/nsEventDispatcher.cpp, line 514

This is because CSP uses XHRs at a time when it isn't safe to run scripts. The XHR itself is async (so we're not spinning the event loop here) but it wants to fire the onreadystatechange event, and that does assert. It seems like we could use an HTTP channel instead of an XHR to get rid of the assertion (and be more safe in the process).

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Or could the XHR be created asynchronously?

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch

Rewrote the reporting mechanism to use a new channel instead of XHR.  I also tweaked the mochitests since they were intermittently causing "unsafe to fire mutation events" upon test completions.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 490761 [details] [diff] [review]
proposed patch

>+          if (!chan || !(chan instanceof Ci.nsIHttpChannel)) {
>+            CSPdebug("Error creating channel for " + uris[i]);
>+            continue;
>+          }

Instead of requiring a HTTP channel here, I'd say just do something like:

try {
  chan.QueryInterface(Ci.nsIHttpChannel)
  chan.requestMethod = "POST";
}
catch (ex) {}

Note that loadFlags live on nsIRequest, so no QI needed to get to those.


>diff -r 1950375b2ec2 content/base/test/test_CSP.html

>-  window.examiner.remove();
>-  SimpleTest.finish();
>+  // (soon -- defer to prevent reflow issues)
>+  setTimeout(function () {
>+    window.examiner.remove();
>+    SimpleTest.finish();
>+  }, 0);

use

SimpleTest.executeSoon(function () {
  ...
});

instead of setTimeout. Same thing elsewhere.

Also would be nice to see a test that makes sure that a POST is sent to the server.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 490761 [details] [diff] [review]
proposed patch

>+CSPErrorReportListener.prototype = {
>+  _reportURI:   null,
>+
>+  QueryInterface: function(iid) {
>+    if(iid.equals(Ci.nsIStreamListener) ||
>+        iid.equals(Ci.nsIRequestObserver) ||
>+        iid.equals(Ci.nsISupports))
>+      return this;
>+    throw Components.results.NS_ERROR_NO_INTERFACE;
>+  },

How about

XPCOMUtils.generateQI([Ci.nsIStreamListener, Ci.nsIRequestObserver]),

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

(In reply to comment #4)
> How about
> 
> XPCOMUtils.generateQI([Ci.nsIStreamListener, Ci.nsIRequestObserver]),

CSPUtils.jsm doesn't import XPCOMUtils.jsm, so I don't think that'll work.

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch

Here's a new rev of the patch with changes incorporated -- and some shiny new xpcshell tests that check that reports are sent on violation.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 491409 [details] [diff] [review]
proposed patch

>+          var chan = ios.newChannel(uris[i], null, null);
>+          if (!chan || !(chan instanceof Ci.nsIHttpChannel)) {

You don't need the nsIHttpChannel check here.

r=me with that removed

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

Attachment: fix

Removed the nsIHttpChannel check as requested.

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

Attachment 492822 [details] [diff] (as uploaded) depends on bug 552523 to land.  It can be re-written to work without 552523, but as reviewed and attached it does.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 492822 [details] [diff] [review]
fix

Please renominate for approval once the dependency gets in.

Comment 11

[(no longer active)]

This doesn't apply cleanly any more, which prevented me from landing it on cedar.  Please submit an updated version of the patch, or land on cedar yourself.

Comment 12

[Sid Stamm [:geekboy or :sstamm]]

Attachment: fix

Rebased (mainly the test), removed dependency on bug 552523, and carried over r=sicking.

Comment 13

[Ian Melven :imelven]

(In reply to Sid Stamm [:geekboy] from comment #12)
> Created attachment 624597 [details] [diff] [review]
> fix
> 
> Rebased (mainly the test), removed dependency on bug 552523, and carried
> over r=sicking.

-          // we need to set an nsIChannelEventSink on the XHR object
-          // so we can tell it to not follow redirects when posting the reports
-          req.channel.notificationCallbacks = new CSPReportRedirectSink();

doesn't removing this unfix bug 650386 ?

if not, CSPReportRedirectSink() should be removed imo

Comment 14

[Sid Stamm [:geekboy or :sstamm]]

Attachment: fix

Yeah, you're right about that, imelven, thanks for catching that.  Verified bustage in tests for bug 650386.  Maybe this is why I shouldn't let reviewed patches sit around for a year without landing.

New patch, verified that tests for bug 650386 will pass.  Since the r=sicking is so old, I'm gonna ask for it again.  Jonas: this is a pretty small patch.  The tests didn't change materially since you r+'ed it, but the patch has changed a bit.

Comment 15

[Ian Melven :imelven]

Comment on attachment 624789 [details] [diff] [review]
fix

LGTM

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 624789 [details] [diff] [review]
fix

We should probably sprinkle some security checks over this code.

First off you should do a checkloaduri check to make sure that you can't submit reports to dangerous protocols like shell:.

Alternatively, you might want co do a same-origin check (I forgot what we decided regarding same-origin policy here).

Second, you should do a contentpolicy check.

Comment 17

[Sid Stamm [:geekboy or :sstamm]]

Thanks, Jonas.  

Regarding checkloaduri and same-origin, we validate the URI when the policy is parsed (http://mxr.mozilla.org/mozilla-central/source/content/base/src/CSPUtils.jsm#252), so I don't want to duplicate that work when we send reports.

ContentPolicy: yeah, this makes sense.  I assume the previous technique (nsIXmlHttpRequest) triggered the policy checks you're asking for here?

Comment 18

[Sid Stamm [:geekboy or :sstamm]]

Attachment: fix

updated patch that checks with the ContentPolicy service before sending out the report pings.

Comment 19

[Sid Stamm [:geekboy or :sstamm]]

Pushed to mozilla-inbound.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bb7b61ca464

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0bb7b61ca464