Make sure OOPIFs are compatible with plugin-types CSP |
||||||
Issue description
We have some code that only enforces CSP checks for local frames:
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:
bool ContentSecurityPolicy::allowPluginTypeForDocument(const Document& document, const String& type, const String& typeAttribute, const KURL& url, ContentSecurityPolicy::ReportingStatus reportingStatus) const
{
if (document.contentSecurityPolicy() && !document.contentSecurityPolicy()->allowPluginType(type, typeAttribute, url))
return false;
// CSP says that a plugin document in a nested browsing context should
// inherit the plugin-types of its parent.
//
// FIXME: The plugin-types directive should be pushed down into the
// current document instead of reaching up to the parent for it here.
LocalFrame* frame = document.frame();
if (.... frame->tree().parent()->isLocalFrame() && document.isPluginDocument()) {
ContentSecurityPolicy* parentCSP = toLocalFrame(frame->tree().parent())->document()->contentSecurityPolicy();
if (parentCSP && !parentCSP->allowPluginType(type, typeAttribute, url))
return false;
}
return true;
}
Let's use this bug to make sure CSP checks work for OOPIFs for all cases of plugin-types enforcement.
,
Aug 2 2016
The observations above come from loading the following document <!DOCTYPE html> <html> <head> <meta http-equiv="Content-Security-Policy" content="plugin-types application/x-blink-test-plugin"> </head> <body> <!-- the "type" below is a lie to get around first line of security checks/defenses --> <iframe type="application/x-blink-test-plugin" src="http://localhost:8000/media/resources/test.pdf" height="400" wi dth="400"></iframe> </body> </html> from http://127.0.0.1:8000/security/contentSecurityPolicy/1.1/plugintypes-cross-origin-plugin-content.html I think if we modify plugintypes-affects-child.html into plugintypes-affects-dishonest-child.html ("dishonest" = lying in type attribute of iframe) and the test still passes after yanking out the 2nd "if" statement, then everything is okay, right?
,
Aug 2 2016
,
Aug 3 2016
I have a layout test that crashes in --site-per-process mode - it turned out that I can get a crash by just modifying existing plugintypes-affects-child.html layout test to 1) have the iframe.src point at a cross-site plugin and 2) lie in iframe.type (to skip early CSP checks):
...
<meta http-equiv="Content-Security-Policy" content="plugin-types text/plain">
...
<!-- the URI below is cross-site from the default layout tests site
http://127.0.0.1:8000 -->
<!-- we lie in the "type" attribute below, to pass early CSP checks -->
<iframe type="text/plain"
src="http://localhost:8000/plugins/resources/mock-plugin.pl">
</iframe>
,
Aug 3 2016
To get parent CSP, we don't need to cast a frame to local frame - the patch below avoids a crash, but surprisingly it doesn't enforce the CSP :-(
--- a/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp
+++ b/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp
@@ -541,8 +541,8 @@ bool ContentSecurityPolicy::allowPluginTypeForDocument(const Document& document,
// FIXME: The plugin-types directive should be pushed down into the
// current document instead of reaching up to the parent for it here.
LocalFrame* frame = document.frame();
- if (frame && frame->tree().parent() && frame->tree().parent()->isLocalFrame() && document.isPluginDocument()) {
- ContentSecurityPolicy* parentCSP = toLocalFrame(frame->tree().parent())->document()->contentSecurityPolicy();
+ if (frame && frame->tree().parent() && document.isPluginDocument()) {
+ ContentSecurityPolicy* parentCSP = frame->securityContext()->contentSecurityPolicy();
if (parentCSP && !parentCSP->allowPluginType(type, typeAttribute, url))
return false;
}
,
Aug 3 2016
Nevermind, I am silly - the patch from #c5 should have said |frame->tree().parent()->securityContext()->...| instead of |frame->securityContext()->...|.
,
Aug 8 2016
The fix is pending at https://codereview.chromium.org/2213593002 Tentatively marking this bug as Proj-IsolateExtensions-BlockingLaunch. Whether it really is a blocker depends on how likely extension pages are to 1) embed a plugin and 2) restrict plugin types via CSP.
,
Aug 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6e2cf9768a02d4ff281dc47d9a1fb07ddb1e27a0 commit 6e2cf9768a02d4ff281dc47d9a1fb07ddb1e27a0 Author: lukasza <lukasza@chromium.org> Date: Thu Aug 18 16:43:20 2016 OOPIF support for 'plugin-types' Content Security Policy. The CL makes 'plugin-types' Content Security Policy compatible with OOPIFs by avoiding isLocalFrame check and toLocalFrame cast inside ContentSecurityPolicy::allowPluginTypeForDocument method. The method was tweaked to get a ContentSecurityPolicy object for the parent frame in a way that works both for local and remote frames. Additionally, it turned out that before this CL, the error message about the blocked plugin would incorrectly report the declared MIME type of the plugin if the parent frame is local (i.e. without --site-per-process), but would report actual, correct MIME type if the parent frame was remote (i.e. with --site-per-process, after fixing allowPluginTypeForDocument method). This would result in non-sensical error messages where the blocked plugin seems to match the list of allowed MIME types: Refused to load 'http://localhost:8000/plugins/resources/mock-plugin.pl' (MIME type 'text/plain') because it violates the following Content Security Policy Directive: 'plugin-types text/plain'. The expected error message is obviously: Refused to load 'http://localhost:8000/plugins/resources/mock-plugin.pl' (MIME type 'application/x-blink-test-plugin') because it violates the following Content Security Policy Directive: 'plugin-types text/plain'. This is fixed by tweaking HTMLPlugInElement::allowedToLoadObject to always pass the value of type attribute declared in the plugin document, rather than passing the value of the type attribute declared in the parent document. BUG= 633749 Review-Url: https://codereview.chromium.org/2213593002 Cr-Commit-Position: refs/heads/master@{#412855} [modify] https://crrev.com/6e2cf9768a02d4ff281dc47d9a1fb07ddb1e27a0/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process [add] https://crrev.com/6e2cf9768a02d4ff281dc47d9a1fb07ddb1e27a0/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/plugintypes-affects-cross-site-child-allowed-expected.txt [add] https://crrev.com/6e2cf9768a02d4ff281dc47d9a1fb07ddb1e27a0/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/plugintypes-affects-cross-site-child-allowed.html [add] https://crrev.com/6e2cf9768a02d4ff281dc47d9a1fb07ddb1e27a0/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/plugintypes-affects-cross-site-child-disallowed-expected.txt [add] https://crrev.com/6e2cf9768a02d4ff281dc47d9a1fb07ddb1e27a0/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/plugintypes-affects-cross-site-child-disallowed.html [modify] https://crrev.com/6e2cf9768a02d4ff281dc47d9a1fb07ddb1e27a0/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp [modify] https://crrev.com/6e2cf9768a02d4ff281dc47d9a1fb07ddb1e27a0/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp
,
Aug 19 2016
,
Aug 19 2016
,
Nov 25 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by lukasza@chromium.org
, Aug 2 2016