843311 - Content Security Policy report destination restriction loosening

Status: RESOLVED FIXED

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

Description

[Neil Matatall]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_5) AppleWebKit/537.32 (KHTML, like Gecko) Chrome/27.0.1416.0 Safari/537.32

Steps to reproduce:

I hardcoded the report-uri scheme into my header for http and https requests from some.example.com to example.com


Actual results:

Violations were only sent when the report-uri scheme matched the scheme the page was served on.

Specifically, an http page tried to post a CSP report to the same host over https, but firefox did not allow it.


Expected results:

CSP should allow me to "upgrade" security so I can make an SSL request on an HTTP document. It should continue the behavior where data on an HTTPS page is not sent to an HTTP report-uri.

I guess this would involve changing http://mxr.mozilla.org/mozilla-central/source/content/base/src/CSPUtils.jsm#362 and the subsequent port check to allow "upgraded" connections.

Comment 1

[Ian Melven :imelven]

Thanks for filing this, Neil ! 

Sid thinks the scheme check here might be mandated by the spec, I'll take a look and bring it up on the WG mailing list if so.

Comment 2

[Ian Melven :imelven]

(In reply to Ian Melven :imelven from comment #1)
> Thanks for filing this, Neil ! 
> 
> Sid thinks the scheme check here might be mandated by the spec, I'll take a
> look and bring it up on the WG mailing list if so.

Dan points out this isn't in the spec, it's all us.

Comment 3

[Tanvi Vyas[:tanvi]]

I think we should definitely allow upgraded report-uris.  Example: The protected resource / the page the user is on is http://foo.com and the report-uri is https://foo.com.  There is no reason that Firefox should block that.  This seems like an easy code change.  Something like...

if (!uri.schemeIs(selfUri.scheme)) {
  if (!(uri.schemeIs("https") && selfUri.schemeIs("http"))) {
    cspWarn(aCSPR, CSPLocalizer.getFormatStr("notSameScheme", [uri.asciiSpec]));
  }
  continue;
}

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Tanvi Vyas [:tanvi] from comment #3)
> if (!uri.schemeIs(selfUri.scheme)) {
>   if (!(uri.schemeIs("https") && selfUri.schemeIs("http"))) {
>     cspWarn(aCSPR, CSPLocalizer.getFormatStr("notSameScheme",
> [uri.asciiSpec]));
>   }
>   continue;
> }

Just to be clear, this won't work ("continue" will punt the report sending).

Maybe move the continue inside the inner if or rewrite two ifs into one:

// Scheme mismatch (and not http->https upgrade)
if (!uri.schemeIs(selfUri.scheme) &&
    !(uri.schemeIs("https") && 
      selfUri.schemeIs("http"))) {
  cspWarn(...);
  continue;
}

But you'll also have to take into consideration the port numbers... we should probably ignore the port check as well if there's a http->https upgrade.  So the scheme and port logic will probably both have to be reworked.

Comment 5

[Tanvi Vyas[:tanvi]]

Good point.  We could try something like ..

 let upgradescheme = false;
 if (!uri.schemeIs(selfUri.scheme)) {
   if (!(uri.schemeIs("https") && selfUri.schemeIs("http")))
     cspWarn(aCSPR, CSPLocalizer.getFormatStr("notSameScheme", [uri.asciiSpec]));
     continue;
   } else {
     upgradescheme = true;
   }
 }  
   
 if (uri.port && uri.port !== selfUri.port) {
   if(!(upgradescheme && uri.port==443 && selfUri.port==80)) {  //This if could be combined with the above if.
     cspWarn(aCSPR, CSPLocalizer.getFormatStr("notSamePort", [uri.asciiSpec]));
     continue;
   }
 }

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

The code was structured as a list of "gates" that introduce shortcuts for invalid URIs.  If we introduce an exception, it might be wise to check for that before the shortcuts.

Maybe just connect them using elses?  This ignores the port check if the schemes change in the allowed pattern.

if (uri.schemeIs("https") && selfUri.schemeIs("http")) {
  // log msg for "allowing the http->https upgrade"
}
else if (!uri.schemeIs(selfUri.scheme)) {
  cspWarn(scheme mismatch);
  continue;
}
else if (uri.port && uri.port !== selfUri.port) {
  cspWarn(port mismatch);
  continue;
}


I'm not convinced we need to check exact port numbers for the upgrade case.

Comment 7

[Daniel Veditz [:dveditz]]

The CSP spec has no restriction on the report-uri destination. To comply with the spec none of the proposed code snippets are necessary, we should instead remove all the eTLD goop from
http://dxr.mozilla.org/mozilla-central/source/content/base/src/CSPUtils.jsm#l354

We should probably warn if the scheme is not http or https, better than just letting the POST fail silently later.

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

Attachment: bug843311

Dan: is this patch what you have in mind to become spec compliant?

I updated the CSP report uri parser unit test to allow other origins.  Ran the CSP unit tests and mochitests locally and they passed.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 829614 [details] [diff] [review]
bug843311

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

That looks spec compliant. r+ for the code part; I only skimmed the test changes.

Comment 10

[Garrett Robinson [:grobinson]]

Comment on attachment 829614 [details] [diff] [review]
bug843311

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

The only nit I have is it might be nice to add some tests to test_csputils.js for the initial situation Neil described in Comment 1 (upgrade to other scheme) and the other scenarios that people thought should be valid in other comments.

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch

Looks like if we want to set up an https server to receive reports (like we did for http in test_cspreports.js), we need something like bug 466524 (httpsd.js).

For now I'm just gonna validate that the parser "keeps" the report URIs even if they're cross origin.

Garrett: re-flagging you for review because of the additional tests I added to test_csputils.js.

Comment 12

[Garrett Robinson [:grobinson]]

Comment on attachment 8360031 [details] [diff] [review]
proposed patch

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

Your patch changes the parsing in CSPUtils.jsm for both x- and 1.0 policies, but only tests the x-csp header's handling of report-uri in test_csputils.js. You should have unit tests for the 1.0 parser as well.

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch

Good catch, Garrett.  Since we are deprecating the old policy syntax and since we had defined the old syntax to require stronger limits on report-uris, I just changed the existing tests to use the 1.0 parser and syntax and test for the appropriate "loose" policy.  Does this work for you, Garrett?

Comment 14

[Garrett Robinson [:grobinson]]

Comment on attachment 8364755 [details] [diff] [review]
proposed patch

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

That works for me! Since we're going to have rewrite all of these tests for the new parser, it doesn't make sense to spend too much time on them anyway.

Comment 15

[Sid Stamm [:geekboy or :sstamm]]

Thanks, inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/84802a40e62d

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/84802a40e62d