650386 - CSP should not follow redirects for report-uri

Status: RESOLVED FIXED

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

Description

[Brandon Sterne (:bsterne)]

From the CSP spec:
"User-agents must not follow HTTP 3xx redirect responses when sending violation reports."

We currently follow a 302 response to the report request by sending a GET to the redirected location.

To reproduce:
1. Load http://hackmill.com/csp/tests/redir-report-uri-test.php
2. Note the requests:
  a. GET http://hackmill.com/csp/tests/redir-report-uri-test.php HTTP/1.1
  b. POST http://hackmill.com/csp/tests/resources/csp-report.php HTTP/1.1
  c. GET http://brandon.sternefamily.net/csp/resources/csp-report HTTP/1.1

c. definitely shouldn't happen.

Comment 1

[Ian Melven :imelven]

should this test case still be working ? i don't see the requests in 2b. and 2c. happening - should i only see this in a nightly ?

Comment 2

[Brandon Sterne (:bsterne)]

I'm definitely still seeing the redirect using Nightly:

1 GET http://hackmill.com/csp/tests/redir-report-uri-test.php HTTP/1.1 	=> HTTP/1.1 200 OK	 [0.277 s]
5 POST http://hackmill.com/csp/tests/resources/csp-report.php HTTP/1.1 	=> HTTP/1.1 302 Moved Temporarily	 [0.149 s]
8 GET http://brandon.sternefamily.net/csp/resources/csp-report HTTP/1.1 	=> HTTP/1.1 404 Not Found	 [0.325 s]

I'm using an external proxy, though.  If you're just using Web Console, those requests won't show up due to bug 639533.

Comment 3

[Ian Melven :imelven]

ah ok, i was just using web console, i'll set up an external proxy and check it out. thanks !

Comment 4

[Ian Melven :imelven]

Attachment: first stab at a patch for this

this is a first stab at a patch for this.. i'm still learning XPCOM/XPConnect so this might not be the sanest/correctest way of doing this. this bug should also probably have an associated test as well, which i will work on next.

Comment 5

[Ian Melven :imelven]

i have a test mostly written for this but bsterne and i are going to rap with jst about new style mochitests to try and do this in the new style if possible and hopefully save an iteration

Comment 6

[Ian Melven :imelven]

Attachment: v2 updated patch

Comment 7

[Ian Melven :imelven]

Attachment: WIP - test for bug 650386

needs work to remove the use of EnablePrivilege, but the test works with it. will update with a non-EnablePrivilege version.

Comment 8

[Ian Melven :imelven]

Attachment: latest patch

this patch includes tests modified to use SpecialPowers and has been updated, sending to bsterne for feedback

Comment 9

[Brandon Sterne (:bsterne)]

Comment on attachment 559580 [details] [diff] [review]
latest patch

>+		  // we need to set an nsIChannelEventSink on the XHR object
>+		  // so we can tell it to not follow redirects when posting the report 
>+          req.channel.notificationCallbacks = reportChannelEventSink;		  	
>+		  

Can you line the comment up with the rest of the code there?

>+var reportChannelEventSink =
>  ...
>+   // nsIInterfaceRequestor
>+   getInterface: function requestor_gi(iid) {
>+     if (iid.equals(Ci.nsIChannelEventSink)) {
>+       try {
>+		 return this;
>+       } catch (e) { do_throw(e); }
>+     }
>+     throw Cr.NS_ERROR_NO_INTERFACE;
>+   },

I'm definitely no authority here, but I haven't seen this pattern before.  Why wrap the single return statement?  Is there a chance return can throw?  Also, do_throw only seems to be used in unit tests, which you appear to have modeled some of this code after.

I'd just simplify it by removing the try/catch, more like:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/plugins/content/pluginInstallerService.js#154

>+   // nsIChannelEventSink 
>+   asyncOnChannelRedirect: function channel_redirect(oldChannel, newChannel, 
>+                                                     flags, callback) {
>+     // cancel the old channel so XHR failure callback happens
>+     oldChannel.cancel(Cr.NS_ERROR_ABORT);
>+   
>+     // notify an observer that we have blocked the report POST due to a redirect, 
>+     // used in testing, do this async since we're in an async call now to begin with 
>+     Services.tm.mainThread.dispatch(
>+       function() {
>+         observerSubject = Cc["@mozilla.org/supports-cstring;1"]
>+                              .createInstance(Ci.nsISupportsCString);
>+         observerSubject.data = oldChannel.URI.asciiSpec;
>+	
>+         Services.obs.notifyObservers(observerSubject,
>+                                      CSP_VIOLATION_TOPIC,
>+                                      "denied redirect while sending violation report");
>+       }, Ci.nsIThread.DISPATCH_NORMAL);
>+   
>+     // throw to stop the redirect happening
>+     throw Cr.NS_BINDING_REDIRECTED;;
>+   }
>+};

Looks good.

Your test make sense too: 1) create a page with a built-in violation and a report-uri, 2) report-uri responds with a 302, 3) test harness listens for CSP_VIOLATION_TOPIC and the data is the one you expect (the thing about the redirect, not the one about the image being blocked).

Comment 10

[Ian Melven :imelven]

Attachment: patch v4

patch updated with feedback from bsterne and adding changes from review of bug #674255 (SpecialPowers patch). will try to get 674255 landed and then have this reviewed and landed.

Comment 11

[Ian Melven :imelven]

Attachment: patch v5

clean up some indentation, ready for review after 674255 lands

Comment 12

[Ian Melven :imelven]

Attachment: patch v6

tweak the test a little bit based on review tweak to 674255

Comment 13

[Ian Melven :imelven]

Attachment: patch v7

fix an assertion that crept in with last fix. going to give this a try run.

Comment 14

[Ian Melven :imelven]

Attachment: patch v8

extra space removed ! waiting for try run to finish..

Comment 15

[Ian Melven :imelven]

try run : https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=1e5c09ff0cf7

no failures that look related to this change, imo.

Comment 16

[Ian Melven :imelven]

Attachment: patch v9

unbitrot, 674255 is hopefully about to land so sending this to dveditz for review

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 566935 [details] [diff] [review]
patch v9

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

This is really close but we need a couple small changes. The only substantive one is logging the blocked redirect to the error console. A reluctant r-


General comments on the test:
* Looks good for an explicit 302 test.

* Doesn't the spec apply equally to 301,303, and 307 redirects? Unless we already have tests for those (seems unlikely or we wouldn't have missed 302) we should add those, too. Yes, this is the paranoia talking, but extra tests are cheap.

* There's a push to have more-explicitly named tests rather than just bug numbers which run together. "test_bug650386_reporturi_redirects.html" for example. Or "...302redirect.html" if you put each redirect type in its own test. Separate tests might add clarity if there are failures but I could go either way. Either way you should be able to keep a single file_bug650386.html test file and call with "?redir=301" etc. and have the .sjs file do a conditional response. I hope anyway, haven't tried doing a test like that.
  Hm, if you combine the tests you can't just test violation==PASS/http-modify==FAIL, you'll have to check which request was which (both pass and fail). My gut feel is that you want a separate test_ host file for each type of redirect to keep the test code simple.

* For similar help-the-future-maintainer reasons there's also a push to make sure every test has a comment at the top that explains what it's testing. The bug number can be a good reference but don't force everyone to make that lookup (and some bugs make a terrible reference, or the test only tests part of a bug). A single line "Test that CSP reportURI does not follow 3xx redirects as per spec" or something on those lines is fine.

::: content/base/src/contentSecurityPolicy.js
@@ -496,0 +500,37 @@
> > +// the POST of the violation report (if it happens) should not follow
> > +// redirects, per the spec. hence, we implement an nsIChannelEventSink here
> > +// so we can tell XHR to abort if a redirect happens. 
> > +var reportChannelEventSink =
NaN more ...

This notification is good for testing but won't help a site author debug why they aren't getting any reports. We need to also log this error to the console. Consistent with the main CSP sendReports you probably want to use CSPWarning(). As Sid explained the problem is blocked due to correct interpretation of the CSP policy, not a problem with CSP itself or understanding the policy. As a completely separate bug we might want to reconsider which things are warnings and which errors, but for now CSPWarning is most consistent.

Comment 18

[Daniel Veditz [:dveditz]]

> > > +var reportChannelEventSink =
> NaN more ...

The context was supposed to be the notifyObservers call

>       Services.obs.notifyObservers(observerSubject,
>                            CSP_VIOLATION_TOPIC,
>                            "denied redirect while sending violation report");

Comment 19

[Ian Melven :imelven]

Attachment: patch v10

New version of patch that addresses Dan's review feedback : 

* Adds tests for 301, 303 and 307 redirects as well as 302 redirects

* Rename files to give more info about what they're testing

* Add comment to test_* files explaining what they are testing

* Add CSPWarning() call when a redirect is blocked

Comment 20

[Ian Melven :imelven]

Comment on attachment 608152 [details] [diff] [review]
patch v10

disregard that r? - i'm gonna try and do this with a test_ file per redirect and 2 .sjs files instead of the way it is now. will r? again when that's done :)

Comment 21

[Ian Melven :imelven]

Attachment: patch v11

I like this version a lot more - less files, but still preserves the test case for each redirect value.

Comment 22

[Ian Melven :imelven]

https://tbpl.mozilla.org/?tree=Try&rev=82bbde692032

the oranges don't look related to this change to me.

Comment 23

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 608215 [details] [diff] [review]
patch v11

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

A few nits, but overall really good.  Unless you've got a reason I'm unaware of, please use the constructor pattern for the reportChannelEventSink (function + prototype -- maybe call it "CSPReportRedirectSink) and I'd like to have another look.

Clearing r? flag too, for now.

::: content/base/src/contentSecurityPolicy.js
@@ +300,5 @@
>            continue;
>  
>          var failure = function(aEvt) {  
>            if (req.readyState == 4 && req.status != 200) {
> +            CSPError("Failed to send report to " + uris[i]);

good catch.

@@ +500,5 @@
>  
> +// the POST of the violation report (if it happens) should not follow
> +// redirects, per the spec. hence, we implement an nsIChannelEventSink here
> +// so we can tell XHR to abort if a redirect happens. 
> +var reportChannelEventSink =

You may want to set this up as a class (function Foo() constructor and a prototype) and instantiate it here.

@@ +504,5 @@
> +var reportChannelEventSink =
> +{
> +  QueryInterface: function requestor_qi(iid) {
> +    if (iid.equals(Ci.nsISupports) ||
> +        iid.equals(Ci.nsIInterfaceRequestor))

you probably also want "|| iid.equals(Ci.nsIChannelEventSink)" here too since this object is one.

@@ +521,5 @@
> +  // nsIChannelEventSink 
> +  asyncOnChannelRedirect: function channel_redirect(oldChannel, newChannel, 
> +                                                    flags, callback) {
> +    CSPWarning("Post of violation report to " + oldChannel.URI.asciiSpec + 
> +      " failed, as a redirect occurred");

nit: please line up string parts (add nine spaces to the beginning of this line).

@@ +531,5 @@
> +    // used in testing, do this async since we're in an async call now to begin with 
> +    Services.tm.mainThread.dispatch(
> +      function() {
> +        observerSubject = Cc["@mozilla.org/supports-cstring;1"]
> +                             .createInstance(Ci.nsISupportsCString);

nit: please unindent by one space to be consistent with the rest of the file (line up [ and .)

@@ +537,5 @@
> +	
> +        Services.obs.notifyObservers(observerSubject,
> +                                      CSP_VIOLATION_TOPIC,
> +                                      "denied redirect while sending violation report");
> +      }, Ci.nsIThread.DISPATCH_NORMAL);

nit: please line up arguments to notifyObservers()

::: content/base/test/file_bug650386_content.sjs
@@ +10,5 @@
> +{
> +  response.setHeader("Cache-Control", "no-cache", false);
> +
> +  // this gets used in the CSP as part of the report URI.
> +  var redirect = request.queryString;

For catching errors in future code, you may want to validate the queryString here... ensure it's a 30x with no additional cruft.  Throw a really nasty error if it's invalid so that if we accidentally break this test in the future it'll be obvious.

Comment 24

[Ian Melven :imelven]

Attachment: patch v12

Address Sid's feedback - change the nsIChannelEventSink implementor to be CSPReportRedirectSink, clean up the nits, do some general whitespace/indentation cleanup, and change the content .sjs file to do a redirect (which will fail the test) if it gets passed a redirect that isn't covered by these tests.

Comment 25

[Ian Melven :imelven]

Attachment: patch v13

Makefile bit rotted :( pushed this to try.

Comment 26

[Ian Melven :imelven]

https://tbpl.mozilla.org/?tree=Try&rev=ae6caccbbfef

Comment 27

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 608905 [details] [diff] [review]
patch v13

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

Looks good.

::: content/base/src/contentSecurityPolicy.js
@@ +547,5 @@
> +    // throw to stop the redirect happening
> +    throw Cr.NS_BINDING_REDIRECTED;
> +  }
> +};
> +

Excellent.

::: content/base/test/file_bug650386_content.sjs
@@ +6,5 @@
> +// returned/type of redirect to do comes from the query string
> +// parameter passed in from the test_bug650386_* files and then also
> +// uses that value in the report-uri parameter of the CSP
> +function handleRequest(request, response)
> +{

really minor nit: please put { on previous line

::: content/base/test/file_bug650386_report.sjs
@@ +4,5 @@
> +// This handles 301, 302, 303 and 307 redirects. The HTTP status code
> +// returned/type of redirect to do comes from the query string
> +// parameter
> +function handleRequest(request, response)
> +{

same as previous nit: collapse line 7 and 8.

Comment 28

[Ian Melven :imelven]

Attachment: patch v14

Thanks Sid. This revision addresses those nits and also cleans up more whitespace. r? to jst

Comment 29

[Sid Stamm [:geekboy or :sstamm]]

Pushed to inbound. 
http://hg.mozilla.org/integration/mozilla-inbound/rev/2f79b816b0da

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2f79b816b0da