Content Security Policy report destination restriction loosening

RESOLVED FIXED in mozilla29

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Neil Matatall, Assigned: geekboy)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla29
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

4 years ago
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.
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM: Core & HTML
Ever confirmed: true
OS: Mac OS X → All
Product: Firefox → Core
Hardware: x86 → All

Updated

4 years ago
Blocks: 493857

Comment 2

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

Comment 4

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

Comment 6

4 years ago
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.
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.
Blocks: 663566
(Assignee)

Comment 8

4 years ago
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.
Assignee: nobody → sstamm
Status: NEW → ASSIGNED
Attachment #829614 - Flags: review?(grobinson)
Attachment #829614 - Flags: review?(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.
Attachment #829614 - Flags: review?(dveditz) → feedback+
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.
Attachment #829614 - Flags: review?(grobinson) → review+
(Assignee)

Comment 11

3 years ago
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.
Attachment #829614 - Attachment is obsolete: true
Attachment #8360031 - Flags: review?(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.
Attachment #8360031 - Flags: review?(grobinson)
(Assignee)

Comment 13

3 years ago
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?
Attachment #8360031 - Attachment is obsolete: true
Attachment #8364755 - Flags: review?(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.
Attachment #8364755 - Flags: review?(grobinson) → review+
(Assignee)

Comment 15

3 years ago
Thanks, inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/84802a40e62d
Flags: in-testsuite+
Target Milestone: --- → mozilla29
https://hg.mozilla.org/mozilla-central/rev/84802a40e62d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Blocks: 963901
Duplicate of this bug: 847069
You need to log in before you can comment on or make changes to this bug.