CSP: Strip the fragment from reported URLs.

CSP: Strip the fragment from reported URLs.

We should have been stripping the fragment from the URL we report for
CSP violations, but we weren't. Now we are, by running the URLs through
`stripURLForUseInReport()`, which implements the stripping algorithm
from CSP2: https://www.w3.org/TR/CSP2/#strip-uri-for-reporting

Eventually, we will migrate more completely to the CSP3 world that
doesn't require such detailed stripping, as it exposes less data to the
reports, but we're not there yet.

BUG=678776
Review-Url: https://codereview.chromium.org/2619783002
Cr-Commit-Position: refs/heads/master@{#458045}
Committed: https://chromium.googlesource.com/chromium/src/+/fea16c8b60ff3d0756d5eb392394963b647bc41a

[Mike West, 2017-01-09 12:33:38]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 12:33:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mike West, 2017-01-09 12:34:15]

mkwst@chromium.org changed reviewers:
+ elawrence@chromium.org

[Mike West, 2017-01-09 12:34:15]

Eric, mind taking a look at this? (Should I have let you upload a fix? Are you
interested in doing that?)

[commit-bot: I haz the power, 2017-01-09 14:15:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-09 14:15:52]

Dry run: This issue passed the CQ dry run.

[elawrence, 2017-01-09 16:34:17]

On 2017/01/09 12:34:15, Mike West (sloooooow) wrote:
> Eric, mind taking a look at this? (Should I have let you upload a fix? Are you
> interested in doing that?)

I probably would've taken a lot longer to find the right place for the fix, as
my instinct would've been to put sanitization at the point of use inside
postViolationReport.

I still wonder if that might be better? Sanitizing in
gatherSecurityPolicyViolationEventData looks to me like it will impact the data
reported to the securitypolicyviolation event on the DOM (via
ContentSecurityPolicy::dispatchViolationEvents). A quick scan of the
specification seems to imply that sanitization is only expected when a report is
POSTed to a reporting endpoint?

[elawrence, 2017-01-09 16:34:42]

https://codereview.chromium.org/2619783002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/2619783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1029:
init.setDocumentURI(blockedURL.strippedForUseAsReferrer());
In the stripURLForUseInReport() function immediately prior, we note

// 'KURL::strippedForUseAsReferrer()' dumps 'String()' for non-webby URLs.
// It's better for developers if we return the origin of those URLs rather than
nothing.

I don't know if that concern applies here?

https://codereview.chromium.org/2619783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1065:
init.setReferrer(document->referrer());
The algorithm described here
(https://www.w3.org/TR/CSP/#deprecated-serialize-violation) says that the
Referer should also be stripped. Is that not necessary because a Referrer
inherently has already been sanitized?

https://codereview.chromium.org/2619783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1159: // sent
explicitly. As for which directive was violated, that's pretty
The justification for allowing HTTPS URIs to leak out over HTTP seems very weak
to me. Yes, the report is sent "explicitly" but probably without awareness of
the web developer, who didn't expect a script injection hole in his website to
begin with, and thus didn't expect the report to ever fire. We're allowing a
webdev to convert XSS into an InfoLeak, which may not be much better.

Allowing HTTP reporting endpoints for HTTPS pages seems like a rotten idea.

[Mike West, 2017-02-23 08:41:19]

I lost track of this, sorry. Mind taking another look, Eric?

https://codereview.chromium.org/2619783002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/2619783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1029:
init.setDocumentURI(blockedURL.strippedForUseAsReferrer());
On 2017/01/09 at 16:34:41, elawrence wrote:
> In the stripURLForUseInReport() function immediately prior, we note
> 
> // 'KURL::strippedForUseAsReferrer()' dumps 'String()' for non-webby URLs.
> // It's better for developers if we return the origin of those URLs rather
than nothing.
> 
> I don't know if that concern applies here?

For the `document-uri`, that's less of a concern, as it's quite difficult to
load any non-webby URL in the content area of Chrome. It would make reports from
`ftp:`, `chrome:`, and `chrome-extension:` URLs less useful, but we also don't
actually send reports for those URLs (as there are no headers, and therefore no
ability to set `report-uri` in the policy).

Still, it's worth being consistent. I'll adjust. :)

https://codereview.chromium.org/2619783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1065:
init.setReferrer(document->referrer());
On 2017/01/09 at 16:34:41, elawrence wrote:
> The algorithm described here
(https://www.w3.org/TR/CSP/#deprecated-serialize-violation) says that the
Referer should also be stripped. Is that not necessary because a Referrer
inherently has already been sanitized?

Yes.

https://codereview.chromium.org/2619783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1159: // sent
explicitly. As for which directive was violated, that's pretty
On 2017/01/09 at 16:34:41, elawrence wrote:
> The justification for allowing HTTPS URIs to leak out over HTTP seems very
weak to me. Yes, the report is sent "explicitly" but probably without awareness
of the web developer, who didn't expect a script injection hole in his website
to begin with, and thus didn't expect the report to ever fire. We're allowing a
webdev to convert XSS into an InfoLeak, which may not be much better.
> 
> Allowing HTTP reporting endpoints for HTTPS pages seems like a rotten idea.

I agree. I'm sure this was a good idea at the time, but it's a bad idea today.
Filed a bug, added a TODO. We should deprecate/remove that behavior.

[Mike West, 2017-02-23 08:41:25]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 08:41:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 10:32:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 10:32:09]

Dry run: This issue passed the CQ dry run.

[elawrence, 2017-02-24 02:00:41]

Thanks for the updates-- the code changes look good and I'm happy to have a bug
to deprecate the use of HTTP report URIs.

I wanted to make sure you saw my earlier question that wasn't attached to a line
of code: "Sanitizing in gatherSecurityPolicyViolationEventData looks to me like
it will impact the data reported to the securitypolicyviolation event on the DOM
(viaContentSecurityPolicy::dispatchViolationEvents). A quick scan of the
specification seems to imply that sanitization is only expected when a report is
POSTed to a reporting endpoint?"

Is that not a problem?

Thanks!

[elawrence, 2017-03-13 19:28:00]

Hey, Mike-- Are you still thinking about this one? We had another bug filed
against Referer leakage here.

On 2017/02/24 02:00:41, elawrence wrote:
> Thanks for the updates-- the code changes look good and I'm happy to have a
bug
> to deprecate the use of HTTP report URIs.
> 
> I wanted to make sure you saw my earlier question that wasn't attached to a
line
> of code: "Sanitizing in gatherSecurityPolicyViolationEventData looks to me
like
> it will impact the data reported to the securitypolicyviolation event on the
DOM
> (viaContentSecurityPolicy::dispatchViolationEvents). A quick scan of the
> specification seems to imply that sanitization is only expected when a report
is
> POSTed to a reporting endpoint?"
> 
> Is that not a problem?
> 
> Thanks!

[Mike West, 2017-03-17 13:20:40]

On 2017/03/13 at 19:28:00, elawrence wrote:
> Hey, Mike-- Are you still thinking about this one? We had another bug filed
against Referer leakage here.

I was not! Now I am. :) The last few weeks have been hectic, sorry.

This was a bug in the spec, thanks for pointing it out! The goal is to keep
things consistent between the reporting mechanisms. We don't send fragments to
the server, generally, and that seems like a good invariant for the reporting
system in general. I've updated the spec in
https://github.com/w3c/webappsec-csp/commit/c60db267c4f295d50652a6ca24d825059....

Rebased, let's see if the bots still like the patch.

[Mike West, 2017-03-17 13:21:43]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 13:22:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 14:55:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 14:55:53]

Dry run: This issue passed the CQ dry run.

[Mike West, 2017-03-20 08:51:10]

mkwst@chromium.org changed reviewers:
+ yoav@yoav.ws

[Mike West, 2017-03-20 08:51:11]

Yoav, would you mind stamping this?

[Yoav Weiss, 2017-03-20 09:17:57]

On 2017/03/20 08:51:11, Mike West (Slow.) wrote:
> Yoav, would you mind stamping this?

LGTM

Seems like stripURLForUseInReport() does more than just removing the fragment
(e.g. makes sure we don't leak file:// paths, ,and avoids cross-origin frame-src
and object-src violations).
Should that be reflected in the issue's description?

[Mike West, 2017-03-20 11:05:53]

Description was changed from

==========
CSP: Strip the fragment from reported URLs.

We should have been stripping the fragment from the URL we report for
CSP violations, but we weren't. Now we are.

BUG=678776
==========

to

==========
CSP: Strip the fragment from reported URLs.

We should have been stripping the fragment from the URL we report for
CSP violations, but we weren't. Now we are, by running the URLs through
`stripURLForUseInReport()`, which implements the stripping algorithm
from CSP2: https://www.w3.org/TR/CSP2/#strip-uri-for-reporting

Eventually, we will migrate more completely to the CSP3 world that
doesn't require such detailed stripping, as it exposes less data to the
reports, but we're not there yet.

BUG=678776
==========

[Mike West, 2017-03-20 11:06:30]

Done, thanks! In the happy future, we won't need `stripURLForUseInReport()`, as
we won't be leaking cross-origin data through violation reports, but we're not
there yet.

[Mike West, 2017-03-20 11:06:32]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2017-03-20 11:06:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-20 12:42:15]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490007992049800,
"parent_rev": "61398872dd226429fc77c0797912cc1ef436d5cb", "commit_rev":
"fea16c8b60ff3d0756d5eb392394963b647bc41a"}

[commit-bot: I haz the power, 2017-03-20 12:43:15]

Description was changed from

==========
CSP: Strip the fragment from reported URLs.

We should have been stripping the fragment from the URL we report for
CSP violations, but we weren't. Now we are, by running the URLs through
`stripURLForUseInReport()`, which implements the stripping algorithm
from CSP2: https://www.w3.org/TR/CSP2/#strip-uri-for-reporting

Eventually, we will migrate more completely to the CSP3 world that
doesn't require such detailed stripping, as it exposes less data to the
reports, but we're not there yet.

BUG=678776
==========

to

==========
CSP: Strip the fragment from reported URLs.

We should have been stripping the fragment from the URL we report for
CSP violations, but we weren't. Now we are, by running the URLs through
`stripURLForUseInReport()`, which implements the stripping algorithm
from CSP2: https://www.w3.org/TR/CSP2/#strip-uri-for-reporting

Eventually, we will migrate more completely to the CSP3 world that
doesn't require such detailed stripping, as it exposes less data to the
reports, but we're not there yet.

BUG=678776

Review-Url: https://codereview.chromium.org/2619783002
Cr-Commit-Position: refs/heads/master@{#458045}
Committed:
https://chromium.googlesource.com/chromium/src/+/fea16c8b60ff3d0756d5eb392394...
==========

[commit-bot: I haz the power, 2017-03-20 12:43:16]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/fea16c8b60ff3d0756d5eb392394...