Bug 53867 – Disable script elements when a CSP header is present

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

Disable script elements when a CSP header is present

Created attachment 81394 [details]
Patch

Created attachment 81408 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Source/WebCore/page/ContentSecurityPolicy.cpp:46:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 16 files


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

Created attachment 81409 [details]
Patch

[Adam Barth]

Comment on attachment 81409 [details]
Patch

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

> LayoutTests/http/tests/security/contentSecurityPolicy/no-policy.html:7
> +if (window.layoutTestController) {
> +  layoutTestController.dumpAsText();
> +}

No brackets needed.

> LayoutTests/http/tests/security/contentSecurityPolicy/script-src-in-iframe.html:9
> +<meta http-equiv="refresh" content="0; URL=http://localhost:8000/security/contentSecurityPolicy/resources/echo-iframe.pl?q=http://127.0.0.1:8000/security/contentSecurityPolicy/resources/script-src.html&csp=allow%20*;%20script-src%20'none'" />

Why a meta refresh?  It would be easier to just open a new window or put the test in a frame.

> Source/WebCore/dom/ScriptElement.cpp:161
> +    if (!m_element->document()->contentSecurityPolicy()->allowScriptFromUrl(sourceUrl))
> +        return;

Hum...  I'm not sure this will do the right thing if there's a redirect chain involved.  This code is just checking the first URL in the redirect chain.  We want to check the final URL in the chain...

> Source/WebCore/page/ContentSecurityPolicy.cpp:48
> +    if (m_document->parentDocument() == m_document)
> +        return !m_isEnabled;
> +    return !m_isEnabled && m_document->parentDocument()->contentSecurityPolicy()->allowScriptFromUrl(sourceUrl);

What is all this business about the parent document?  We shouldn't be talking to the parent document at all.  Each resource needs to have its own security policy.

> Source/WebCore/page/ContentSecurityPolicy.h:42
> +    bool allowScriptFromUrl(const String&) const;

allowScriptFromUrl => allowScriptFromURL

> Source/WebCore/page/ContentSecurityPolicy.h:49
>      String m_header;
> +
> +    bool m_isEnabled;
> +
> +    Document* m_document;

A really minor style nit: I'd reverse the order here and remove the blank lines.  Usually, I like to put the "parent object" first, which, in this case, is the document.  Then isEnabled is the most important piece of state (because it controls the behavior of the whole object).

Created attachment 81776 [details]
Patch

This patch doesn't take the redirect chain into account. I guess I need to add another method to HTMLScriptRunnerHost that checks the URL at the end of the redirect chain. will do that in a separate patch.

I had to move the header handling from MainResourceLoader to FrameLoader, as the DocumentWriter deletes the document and creates a new one after the headers are received (and therefore the CSP would get lost).

Created attachment 81778 [details]
Patch

seems like shouldLoadExternalScriptFromSrc is only used for CSP now, so I can move the call to when the response is there

[Adam Barth]

(In reply to comment #9)
> seems like shouldLoadExternalScriptFromSrc is only used for CSP now, so I can move the call to when the response is there

Yeah, I was going to remove shouldLoadExternalScriptFromSrc, but I left it there in case it would be useful for CSP.

(In reply to comment #9)
> seems like shouldLoadExternalScriptFromSrc is only used for CSP now, so I can move the call to when the response is there

related question: is it possible to write a layout test with a redirect chain that includes other hosts than 127.0.0.1?

[Adam Barth]

Comment on attachment 81778 [details]
Patch

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

This gets redirects wrong, of course, but it's a starting point.

> Source/WebCore/page/ContentSecurityPolicy.h:40
> +    // Determines whether the external script should be loaded based on the Content
> +    // Security Policy.

This comment doesn't add any value.  We should just remove it.

[Adam Barth]

It might be nice to add a FIXME about getting redirects right.  Ideally, we'd also add a test that demonstrates the bug.

(In reply to comment #13)
> It might be nice to add a FIXME about getting redirects right.  Ideally, we'd also add a test that demonstrates the bug.

are there existing similar tests?

Since we only got a httpd running on 127.0.0.1:8000, it seems difficult to construct a redirect chain that involves other hosts?

[Adam Barth]

> are there existing similar tests?

Yes.  Look for tests in LayoutTests/http/tests with "redirect" in their name.  There's a PHP script to help.

> Since we only got a httpd running on 127.0.0.1:8000, it seems difficult to construct a redirect chain that involves other hosts?

We usually use localhost as an alternative host name.

[WebKit Review Bot]

Attachment 81778 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7797402

[Build Bot]

Attachment 81778 [details] did not build on win:
Build output: http://queues.webkit.org/results/7791349

[Collabora GTK+ EWS bot]

Attachment 81778 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7829349

[Early Warning System Bot]

Attachment 81778 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7847358

[WebKit Review Bot]

Attachment 81778 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7791365

Created attachment 81807 [details]
Patch

[WebKit Review Bot]

Attachment 81778 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7724412

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 81807 [details]:

http/tests/security/xssAuditor/script-tag-post-null-char.html bug 54110 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 81807 [details]
Patch

Clearing flags on attachment: 81807

Committed r78058: <http://trac.webkit.org/changeset/78058>