Bug 53867

Summary: Disable script elements when a CSP header is present
Product: WebKit Reporter: jochen
Component: New BugsAssignee: jochen
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, buildbot, commit-queue, dglazkov, gustavo.noronha, gustavo, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 53572    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
abarth: review+
Patch none

Description jochen 2011-02-06 01:16:57 PST
Disable script elements when a CSP header is present
Comment 1 jochen 2011-02-06 01:17:33 PST
Created attachment 81394 [details]
Patch
Comment 2 jochen 2011-02-06 08:35:05 PST
Created attachment 81408 [details]
Patch
Comment 3 WebKit Review Bot 2011-02-06 08:37:02 PST
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.
Comment 4 jochen 2011-02-06 08:46:02 PST
Created attachment 81409 [details]
Patch
Comment 5 Adam Barth 2011-02-06 10:21:49 PST
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).
Comment 6 jochen 2011-02-09 02:20:20 PST
Created attachment 81776 [details]
Patch
Comment 7 jochen 2011-02-09 02:23:17 PST
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).
Comment 8 jochen 2011-02-09 02:27:29 PST
Created attachment 81778 [details]
Patch
Comment 9 jochen 2011-02-09 02:29:30 PST
seems like shouldLoadExternalScriptFromSrc is only used for CSP now, so I can move the call to when the response is there
Comment 10 Adam Barth 2011-02-09 02:31:33 PST
(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.
Comment 11 jochen 2011-02-09 02:33:07 PST
(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?
Comment 12 Adam Barth 2011-02-09 02:33:48 PST
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.
Comment 13 Adam Barth 2011-02-09 02:34:16 PST
It might be nice to add a FIXME about getting redirects right.  Ideally, we'd also add a test that demonstrates the bug.
Comment 14 jochen 2011-02-09 02:36:22 PST
(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?
Comment 15 Adam Barth 2011-02-09 02:37:48 PST
> 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.
Comment 16 WebKit Review Bot 2011-02-09 02:46:26 PST
Attachment 81778 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7797402
Comment 17 Build Bot 2011-02-09 03:18:56 PST
Attachment 81778 [details] did not build on win:
Build output: http://queues.webkit.org/results/7791349
Comment 18 Collabora GTK+ EWS bot 2011-02-09 03:21:31 PST
Attachment 81778 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7829349
Comment 19 Early Warning System Bot 2011-02-09 03:35:26 PST
Attachment 81778 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7847358
Comment 20 WebKit Review Bot 2011-02-09 04:52:23 PST
Attachment 81778 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7791365
Comment 21 jochen 2011-02-09 07:06:34 PST
Created attachment 81807 [details]
Patch
Comment 22 WebKit Review Bot 2011-02-09 07:24:23 PST
Attachment 81778 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7724412
Comment 23 WebKit Commit Bot 2011-02-09 09:02:08 PST
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: dbates@webkit.org)
The commit-queue is continuing to process your patch.
Comment 24 WebKit Commit Bot 2011-02-09 09:03:52 PST
Comment on attachment 81807 [details]
Patch

Clearing flags on attachment: 81807

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