Last Comment Bug 843311 - Content Security Policy report destination restriction loosening
: Content Security Policy report destination restriction loosening
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla29
Assigned To: Sid Stamm [:geekboy or :sstamm]
:
Mentors:
: 847069 (view as bug list)
Depends on:
Blocks: CSP csp-w3c-1.0 963901
  Show dependency treegraph
 
Reported: 2013-02-20 13:00 PST by Neil Matatall
Modified: 2014-06-26 12:08 PDT (History)
10 users (show)
mozbugs: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
bug843311 (12.01 KB, patch)
2013-11-08 15:40 PST, Sid Stamm [:geekboy or :sstamm]
garrett.f.robinson+mozilla: review+
dveditz: feedback+
Details | Diff | Review
proposed patch (13.32 KB, patch)
2014-01-14 14:04 PST, Sid Stamm [:geekboy or :sstamm]
no flags Details | Diff | Review
proposed patch (14.35 KB, patch)
2014-01-23 16:20 PST, Sid Stamm [:geekboy or :sstamm]
garrett.f.robinson+mozilla: review+
Details | Diff | Review

Description Neil Matatall 2013-02-20 13:00:41 PST
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 2013-02-20 13:12:57 PST
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 2013-02-20 14:07:32 PST
(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 - behind on reviews [:tanvi] 2013-04-17 14:58:46 PDT
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] 2013-04-18 09:18:44 PDT
(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 - behind on reviews [:tanvi] 2013-04-18 11:42:22 PDT
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] 2013-04-18 11:54:15 PDT
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] 2013-06-18 08:39:35 PDT
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] 2013-11-08 15:40:12 PST
Created attachment 829614 [details] [diff] [review]
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] 2013-11-08 16:48:19 PST
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] 2013-11-11 13:59:13 PST
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] 2014-01-14 14:04:34 PST
Created attachment 8360031 [details] [diff] [review]
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] 2014-01-22 16:55:57 PST
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] 2014-01-23 16:20:51 PST
Created attachment 8364755 [details] [diff] [review]
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] 2014-01-24 10:02:42 PST
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] 2014-01-24 10:25:38 PST
Thanks, inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/84802a40e62d
Comment 16 Ryan VanderMeulen [:RyanVM] 2014-01-24 13:56:01 PST
https://hg.mozilla.org/mozilla-central/rev/84802a40e62d
Comment 17 Daniel Veditz [:dveditz] 2014-06-26 12:08:43 PDT
*** Bug 847069 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.