Closed
Bug 1026520
Opened 10 years ago
Closed 9 years ago
Erroneous CSP reports for hash-source
Categories
(Core :: DOM: Security, defect, P1)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla43
People
(Reporter: mgoodwin, Assigned: ckerschb)
References
(Blocks 2 open bugs, )
Details
Attachments
(5 files, 2 obsolete files)
15.19 KB,
patch
|
dveditz
:
review+
Sylvestre
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
3.64 KB,
patch
|
dveditz
:
review+
Sylvestre
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
17.50 KB,
patch
|
ckerschb
:
review+
Sylvestre
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
1.75 KB,
patch
|
dveditz
:
review+
Sylvestre
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
210.66 KB,
image/png
|
Details |
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"
Reporter | ||
Comment 1•10 years ago
|
||
It seems reports are also sent if a report-uri is set.
Comment 2•10 years ago
|
||
Is this via Content-Security-Policy-Report-Only headers? This seems suspiciously like report-only behavior plus failure to match the hash.
Assignee | ||
Comment 3•10 years ago
|
||
(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 ".
Reporter | ||
Comment 4•10 years ago
|
||
(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•10 years ago
|
||
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....
Assignee | ||
Comment 6•10 years ago
|
||
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
Flags: needinfo?(sstamm)
Comment 7•10 years ago
|
||
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.)
Flags: needinfo?(sstamm)
Assignee | ||
Comment 8•10 years ago
|
||
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!
Attachment #8449803 -
Flags: review?(sstamm)
Comment 9•10 years ago
|
||
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?
Attachment #8449803 -
Flags: review?(sstamm) → review-
Comment 10•10 years ago
|
||
Comment on attachment 8449803 [details] [diff] [review]
bug_1026520.patch
Yeah, whatever sstamm said.
Attachment #8449803 -
Flags: review-
Comment 11•10 years ago
|
||
Why you gotta be so mean, mt?
Assignee | ||
Comment 12•10 years ago
|
||
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'
Updated•10 years ago
|
Assignee: nobody → francois
Priority: -- → P2
Assignee | ||
Comment 14•10 years ago
|
||
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.
Assignee | ||
Comment 15•10 years ago
|
||
Francois, we are triaging CSP bugs at the moment. Any progress or ideas on this bug?
Flags: needinfo?(francois)
Comment 16•10 years ago
|
||
No progress. I haven't looked at this yet, so I'll unassign myself in case someone else wants to take it on.
Assignee: francois → nobody
Flags: needinfo?(francois)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → mozilla
Priority: P2 → P1
Assignee | ||
Comment 17•9 years ago
|
||
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.
Attachment #8449803 -
Attachment is obsolete: true
Attachment #8654563 -
Flags: review?(dveditz)
Assignee | ||
Comment 18•9 years ago
|
||
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.
Attachment #8654564 -
Flags: review?(dveditz)
Assignee | ||
Comment 19•9 years ago
|
||
Attachment #8654565 -
Flags: review?(dveditz)
Assignee | ||
Updated•9 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Assignee | ||
Comment 20•9 years ago
|
||
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.
Updated•9 years ago
|
Attachment #8654565 -
Flags: review?(dveditz) → review+
Comment 21•9 years ago
|
||
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•9 years ago
|
||
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?
Flags: needinfo?(dveditz)
Comment 26•9 years ago
|
||
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
Attachment #8654563 -
Flags: review?(dveditz) → review+
Comment 27•9 years ago
|
||
Comment on attachment 8654564 [details] [diff] [review]
bug_1026520_inline_callsites.patch
Review of attachment 8654564 [details] [diff] [review]:
-----------------------------------------------------------------
r=dveditz
Attachment #8654564 -
Flags: review?(dveditz) → review+
Updated•9 years ago
|
Flags: needinfo?(dveditz)
Assignee | ||
Comment 28•9 years ago
|
||
(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 :-)
Attachment #8654563 -
Attachment is obsolete: true
Attachment #8662529 -
Flags: review+
Assignee | ||
Comment 29•9 years ago
|
||
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!
Attachment #8662668 -
Flags: review?(dveditz)
Comment 30•9 years ago
|
||
Comment on attachment 8662668 [details] [diff] [review]
bug_1026520_web_platform_test_updates.patch
Review of attachment 8662668 [details] [diff] [review]:
-----------------------------------------------------------------
r=dveditz
Attachment #8662668 -
Flags: review?(dveditz) → review+
Comment 31•9 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Assignee | ||
Comment 34•9 years ago
|
||
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
Attachment #8654564 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 35•9 years ago
|
||
Comment on attachment 8654565 [details] [diff] [review]
bug_1026520_inline_test_updates.patch
Approval Request Comment
See comment 34.
Attachment #8654565 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 36•9 years ago
|
||
Comment on attachment 8662529 [details] [diff] [review]
bug_1026520_inline_csp_changes.patch
Approval Request Comment
See comment 34.
Attachment #8662529 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 37•9 years ago
|
||
Comment on attachment 8662668 [details] [diff] [review]
bug_1026520_web_platform_test_updates.patch
Approval Request Comment
See comment 34.
Attachment #8662668 -
Flags: approval-mozilla-beta?
Updated•9 years ago
|
Comment 38•9 years ago
|
||
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.
Attachment #8654564 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Updated•9 years ago
|
Attachment #8654565 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Updated•9 years ago
|
Attachment #8662529 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Updated•9 years ago
|
Attachment #8662668 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment 39•9 years ago
|
||
This issue looks like it's still happening for me on FF 44 on Mac Osx 10.10.5
Comment 40•9 years ago
|
||
screenshot of test page with CSP warning despite use of nonce.
Assignee | ||
Comment 41•9 years ago
|
||
(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?
Flags: needinfo?(afanen01)
Comment 42•9 years ago
|
||
Hi, Access this page: https://nucco.org/files/login.php
Assignee | ||
Comment 44•9 years ago
|
||
(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.
Flags: needinfo?(mozilla)
Flags: needinfo?(afanen01)
Comment 45•9 years ago
|
||
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.
Assignee | ||
Comment 46•9 years ago
|
||
(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!
You need to log in
before you can comment on or make changes to this bug.
Description
•