Summary: | Disable script elements when a CSP header is present | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | jochen | ||||||||||||||
Component: | New Bugs | Assignee: | 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
jochen
2011-02-06 01:16:57 PST
Created attachment 81394 [details]
Patch
Created attachment 81408 [details]
Patch
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
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 (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? 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. 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? > 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. Attachment 81778 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7797402 Attachment 81778 [details] did not build on win: Build output: http://queues.webkit.org/results/7791349 Attachment 81778 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7829349 Attachment 81778 [details] did not build on qt: Build output: http://queues.webkit.org/results/7847358 Attachment 81778 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7791365 Created attachment 81807 [details]
Patch
Attachment 81778 [details] did not build on mac: Build output: http://queues.webkit.org/results/7724412 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 on attachment 81807 [details] Patch Clearing flags on attachment: 81807 Committed r78058: <http://trac.webkit.org/changeset/78058> |