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

RESOLVED FIXED in mozilla15

Status

()

defect
RESOLVED FIXED
9 years ago
4 months ago

People

(Reporter: mrbkap, Assigned: geekboy)

Tracking

(Blocks 1 bug)

Trunk
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

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).
Or could the XHR be created asynchronously?
Posted patch proposed patch (obsolete) — Splinter Review
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.
Assignee: nobody → sstamm
Status: NEW → ASSIGNED
Attachment #490761 - Flags: review?(jonas)
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.
Attachment #490761 - Flags: review?(jonas) → review-
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]),
(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.
Posted patch proposed patch (obsolete) — Splinter Review
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.
Attachment #490761 - Attachment is obsolete: true
Attachment #491409 - Flags: review?(jonas)
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
Attachment #491409 - Flags: review?(jonas) → review+
Posted patch fix (obsolete) — Splinter Review
Removed the nsIHttpChannel check as requested.
Attachment #491409 - Attachment is obsolete: true
Attachment #492822 - Flags: review+
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.
Depends on: 552523
Attachment #492822 - Flags: approval2.0?
Attachment #492822 - Flags: approval2.0? → approval2.0+
Comment on attachment 492822 [details] [diff] [review]
fix

Please renominate for approval once the dependency gets in.
Attachment #492822 - Flags: approval2.0+ → approval2.0-
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.
Whiteboard: not-ready-for-cedar
Posted patch fix (obsolete) — Splinter Review
Rebased (mainly the test), removed dependency on bug 552523, and carried over r=sicking.
Attachment #492822 - Attachment is obsolete: true
Attachment #624597 - Flags: review+
(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
Posted patch fix (obsolete) — Splinter Review
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.
Attachment #624597 - Attachment is obsolete: true
Attachment #624789 - Flags: review?(jonas)
Attachment #624789 - Flags: feedback?(imelven)
Comment on attachment 624789 [details] [diff] [review]
fix

LGTM
Attachment #624789 - Flags: feedback?(imelven) → feedback+
No longer depends on: 552523
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: not-ready-for-cedar
Attachment #624789 - Flags: review?(jonas) → review?(jst)
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.
Attachment #624789 - Flags: review?(jst) → review-
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?
Posted patch fixSplinter Review
updated patch that checks with the ContentPolicy service before sending out the report pings.
Attachment #624789 - Attachment is obsolete: true
Attachment #626199 - Flags: review?(jonas)
Pushed to mozilla-inbound.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bb7b61ca464
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/0bb7b61ca464
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Blocks: CSP
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.