Bug 138492 – CSP is enforced for eval in report-only mode on first page load

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Alexey Proskuryakov]

If a page that disallows eval() in report-only mode is the first page to be loaded in a window, then the policy will actually be enforced.

There are two code path for applying the eval policy. If we have a JS context already, then we apply it right away, checking for whether it report only. But if we didn't have a JS context when parsing the policy yet, then this is delayed until after the context is created. And in this code path, we don't check for report only mode.

rdar://problem/15782525

[Alexey Proskuryakov]

Created attachment 241160 [details]
proposed fix

[WebKit Commit Bot]

Attachment 241160 [details] did not pass style-queue:


ERROR: Source/WebCore/page/ContentSecurityPolicy.cpp:1015:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/page/ContentSecurityPolicy.cpp:1023:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/page/ContentSecurityPolicy.cpp:1031:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/page/ContentSecurityPolicy.cpp:1038:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/page/ContentSecurityPolicy.cpp:1045:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/page/ContentSecurityPolicy.cpp:1054:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/page/ContentSecurityPolicy.cpp:1063:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/page/ContentSecurityPolicy.cpp:1070:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/page/ContentSecurityPolicy.cpp:1077:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/page/ContentSecurityPolicy.cpp:1084:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/page/ContentSecurityPolicy.cpp:1091:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/page/ContentSecurityPolicy.cpp:1098:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/page/ContentSecurityPolicy.cpp:1111:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/page/ContentSecurityPolicy.cpp:1118:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 14 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Comment on attachment 241160 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=241160&action=review

> Source/WebCore/ChangeLog:9
> +        This is covered by existing tests when running as one test per process invocation.

I take it that we don't have test coverage for the CSP script interface since you didn't include any rebased test results in this patch. I suggest that we add test coverage to ensure we don't regress the results returned using the CSP script interface (e.g. document.securityPolicy.allowEval()).

Additional remarks:

Notice that the CSP script interface is only available when building WebKit with ENABLE_CSP_NEXT enabled. We have some existing tests in directory LayoutTests/http/tests/security/contentSecurityPolicy/1.1 (http://trac.webkit.org/browser/trunk/LayoutTests/http/tests/security/contentSecurityPolicy/1.1). For instance, we test the value of document.securityPolicy.allowsEval with different CSP policies in <http://trac.webkit.org/browser/trunk/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/securitypolicy-alloweval.html?rev=133620>. We cannot use the same logic for injecting the report-only CSP policy as we do for other CSP policies because the Content-Security-Policy-Report-Only directive is only honored when specified as an HTTP header according to the note in section Content-Security-Policy-Report-Only Header Field of the Content Security Policy spec (Editor’s Draft, 3 November 2014) <https://w3c.github.io/webappsec/specs/content-security-policy/#content-security-policy-report-only-header-field>.

[Daniel Bates]

(In reply to comment #3)
> I take it that we don't have test coverage for the CSP script interface
> since you didn't include any rebased test results in this patch.

I meant to write:

I take it that we don't have test coverage for the CSP script interface when using a report-only policy since you didn't include any rebased test results in this patch.

> that we add test coverage to ensure we don't regress the results returned
> using the CSP script interface (e.g. document.securityPolicy.allowEval()).
> 
> Additional remarks:
> 
> [...]
> We cannot use the same logic for injecting the report-only CSP policy as we do
> for other CSP policies because the Content-Security-Policy-Report-Only
> directive is only honored when specified as an HTTP header according to the
> note in section Content-Security-Policy-Report-Only Header Field of the
> Content Security Policy spec (Editor’s Draft, 3 November 2014)
> <https://w3c.github.io/webappsec/specs/content-security-policy/#content-
> security-policy-report-only-header-field>.

For completeness, this restriction that the Content-Security-Policy-Report-Only header must be specified as a HTTP header was added in Content Security Policy 1.1 (W3C Working Draft 04 June 2013): <http://www.w3.org/TR/2013/WD-CSP11-20130604/>. That is, earlier drafts recognized the Content-Security-Policy-Report-Only header when specified in a HTML Meta element.

When we choose to update WebKit's implementation of CSP to conform the latest draft and remove our support for recognizing the Content-Security-Policy-Report-Only header when specified using the HTML Meta element then we will need to update the test <http://trac.webkit.org/browser/trunk/LayoutTests/http/tests/security/contentSecurityPolicy/eval-allowed-in-report-only-mode.html?rev=145268> (included in the patch for bug #111867).

[Daniel Bates]

Comment on attachment 241160 [details]
proposed fix

r=me

This patch seems reasonable to me.

Alexey Proskuryakov pointed out that there are issues with our implementation of ENABLE_CSP_NEXT, including discrepancies between the names of functions we expose on document.securityPolicy compared to the spec at the time of writing (e.g. SecurityPolicy.allowEval() vs SecurityPolicy.allowsEval() in the spec and OpenSource code, respectively). He also suspects there are likely build issue when ENABLE_CSP_NEXT is enabled (since there is no OpenSource builder that enables it). Ideally, we should either remove this feature (and existing tests for it) or complete the implementation of it. For now, it seems reasonable to land this patch without additional tests. Should we look to complete the ENABLE_CSP_NEXT feature then we can add more comprehensive test coverage.

[Daniel Bates]

For completeness, the Content Security Policy spec (Editor’s Draft, 3 November 2014), <https://w3c.github.io/webappsec/specs/content-security-policy/#script-interfaces>, doesn't describe the expected behavior of the SecurityPolicy script interface for a report-only CSP policy. Should the script interface behave as if no policy is specified? In particular, should SecurityPolicy.allow{Request, Node, Eval, InlineEventHandler}() always return true for a report-only policy? Or are we looking to carry out the illusion that the report-only CSP policy is being enforced and return appropriate return values? I suspect the former since we ultimately want the server operator to switch from a report-only (non-enforced) CSP policy to an non-report-only (enforced) CSP policy.

[WebKit Commit Bot]

Comment on attachment 241160 [details]
proposed fix

Clearing flags on attachment: 241160

Committed r175771: <http://trac.webkit.org/changeset/175771>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.