Last Comment Bug 612391 - CSP should use an HTTP channel instead of XHR for reporting violations
: CSP should use an HTTP channel instead of XHR for reporting violations
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Sid Stamm [:geekboy or :sstamm]
:
Mentors:
Depends on:
Blocks: CSP
  Show dependency treegraph
 
Reported: 2010-11-15 13:49 PST by Blake Kaplan (:mrbkap) (please use needinfo!)
Modified: 2012-05-24 09:40 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed patch (9.11 KB, patch)
2010-11-15 17:10 PST, Sid Stamm [:geekboy or :sstamm]
jonas: review-
Details | Diff | Review
proposed patch (14.90 KB, patch)
2010-11-17 18:00 PST, Sid Stamm [:geekboy or :sstamm]
jonas: review+
Details | Diff | Review
fix (10.64 KB, patch)
2010-11-23 15:01 PST, Sid Stamm [:geekboy or :sstamm]
mozbugs: review+
mbeltzner: approval2.0-
Details | Diff | Review
fix (12.36 KB, patch)
2012-05-16 16:55 PDT, Sid Stamm [:geekboy or :sstamm]
mozbugs: review+
Details | Diff | Review
fix (12.50 KB, patch)
2012-05-17 09:57 PDT, Sid Stamm [:geekboy or :sstamm]
jonas: review-
ian.melven: feedback+
Details | Diff | Review
fix (13.14 KB, patch)
2012-05-22 15:08 PDT, Sid Stamm [:geekboy or :sstamm]
jonas: review+
Details | Diff | Review

Description Blake Kaplan (:mrbkap) (please use needinfo!) 2010-11-15 13:49:28 PST
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] 2010-11-15 13:55:14 PST
Or could the XHR be created asynchronously?
Comment 2 Sid Stamm [:geekboy or :sstamm] 2010-11-15 17:10:01 PST
Created attachment 490761 [details] [diff] [review]
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) 2010-11-15 17:25:58 PST
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 2010-11-16 04:10:15 PST
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] 2010-11-16 06:53:43 PST
(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] 2010-11-17 18:00:09 PST
Created attachment 491409 [details] [diff] [review]
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) 2010-11-22 16:11:42 PST
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] 2010-11-23 15:01:30 PST
Created attachment 492822 [details] [diff] [review]
fix

Removed the nsIHttpChannel check as requested.
Comment 9 Sid Stamm [:geekboy or :sstamm] 2010-11-23 15:03:39 PST
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] 2011-02-23 18:14:15 PST
Comment on attachment 492822 [details] [diff] [review]
fix

Please renominate for approval once the dependency gets in.
Comment 11 :Ehsan Akhgari (busy, don't ask for review please) 2011-03-22 08:13:53 PDT
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] 2012-05-16 16:55:03 PDT
Created attachment 624597 [details] [diff] [review]
fix

Rebased (mainly the test), removed dependency on bug 552523, and carried over r=sicking.
Comment 13 Ian Melven :imelven 2012-05-16 17:03:54 PDT
(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] 2012-05-17 09:57:23 PDT
Created attachment 624789 [details] [diff] [review]
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 2012-05-17 14:59:29 PDT
Comment on attachment 624789 [details] [diff] [review]
fix

LGTM
Comment 16 Jonas Sicking (:sicking) 2012-05-22 14:05:28 PDT
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] 2012-05-22 14:13:26 PDT
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] 2012-05-22 15:08:46 PDT
Created attachment 626199 [details] [diff] [review]
fix

updated patch that checks with the ContentPolicy service before sending out the report pings.
Comment 19 Sid Stamm [:geekboy or :sstamm] 2012-05-23 16:01:24 PDT
Pushed to mozilla-inbound.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bb7b61ca464
Comment 20 Ed Morley [:emorley] 2012-05-24 09:33:17 PDT
https://hg.mozilla.org/mozilla-central/rev/0bb7b61ca464

Note You need to log in before you can comment on or make changes to this bug.