1026520 - Erroneous CSP reports for hash-source

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Mark Goodwin [:mgoodwin]]

Issue:
Firefox reports violations for documents that comply with the provided CSP if hash-sources are used.

STR
1) open web console
2) Visit a document with a script-src directive containing at least one hash-source (see URL)
3) Observe a) the scripts executing (as they should) and b) the console messages "Content Security Policy: The page's settings blocked the loading of a resource: An attempt to execute inline scripts has been blocked"

Comment 1

[Mark Goodwin [:mgoodwin]]

It seems reports are also sent if a report-uri is set.

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

Is this via Content-Security-Policy-Report-Only headers?  This seems suspiciously like report-only behavior plus failure to match the hash.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #2)
> Is this via Content-Security-Policy-Report-Only headers?  This seems
> suspiciously like report-only behavior plus failure to match the hash.

If it's report-only then the console message should be
 "The page's settings observed the loading of a resource at "
and not
 "The page's settings blocked the loading of ".

Comment 4

[Mark Goodwin [:mgoodwin]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #2)
> Is this via Content-Security-Policy-Report-Only headers?  This seems
> suspiciously like report-only behavior plus failure to match the hash.

no, not report only. Also the hash matches. It's kinda weird.

For example: https://people.mozilla.org/~mgoodwin/csp/test2.html has three scripts - two scripts match the hash-sources, one doesn't. Violations are shown (and reported) for all three (you can check the response header and reports in the devtools).

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

ckerschb: we found at least one other case in the new impl where the wrong message was being used on the console for report-only violations.  

How are you validating the hashes?  Via printfs or other debugging in gecko?  Maybe there's an error....

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Here it goes:
The matching of hashes in nsCSPHashSrc::allows works correctly, the problem is in nsScriptloader.cpp [1].
First we call GetAllowsInlineScript which returns false and hence appends an element to the violations-vector.
The variable allowInlineScript is still false till we hit GetAllowsHash which finally allows the load by setting allowInlineScript to true.

So, now the hot-sauce, the violations-vector is not empty, and therefore a call to LogViolationDetails gets triggered, which incorrectly displays the message in the console.

My suggestion, only report if allowInlineScript is still false:

>   // every VIOLATION_TYPE_NONCE_SCRIPT and VIOLATION_TYPE_HASH_SCRIPT are also
>   // VIOLATION_TYPE_INLINE_SCRIPT, but reporting the
>   // VIOLATION_TYPE_INLINE_SCRIPT is redundant and does not help the developer.
> -  if (!violations.IsEmpty()) {
> +  if (!allowInlineScript && !violations.IsEmpty()) {
>     MOZ_ASSERT(violations[0] == nsIContentSecurityPolicy::VIOLATION_TYPE_INLINE_SCRIPT,
>                "How did we get any violations without an initial inline script violation?");

Sid, do you agree?

[1] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsScriptLoader.cpp#457

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

It would be cleaner if we removed the VIOLATION_TYPE_INLINE_SCRIPT from the violations vector (so that var always contains the relevant violations).  Or better yet, just move the "if (reportInlineViolation)" block that adds the INLINE_SCRIPT violation to the vector down after the nonce/hash if blocks.  Pseudocode:

getAllowsInlineScript(&reportInlineViolation, &allowInlineScript);

if (!allowInlineScript) {
  // do the nonce check and append to the violations vector if appropriate
}

if (!allowInlineScript) {
  // do the hash check and append to the violations if appropriate);
}

if (!allowInlineScript && reportInlineViolation) {
  // inline script is totally not allowed by anything, add inline violation to vector
}

If that is easy to do, that's a better fix that makes the code easier to read and makes it obvious that an inline script violation ONLY gets reported when nonce and hash both said "no".

(Also, if you make a patch, please ensure all the booleans are explicitly initialized to false or true or whatever makes most sense.)

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1026520.patch

I ended up refactoring the whole function. I think the we are now more explicit about why or why not we are allowing/blocking an inline script.

With this patch applied I pass all mochis, and the two tests Mark provided perform as expected. No console message for the first test and one message for the second test - as expected.

Thanks a lot Mark for revealing the problem and providing the testcases!

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8449803 [details] [diff] [review]
bug_1026520.patch

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

Fix the report-only mode early return issue and re-flag me please.  Mostly looks good.

::: content/base/src/nsScriptLoader.cpp
@@ +447,5 @@
> +  /* An inline script can be allowed because
> +   *   a) all inline scripts are allowed,
> +   *   b) it is whitelisted by a nonce-source, or
> +   *   c) it is whitelisted by a hash-source.
> +   */

Nit: please use line comments as the rest of this file tends towards that for in-function blocks.

@@ +450,5 @@
> +   *   c) it is whitelisted by a hash-source.
> +   */
> +  bool allowInlineScript = false;
> +  bool reportViolation = false;
> +  unsigned short violationType = 0;

Please use a constant for the initializer like VIOLATION_TYPE_INLINE_SCRIPT to make this more explicit or add a comment saying "// 0 means no violation".

@@ +458,3 @@
>    NS_ENSURE_SUCCESS(rv, false);
> +  if (allowInlineScript) {
> +    return true;

This should not be an early return: in report-only mode, the policy might prohibit the inline script yet allowInlineScript will be true since it's report-only.  This is the same for the hash and nonce cases below.

http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIContentSecurityPolicy.idl#77

@@ +509,5 @@
>        scriptSample.AppendLiteral("...");
>      }
> +    csp->LogViolationDetails(violationType, NS_ConvertUTF8toUTF16(asciiSpec),
> +                             scriptSample, aElement->GetScriptLineNumber(),
> +                             nonce, scriptText);

To be absolutely clear, this assumes there will be only one violation that is one of {INLINE_SCRIPT, HASH, NONCE}, right?

Comment 10

[Martin Thomson [:mt:]]

Comment on attachment 8449803 [details] [diff] [review]
bug_1026520.patch

Yeah, whatever sstamm said.

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

Why you gotta be so mean, mt?

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

The problem seems to be more difficult than we initially thought because in nsScriptLoader we can not differentiate between a policy and a report-only-policy. Hence we can not simply overrule the outcome of unsafe-inline with hash, or nonce. Consider the following scenario:

> policy-ro: script-src: example.com
> policy: script-src 'nonce-asdf'

We want the report to be sent for the policy-ro.

In a different scenario we have the following policy:
policy: script-src: example.com 'nonce-asdf'

in which case we do not want to report a violation for script-src since the 'nonce' overrules the outcome and allows the load.

The problem is that getAllows-function-family currently can not tell the caller whether it's a RO policy or not - it just tells you 'yes' and 'no'

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Francois, the description in Bug 1125565 is quite comprehensive. You might want to check it out in the other bug as well when fixing this one.

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Francois, we are triaging CSP bugs at the moment. Any progress or ideas on this bug?

Comment 16

[François Marier [:francois]]

No progress. I haven't looked at this yet, so I'll unassign myself in case someone else wants to take it on.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1026520_inline_csp_changes.patch

Dan, instead of having each callsite check for unsafe-inline, nonce-src, and hash-src I encapsulated those checks within CSP. The reason we haven't done that so far is still legacy from introducing hash and nonce. Anyway, the most important part is this:

> +  for (uint32_t i = 0; i < mPolicies.Length(); i++) {
> +    bool allowed =
> +      mPolicies[i]->allows(aContentType, CSP_UNSAFE_INLINE, EmptyString()) ||
> +      mPolicies[i]->allows(aContentType, CSP_NONCE, aNonce) ||
> +      mPolicies[i]->allows(aContentType, CSP_HASH, aContent);

which basically allows us to check if either unsafe-inline, hash or nonce allows the inline script/style and only if none of those three checks succeeds we report and error and log a message to the console.

I will file a follow up to eliminate LogViolationDetails completely and will also inline the reporting for unsafeEval within that follow up bug.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1026520_inline_callsites.patch

I think it's fine that you review those callsite changes as well, otherwise we have to find some dom peer, but I think it's fine you reviewing those changes.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1026520_inline_test_updates.patch

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

I filed Bug 1026520 which will completely encapsulate error reporting (remove LogViolationDetails) within CSP so that no callsite has to perform that additional and error prone work.

Comment 21

[Barry]

If you include a Hash and unsafe-inline, then Firefox reports the following:
  Content Security Policy: Ignoring "'unsafe-inline'" within script-src: nonce-source or hash-source specified
But then, due to this bug, it fails to validate the hash properly, so you're stuck :-(

Depending on how complex this change is, and when this release is scheduled for, should Firefox stop above ignore option in the mean time?

This would allow you to put unsafe-inline AND the hash.
* Chrome could use the hash (as it does work there and it also ignores unsafe-inline when hash exists).
* Firefox could fall back to using unsafe-inline for now.
Currently I'm unable to use hash, even for Chrome users, due to a combination of this bug and the ignore message.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Daniel, we seem to have a bunch of duplicates of this bug (at least in its "nonce use causes violation reports even though the script runs" form)... do you know when you'll get to the review?

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 8654563 [details] [diff] [review]
bug_1026520_inline_csp_changes.patch

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

r=dveditz

I have some concerns about over-reporting, but looking at the interfaces on the old methods this is not a problem introduced by this patch.

::: dom/security/nsCSPContext.cpp
@@ -380,2 @@
>  NS_IMETHODIMP
> -nsCSPContext::getAllowsInternal(nsContentPolicyType aContentType,

This shouldn't have been NS_IMETHODIMP because it's not in the interface, but it wasn't doing any harm (and now it'll be gone).

@@ +388,5 @@
>        // policy is violated: must report the violation and allow the inline
>        // script if the policy is report-only.
>        *outShouldReportViolation = true;
>        if (!mPolicies[i]->getReportOnlyFlag()) {
> +        *outAllowsEval = false;

It looks like you'll send the eval violation to the report-uri of all policies, not just the ones that don't allow eval. Is that true? I notice that GetAllowsInline() tries to do the correct thing and pass the policy index through to AsyncReportViolation(). [I haven't checked to see if that method is doing the right thing with it, however.]

@@ +410,5 @@
> +  // if the nonce is non empty, then we report the nonce error, otherwise
> +  // let's report the hash error; no need to report the unsafe-inline error
> +  // anymore.
> +  if (!aNonce.IsEmpty()) {
> +    violationType = aContentType == nsIContentPolicy::TYPE_SCRIPT

I would prefer parens around the equality (here and elsewhere, I won't keep mentioning it), but if the rest of the file doesn't then "when in Rome..." wins

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 8654564 [details] [diff] [review]
bug_1026520_inline_callsites.patch

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

r=dveditz

Comment 28

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1026520_inline_csp_changes.patch

(In reply to Daniel Veditz [:dveditz] from comment #26)
> @@ +388,5 @@
> >        if (!mPolicies[i]->getReportOnlyFlag()) {
> > +        *outAllowsEval = false;
> 
> It looks like you'll send the eval violation to the report-uri of all
> policies, not just the ones that don't allow eval. Is that true? I notice
> that GetAllowsInline() tries to do the correct thing and pass the policy
> index through to AsyncReportViolation(). [I haven't checked to see if that
> method is doing the right thing with it, however.]

Back in the days when CSP was still implemented within JS, we used LogViolationDetails to send reports. This patch cleans up all 'inline' calls and Bug 1199977 will eliminate LogViolationDetails for 'eval'. With this patch we haven't changed any logic for eval, we will do that within Bug 1199977, but that's not that urgent. I don't think we are overreporting for eval, we are just reporting back that reports should be sent and then LogViolationDetails selects the policies that should receive a report.

> @@ +410,5 @@
> > +  if (!aNonce.IsEmpty()) {
> > +    violationType = aContentType == nsIContentPolicy::TYPE_SCRIPT
> 
> I would prefer parens around the equality (here and elsewhere, I won't keep
> mentioning it), but if the rest of the file doesn't then "when in Rome..."
> wins

Usually I use parans and then reviewers requested no parans :-) I am more with you on that topic hence I added parans :-)

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1026520_web_platform_test_updates.patch

Dan, it seems we are passing more csp web-platform tests now. We can remove those three *.ini files since we are passing those tests. I manually verified that the our system behaves as it should before I flagged you for review. Looking at the test description also highlights that we are now doing the right thing:

> This tests the effect of a valid script-hash value.
> It passes if no CSP violation is generated, and the alert_assert() is executed.

Which means, now we are not generating a CSP violation anymore. Looks good to me!

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 8662668 [details] [diff] [review]
bug_1026520_web_platform_test_updates.patch

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

r=dveditz

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f860449c1a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/19eb4586448f
https://hg.mozilla.org/integration/mozilla-inbound/rev/778c2ddda443
https://hg.mozilla.org/integration/mozilla-inbound/rev/4821738e72af

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8f860449c1a9
https://hg.mozilla.org/mozilla-central/rev/19eb4586448f
https://hg.mozilla.org/mozilla-central/rev/778c2ddda443
https://hg.mozilla.org/mozilla-central/rev/4821738e72af

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8654564 [details] [diff] [review]
bug_1026520_inline_callsites.patch

Approval Request Comment
To follow the CSP spec as well as for security purposes 'unsafe-inline' should be ignored if hash or nonce is specified. We landed that fix in Bug 1004703 which in turn made the known problem of overreporting CSP failures more problematic so that even GMail had to fall back and ship using 'unsafe-inline' rather than using hashes or nonces. We should encourage the support of hashes are nonces however, hence we should uplift that patch to Beta to allow people using hashes or nonces without being swamped with incorrect error reports.

[Feature/regressing bug #]:
https://bugzilla.mozilla.org/show_bug.cgi?id=1004703

[User impact if declined]:
a) Firefox users get incorrect console messages claiming that inline scripts were blocked by CSP but in fact they were not.
b) Web sites authors using CSP get swamped with incorrect error reports.

[Describe test coverage new/current, TreeHerder]:
Manually tests using Mark Goodwin's testpage as well as other test pages including updating the tests on treeherder (see other patches in this bug).

[Risks and why]: 
Low, because the fix only affects web pages that do ship CSP and the fix only affects error reporting to the console as well as back to the web page.

[String/UUID change made/needed]:
Yes

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8654565 [details] [diff] [review]
bug_1026520_inline_test_updates.patch

Approval Request Comment
See comment 34.

Comment 36

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8662529 [details] [diff] [review]
bug_1026520_inline_csp_changes.patch

Approval Request Comment
See comment 34.

Comment 37

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8662668 [details] [diff] [review]
bug_1026520_web_platform_test_updates.patch

Approval Request Comment
See comment 34.

Comment 38

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8654564 [details] [diff] [review]
bug_1026520_inline_callsites.patch

I prefer if this change rides the train, patches are pretty big, we had this bug for a while, 42 is a special release, no need to uplift that to beta.

Comment 39

[123Knuckles]

This issue looks like it's still happening for me on FF 44 on Mac Osx 10.10.5

Comment 40

[123Knuckles]

Attachment: Screen Shot 2016-01-29 at 09.58.24.png

screenshot of test page with CSP warning despite use of nonce.

Comment 41

[Christoph Kerschbaumer [:ckerschb]]

(In reply to FKnuckles from comment #40)
> Created attachment 8713538 [details]
> Screen Shot 2016-01-29 at 09.58.24.png
> 
> screenshot of test page with CSP warning despite use of nonce.

Huh, really? Are you sure? Got a testcase online somewhere so I can verify the problem and fix it?

Comment 42

[123Knuckles]

Hi, Access this page: https://nucco.org/files/login.php

Comment 43

[123Knuckles]

see comment #42

Comment 44

[Christoph Kerschbaumer [:ckerschb]]

(In reply to FKnuckles from comment #42)
> Hi, Access this page: https://nucco.org/files/login.php

Thanks for the testpage, but I can't reproduce the problem using Linux and Mac. Have you tried reproducing the problem using the link above, or just your local version? Please also try to clear the web console and also try to use 'shift refresh' to make sure you are not accessing some cached version. If the problem still exists, please let me know and i'll have a closer look once again.

Comment 45

[123Knuckles]

I tested in Ubuntu and didn't reproduce this, and so I started adding extensions one after the other, and the issue seems to be introduced by the LastPass Password manager extension. 

This looks like a lastPass bug. I'll report it to them. 


(In reply to Christoph Kerschbaumer [:ckerschb] from comment #44)
> (In reply to FKnuckles from comment #42)
> > Hi, Access this page: https://nucco.org/files/login.php
> 
> Thanks for the testpage, but I can't reproduce the problem using Linux and
> Mac. Have you tried reproducing the problem using the link above, or just
> your local version? Please also try to clear the web console and also try to
> use 'shift refresh' to make sure you are not accessing some cached version.
> If the problem still exists, please let me know and i'll have a closer look
> once again.

Comment 46

[Christoph Kerschbaumer [:ckerschb]]

(In reply to FKnuckles from comment #45)
> I tested in Ubuntu and didn't reproduce this, and so I started adding
> extensions one after the other, and the issue seems to be introduced by the
> LastPass Password manager extension. 
> 
> This looks like a lastPass bug. I'll report it to them. 

Right on - thanks for reporting the problem anyway - glad that we got it resolved!