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

RESOLVED FIXED in mozilla15

Status

()

Core
DOM
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: mrbkap, Assigned: geekboy)

Tracking

(Blocks: 1 bug)

Trunk
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

7 years ago
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

7 years ago
Or could the XHR be created asynchronously?
(Assignee)

Comment 2

7 years ago
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.
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]),
(Assignee)

Comment 5

7 years ago
(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.
(Assignee)

Comment 6

7 years ago
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.
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+
(Assignee)

Comment 8

7 years ago
Created attachment 492822 [details] [diff] [review]
fix

Removed the nsIHttpChannel check as requested.
Attachment #491409 - Attachment is obsolete: true
Attachment #492822 - Flags: review+
(Assignee)

Comment 9

7 years ago
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
(Assignee)

Updated

7 years ago
Attachment #492822 - Flags: approval2.0?

Updated

6 years ago
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
(Assignee)

Comment 12

5 years ago
Created attachment 624597 [details] [diff] [review]
fix

Rebased (mainly the test), removed dependency on bug 552523, and carried over r=sicking.
Attachment #492822 - Attachment is obsolete: true
Attachment #624597 - Flags: review+

Comment 13

5 years ago
(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
(Assignee)

Comment 14

5 years ago
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.
Attachment #624597 - Attachment is obsolete: true
Attachment #624789 - Flags: review?(jonas)
Attachment #624789 - Flags: feedback?(imelven)

Comment 15

5 years ago
Comment on attachment 624789 [details] [diff] [review]
fix

LGTM
Attachment #624789 - Flags: feedback?(imelven) → feedback+
(Assignee)

Updated

5 years ago
No longer depends on: 552523
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: not-ready-for-cedar
(Assignee)

Updated

5 years ago
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-
(Assignee)

Comment 17

5 years ago
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?
(Assignee)

Comment 18

5 years ago
Created attachment 626199 [details] [diff] [review]
fix

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)
Attachment #626199 - Flags: review?(jonas) → review+
(Assignee)

Comment 19

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Blocks: 493857
You need to log in before you can comment on or make changes to this bug.