594446 - CSP report-uri should accept relative URIs

Status: RESOLVED FIXED

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

Description

[James Socol [:jsocol, :james]]

From the CSP spec on report URIs: "Relative URIs are acceptable, and are resolved within the same scheme, host and port as the document served with the CSP."

Like bug 558429 for policy-uri, report-uri should accept relative URIs. Currently (9/08 nightly) given

X-Content-Security-Policy: allow 'self'; report-uri /csp/report

the following message is printed to the error console:

Warning: CSP: couldn't parse report URI: /csp/report

Comment 1

[Wil Clouser [:clouserw]]

This blocks us from using CSP on AMO.

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

Bah, I should read the whole bug before duping.  I'll take this one.

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

Attachment: fix

Okay, this was originally supposed to be a one-line fix, but in the course of flipping the switch for relative URIs, I had to clean up some of the parsing code.  Now any report URIs parsed from a string are stored in the CSPRep object as absolute URIs so that when reports go out, they go out absolutely.

Anyhow, the bulk of this fix is actually testing code in test_csputils.js, the rest is pretty straightforward.  The parser code for report-uri now is a series of checks for:
1. the report URI parses as a valid nsIURI
2. the report URI has a host or IP address 
3. the host matches ETLD+1 of the protected content's URI, or if it is an IP address, it completely matches the protected content
4. scheme and port match the protected content's URI.

For good hygiene, the parser also now canonicalizes the report URIs by turning them into an nsIURI and then storing the asciiSpec of that; this has the bonus of doing the relative URI resolution in the same step.

Comment 5

[Johnny Stenback (:jst)]

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

># HG changeset patch
># User Sid Stamm <[e-mail]
># Date 1285281527 25200
># Node ID 077c86a74d6d772cc5edc80f12d036b0146ba15f
># Parent  e8471f5c358dd11ab7a961fc95db987031509bfd
>Bug 594446 - CSP report-uri should accept relative URIs
>
>diff -r e8471f5c358d content/base/src/CSPUtils.jsm
>--- a/content/base/src/CSPUtils.jsm	Wed May 19 16:31:44 2010 -0700
>+++ b/content/base/src/CSPUtils.jsm	Fri Sep 24 16:12:11 2010 -0700
>@@ -211,53 +211,75 @@ CSPRep.fromString = function(aStr, self)
>         // process dirs, and enforce that 'self' is defined.
>         var dv = CSPSourceList.fromString(dirvalue, self, true);
>         if (dv) {
>           aCSPR._directives[sdi] = dv;
>           continue directive;
>         }
>       }
>     }
>-    
>+
>     // REPORT URI ///////////////////////////////////////////////////////
>     if (dirname === UD.REPORT_URI) {
>       // might be space-separated list of URIs
>       var uriStrings = dirvalue.split(/\s+/);
>       var okUriStrings = [];
> 
>-      // Verify that each report URI is in the same etld + 1
>-      // if "self" is defined, and just that it's valid otherwise.
>       for (let i in uriStrings) {
>+        var uri = null;
>         try {
>-          var uri = gIoService.newURI(uriStrings[i],null,null);
>+          // Relative URIs are okay, but to ensure we send the reports to the
>+          // right spot, the relative URIs are expanded here during parsing.
>+          // The resulting CSPRep instance will have only absolute URIs.
>+          uri = gIoService.newURI(uriStrings[i],null,selfUri);
>+
>+          // if there's no host, don't do the ETLD+ check.  This will throw
>+          // NS_ERROR_FAILURE if the URI doesn't have a host, causing a parse
>+          // failure.
>+          uri.host;
>+
>+          // Verify that each report URI is in the same etld + 1 and that the
>+          // scheme and port match "self" if "self" is defined, and just that
>+          // it's valid otherwise.
>           if (self) {
>-            if (gETLDService.getBaseDomain(uri) ===
>+            if (gETLDService.getBaseDomain(uri) !==
>                 gETLDService.getBaseDomain(selfUri)) {
>-              okUriStrings.push(uriStrings[i]);
>-            } else {
>               CSPWarning("can't use report URI from non-matching eTLD+1: "
>                          + gETLDService.getBaseDomain(uri));
>+              continue;
>+            }
>+            if (!uri.schemeIs(selfUri.scheme)) {
>+              CSPWarning("can't use report URI with different scheme from "
>+                         + "originating document: " + uri.asciiSpec);
>+              continue;
>+            }
>+            if (uri.port && uri.port !== selfUri.port) {
>+              CSPWarning("can't use report URI with different port from "
>+                         + "originating document: " + uri.asciiSpec);
>+              continue;
>             }
>           }
>         } catch(e) {
>           switch (e.result) {
>             case Components.results.NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS:
>             case Components.results.NS_ERROR_HOST_IS_IP_ADDRESS:
>-              if (uri.host === selfUri.host) {
>-                okUriStrings.push(uriStrings[i]);
>-              } else {
>-                CSPWarning("page on " + selfUri.host + " cannot send reports to " + uri.host);
>+              if (uri.host !== selfUri.host) {
>+                CSPWarning("page on " + selfUri.host
>+                           + " cannot send reports to " + uri.host);
>+                continue;
>               }
>               break;
> 
>             default:
>               CSPWarning("couldn't parse report URI: " + uriStrings[i]);
>-              break;
>+              continue;
>           }
>         }
>+        // all verification passed: same ETLD+1, scheme, and port.
>+        okUriStrings.push(uri.asciiSpec);
>       }
>       aCSPR._directives[UD.REPORT_URI] = okUriStrings.join(' ');
>       continue directive;
>     }
> 
>     // POLICY URI //////////////////////////////////////////////////////////
>     if (dirname === UD.POLICY_URI) {
>       // POLICY_URI can only be alone
>diff -r e8471f5c358d content/base/src/contentSecurityPolicy.js
>--- a/content/base/src/contentSecurityPolicy.js	Wed May 19 16:31:44 2010 -0700
>+++ b/content/base/src/contentSecurityPolicy.js	Fri Sep 24 16:12:11 2010 -0700
>@@ -327,16 +327,19 @@ ContentSecurityPolicy.prototype = {
>         }
>       }
>       CSPdebug("Constructed violation report:\n" + JSON.stringify(report));
> 
>       CSPWarning("Directive \"" + violatedDirective + "\" violated"
>                + (blockedUri['asciiSpec'] ? " by " + blockedUri.asciiSpec : ""));
> 
>       // For each URI in the report list, send out a report.
>+      // We make the assumption that all of the URIs are absolute URIs; this
>+      // should be taken care of in CSPRep.fromString (where it converts any
>+      // relative URIs into absolute ones based on "self").
>       for (let i in uris) {
>         if (uris[i] === "")
>           continue;
> 
>         var failure = function(aEvt) {  
>           if (req.readyState == 4 && req.status != 200) {
>             CSPError("Failed to send report to " + reportURI);
>           }  
>diff -r e8471f5c358d content/base/test/unit/test_csputils.js
>--- a/content/base/test/unit/test_csputils.js	Wed May 19 16:31:44 2010 -0700
>+++ b/content/base/test/unit/test_csputils.js	Fri Sep 24 16:12:11 2010 -0700
>@@ -42,16 +42,36 @@ do_load_httpd_js();
> 
> var httpServer = new nsHttpServer();
> 
> const POLICY_FROM_URI = "allow 'self'; img-src *";
> const POLICY_PORT = 9000;
> const POLICY_URI = "http://localhost:" + POLICY_PORT + "/policy";
> const POLICY_URI_RELATIVE = "/policy";
> 
>+// helper to assert that an array has the given value somewhere.
>+function do_check_in_array(arr, val, stack) {
>+  if (!stack)
>+    stack = Components.stack.caller;
>+
>+  var text = val + " in [" + arr.join(",") + "]";
>+
>+  for(var i in arr) {
>+    dump(".......... " + i + "> " + arr[i] + "\n");
>+    if(arr[i] == val) {
>+      //succeed
>+      ++_passedChecks;
>+      dump("TEST-PASS | " + stack.filename + " | [" + stack.name + " : " +
>+           stack.lineNumber + "] " + text + "\n");
>+      return;
>+    }
>+  }
>+  do_throw(text, stack);
>+}
>+
> // helper to assert that an object or array must have a given key
> function do_check_has_key(foo, key, stack) {
>   if (!stack) 
>     stack = Components.stack.caller;
> 
>   var keys = [];
>   for(let k in keys) { keys.push(k); }
>   var text = key + " in [" + keys.join(",") + "]";
>@@ -500,16 +520,76 @@ test(function test_FrameAncestor_default
> 
>       //"frame-ancestors should only allow self"
>       do_check_true(cspr.permits("http://self.com:34", SD.FRAME_ANCESTORS));
>       do_check_false(cspr.permits("https://foo.com:400", SD.FRAME_ANCESTORS));
>       do_check_false(cspr.permits("https://self.com:34", SD.FRAME_ANCESTORS));
>       do_check_false(cspr.permits("http://self.com", SD.FRAME_ANCESTORS));
>       do_check_false(cspr.permits("http://subd.self.com:34", SD.FRAME_ANCESTORS));
>      });
>+
>+test(function test_CSP_ReportURI_parsing() {
>+      var cspr;
>+      var SD = CSPRep.SRC_DIRECTIVES;
>+      var self = "http://self.com:34";
>+      var parsedURIs = [];
>+
>+      var uri_valid_absolute = self + "/report.py";
>+      var uri_invalid_host_absolute = "http://foo.org:34/report.py";
>+      var uri_valid_relative = "/report.py";
>+      var uri_valid_relative_expanded = self + uri_valid_relative;
>+      var uri_valid_relative2 = "foo/bar/report.py";
>+      var uri_valid_relative2_expanded = self + "/" + uri_valid_relative2;
>+      var uri_invalid_relative = "javascript:alert(1)";
>+
>+      cspr = CSPRep.fromString("allow *; report-uri " + uri_valid_absolute, self);
>+      parsedURIs = cspr.getReportURIs().split(/\s+/);
>+      do_check_in_array(parsedURIs, uri_valid_absolute);
>+      do_check_eq(parsedURIs.length, 1);
>+
>+      cspr = CSPRep.fromString("allow *; report-uri " + uri_invalid_host_absolute, self);
>+      parsedURIs = cspr.getReportURIs().split(/\s+/);
>+      do_check_in_array(parsedURIs, "");
>+      do_check_eq(parsedURIs.length, 1); // the empty string is in there.
>+
>+      cspr = CSPRep.fromString("allow *; report-uri " + uri_invalid_relative, self);
>+      parsedURIs = cspr.getReportURIs().split(/\s+/);
>+      do_check_in_array(parsedURIs, "");
>+      do_check_eq(parsedURIs.length, 1);
>+
>+      cspr = CSPRep.fromString("allow *; report-uri " + uri_valid_relative, self);
>+      parsedURIs = cspr.getReportURIs().split(/\s+/);
>+      do_check_in_array(parsedURIs, uri_valid_relative_expanded);
>+      do_check_eq(parsedURIs.length, 1);
>+
>+      cspr = CSPRep.fromString("allow *; report-uri " + uri_valid_relative2, self);
>+      parsedURIs = cspr.getReportURIs().split(/\s+/);
>+      dump(parsedURIs.length);
>+      do_check_in_array(parsedURIs, uri_valid_relative2_expanded);
>+      do_check_eq(parsedURIs.length, 1);
>+
>+      // combination!
>+      cspr = CSPRep.fromString("allow *; report-uri " +
>+                               uri_valid_relative2 + " " +
>+                               uri_valid_absolute, self);
>+      parsedURIs = cspr.getReportURIs().split(/\s+/);
>+      do_check_in_array(parsedURIs, uri_valid_relative2_expanded);
>+      do_check_in_array(parsedURIs, uri_valid_absolute);
>+      do_check_eq(parsedURIs.length, 2);
>+
>+      cspr = CSPRep.fromString("allow *; report-uri " +
>+                               uri_valid_relative2 + " " +
>+                               uri_invalid_host_absolute + " " +
>+                               uri_valid_absolute, self);
>+      parsedURIs = cspr.getReportURIs().split(/\s+/);
>+      do_check_in_array(parsedURIs, uri_valid_relative2_expanded);
>+      do_check_in_array(parsedURIs, uri_valid_absolute);
>+      do_check_eq(parsedURIs.length, 2);
>+    });
>+
> /*
> 
> test(function test_CSPRep_fromPolicyURI_failswhenmixed() {
>         var cspr;
>         var self = "http://localhost:" + POLICY_PORT;
>         var closed_policy = CSPRep.fromString("allow 'none'");
>         var my_uri_policy = "policy-uri " + POLICY_URI;
>

Comment 6

[Johnny Stenback (:jst)]

Oh, and ignore that massive comment with the whole patch in it. Not sure how that happened.

Comment 7

[James Socol [:jsocol, :james]]

This is blocking web dev from launching CSP on a number of properties, including AMO and SUMO. Is there anything preventing it from landing?

Comment 8

[James Socol [:jsocol, :james]]

Requesting blocking2.0 since this is the last hurtle to deploying CSP on some of our major web properties, and, as I understand it, a pretty low-risk fix.

Comment 9

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

Webdev needs lead time, we want to point to CSP as something that's good for the web and that we dogfood, I see no reason to delay this any further.

Comment 10

[Reed Loden [:reed]]

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

http://hg.mozilla.org/mozilla-central/rev/23605bdd871a