1418241 - CSP violation: blockedURI

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P3)

Description

[Chung-Sheng Fu [:cfu]]

Some web-platform tests fails due to the blockedURI field in CSP violation reports.  To fix the failures:
1. support "inline" and "eval"
2. do not strip the URI (the algorithm is in CSP Level 2 https://www.w3.org/TR/CSP2/#strip-uri-for-reporting, we have to check the latest standard)

Comment 1

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 1418241 - Fix SecurityPolicyViolationEvent.blockedURI.

Review commit: https://reviewboard.mozilla.org/r/209046/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209046/

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8938275 [details]
Bug 1418241 - Fix SecurityPolicyViolationEvent.blockedURI.

https://reviewboard.mozilla.org/r/209046/#review214818

In theory that change looks correct. However I would imagine that some mochitests and web-platform tests fail/work because of that change, no? Can I see a TRY run?

::: dom/security/nsCSPContext.cpp:668
(Diff revision 1)
> +          selfICString->SetData(nsDependentCString("eval"));
> +          break;
> +        default:
> -      selfICString->SetData(nsDependentCString("self"));
> +          selfICString->SetData(nsDependentCString("self"));
> +          break;
> +      }

nit: rather make this an if-else than a switch since there are only 2 possible cases.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: csp_blockedURI.patch

Can I still this bug?

Comment 4

[Andrea Marchesini [:baku]]

Attachment: csp_blockedURI.patch

Fixed a xpcshell-test failure.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: csp_blockedURI.patch

More WPTs fixed.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8989480 [details] [diff] [review]
csp_blockedURI.patch

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

Hey Baku, thanks for working on this, but for some reason I can't wrap my head around one thing - please answer my question underneath.

If that part is really not needed anymore then we should update the comments within StripURIForReporting. E.g. remove comment for step 2) and update 3) to 2) or something like that.

::: dom/security/nsCSPContext.cpp
@@ -854,5 @@
> -    // cross origin redirects also fall into this category, see:
> -    // http://www.w3.org/TR/CSP/#violation-reports
> -    aURI->GetPrePath(outStrippedURI);
> -    return;
> -  }

Why can we remove this part?

Comment 7

[Andrea Marchesini [:baku]]

Attachment: csp_blockedURI.patch

Comment updated. Answering your question: by spec:

blockedURL "The result of executing the URL serializer on violation’s resource, with the exclude fragment flag set."

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8989717 [details] [diff] [review]
csp_blockedURI.patch

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

I would be fine with that change, but I would like Dan to take another look to make sure that works correctly for redirects. Dan the blockedURI for redirects should be the entire URI modulo the fragment parts. Do we agree here?

Comment 9

[Daniel Veditz [:dveditz]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #8)
> Dan the blockedURI for redirects should be the entire URI modulo
> the fragment parts. Do we agree here?

No, I don't -- and this is important because it's a known Same-origin-policy violation
https://homakov.blogspot.com/2014/01/using-content-security-policy-for-evil.html

The current behavior and comment matches CSP-2
https://w3c.github.io/webappsec-csp/2/#strip-uri-for-reporting

The language quoted in comment 7 comes from CSP-3, but it's not complete. If you follow the link on "resource" to section 2.4 and read on to 2.4.1 it also says we should be reporting the URL --BEFORE REDIRECTS-- when we encounter a violation. This message is repeated in section 7.5 under "Security Considerations".

https://w3c.github.io/webappsec-csp/#create-violation-for-global (2.4.1) contains the following note:

  Note: User agents need to ensure that the source file is
  the URL requested by the page, pre-redirects. If that’s
  not possible, user agents need to strip the URL down to
  an origin to avoid unintentional leakage.

I can't tell from a quick look at the patch but I don't see anything that looks like it's preserving the original URL for reporting so I assume it's still the URL after redirects. In that case we have to keep stripping. If I'm wrong and blocked-uri is the original in-page URL before redirects then stripping just the fragment is OK.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 8989717 [details] [diff] [review]
csp_blockedURI.patch

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

aOriginalURI is passed along as far as nsCSPContext::GatherSecurityPolicyViolationEventData() and then not used in the final reporting. The wasRedirected flag is passed along as far as nsCSPContext::permitsInternal() but there's no way to pass that along to AsyncReportViolation(). If the originalURI isn't used inside AsyncReportViolation then maybe we should just send originalURI as the blockedURI at that point if it's redirected.

Lots of ways to resolve this but we need to do one of them.

I also guess this means the WPT don't have a test of reports after redirection? We should add one of those, too.

unfortunately r-

Comment 11

[Andrea Marchesini [:baku]]

Daniel thanks for you review. I'm happy to work on this issue, but the topic of this bug is just about eval/inline and strip URL ref fragment. Can I work on what you are saying in follow up? Adding a WPT too, of course.

Comment 12

[Daniel Veditz [:dveditz]]

Unfortunately, no, because my complaint is primarily about the stripping the ref fragment. the eval/inline stuff is fine--do we want to split the two issues?

The current behavior that this patch changes matches CSP-2 which is Recommendation status and a perfectly fine thing to match. It doesn't match CSP-3 it's true but if you implement the ref stripping without ALSO doing the "only report the pre-redirect in-document URL" then you introduce a SOP violation that's worse than not matching CSP-3. It is unfortunate that there's no WPT for this important part of the CSP-3 spec change since the change was motivated by security concerns: CSP-2 is technically violating SOP also, but "just the domain" is less bad than full URLs which might have auth tokens or other sensitive parameters.

We can't add the ref-stripping part until we're ready to do it right the whole way, which is more complicated than the patch here.

Comment 13

[Andrea Marchesini [:baku]]

Attachment: csp_blockedURI.patch

Now I see the point. Thanks!
Let's remove the stripping fragment part. I'll file a separate bug for that.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8991822 [details] [diff] [review]
csp_blockedURI.patch

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

Thanks; to sum it up, this changeset only touches the blockedURI in case of inline violations and does not perform any changes to blockedURI in case it's a real nsIURI. That is an improvement over the status quo. r=me

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a96972f2ad9e
CSP violation: blockedURI inline/eval, r=ckerschb

Comment 16

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/a96972f2ad9e