Summary: | CSP should correctly block plugin resources rendered in PluginDocuments. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mike West <mkwst> | ||||||||||||
Component: | WebKit Misc. | Assignee: | Mike West <mkwst> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, bauerb, japhet, jochen, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Mike West
2012-07-30 13:49:50 PDT
Created attachment 155367 [details]
WIP: Not for review.
This looks like it has different behavior on Mac. I can replicate the failure here at home, but it never touches SubframeLoader::requestPlugin or SubframeLoader::loadPlugin (at least, none of my cleverly placed `addConsoleMessage` calls does anything useful). Bleh.
I'll fiddle with it more tomorrow back on linux. Just throwing a patch here so I know what I was trying and failing to do.
Comment on attachment 155367 [details] WIP: Not for review. View in context: https://bugs.webkit.org/attachment.cgi?id=155367&action=review > Source/WebCore/loader/SubframeLoader.cpp:115 > + message = makeString("I'm a plugin document! Owner: ", m_frame->document()->ownerElement()->document()->url().string(), ", Type: ", m_frame->document()->ownerElement()->fastGetAttribute(HTMLNames::typeAttr)); four-space indent :) Perhaps an easier solution is to have a plugin document inherit the parent's CSP policy like we do for srcdoc documents? (In reply to comment #3) > Perhaps an easier solution is to have a plugin document inherit the parent's CSP policy like we do for srcdoc documents? I do think that's the right answer. This just seemed simpler. If it had worked, I'd have had a patch up an hour ago! :) Ah well. I'll get a debugger set up tomorrow to track down the plugin document's instantiation, and pass over the CSP. Then I'll cross my fingers and hope it works cross-platform. :) Ok, the cross-platform issue wasn't a cross-platform issue. It was me being unable to copy and paste correctly. :) And, I now recall why I preferred this approach last night: CSP isn't the only check made against the document. In requestPlugin, we check the sandbox status, and in loadPlugin we check `securityOrigin->canDisplay`. I'm not sure either of those are set correctly for the PluginDocument, and I don't see any tests either way. :/ I'll try to write some today. If they work correctly, great, I'll try to inject the CSP into the plugin document. If not, walking up the document's ownerElement as done in this patch might be a better approach. Created attachment 155508 [details]
Patch
After a bit of research, here's an implementation that I think works. I've reverted to Adam's idea of populating the PluginDocument with the parent's CSP after taking a look at the code that actually does this work (`DocumentWriter::begin`) 1. Sandboxing is dealt with correctly by creating a `SinkDocument` that ignores plugins. 2. The security origin is correctly brought over. The only bit missing is the CSP, which this patch populates. I don't completely understand how DocumentWriter works, however. An `ownerDocument` can be passed in, but hasn't been populated in any case I've seen. I'm using `m_frame->ownerElement()->document()` instead, which seems correct, but I'm not sure. WDYT? Created attachment 155509 [details]
Dropping FIXME comment.
Comment on attachment 155509 [details] Dropping FIXME comment. I'm unsure about whether m_frame->ownerElement->document is the right thing, so I'm deferring to Adam View in context: https://bugs.webkit.org/attachment.cgi?id=155509&action=review > LayoutTests/ChangeLog:10 > + `Content-Type application/x-webkit-test-netscape` header. backticks look just wrong :) > LayoutTests/http/tests/plugins/resources/mock-plugin.pl:5 > +my $cgi = new CGI; not required (incl. use CGI)? > LayoutTests/http/tests/security/contentSecurityPolicy/object-src-url-allowed.html:6 > + testRunner.dumpAsText(); 4 spaces > LayoutTests/http/tests/security/contentSecurityPolicy/object-src-url-blocked.html:6 > + testRunner.dumpAsText(); 4 spaces > LayoutTests/http/tests/security/contentSecurityPolicy/resources/echo-object-data.pl:15 > +print " log=\"".$cgi->param('log')."\"\n" if $cgi->param('log'); how does this change relate to the rest of the change? (In reply to comment #9) > (From update of attachment 155509 [details]) > I'm unsure about whether m_frame->ownerElement->document is the right thing, so I'm deferring to Adam > > > View in context: https://bugs.webkit.org/attachment.cgi?id=155509&action=review > > > LayoutTests/ChangeLog:10 > > + `Content-Type application/x-webkit-test-netscape` header. > > backticks look just wrong :) Markdown, baby! > > LayoutTests/http/tests/plugins/resources/mock-plugin.pl:5 > > +my $cgi = new CGI; > > not required (incl. use CGI)? This is copy/pasted from other Perl scripts I've seen. Can you point me at one that does it the way you'd like? It's been a loooong time since I knew what I was doing in Perl. :) > > LayoutTests/http/tests/security/contentSecurityPolicy/object-src-url-allowed.html:6 > > + testRunner.dumpAsText(); > > 4 spaces > > > LayoutTests/http/tests/security/contentSecurityPolicy/object-src-url-blocked.html:6 > > + testRunner.dumpAsText(); > > 4 spaces Arg. > > > LayoutTests/http/tests/security/contentSecurityPolicy/resources/echo-object-data.pl:15 > > +print " log=\"".$cgi->param('log')."\"\n" if $cgi->param('log'); > > how does this change relate to the rest of the change? It's unrelated cleanup. I'm happy to move it to a separate patch. Created attachment 155746 [details]
Patch
> I don't completely understand how DocumentWriter works, however. An `ownerDocument` can be passed in, but hasn't been populated in any case I've seen.
That happens when one document calls document.open() on another document.
Comment on attachment 155746 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155746&action=review > Source/WebCore/loader/DocumentWriter.cpp:145 > + if (document->isPluginDocument() && m_frame->ownerElement()) > + document->contentSecurityPolicy()->copyStateFrom(m_frame->ownerElement()->document()->contentSecurityPolicy()); I wonder if we should do this work in Document::initContentSecurityPolicy. We could check for isPluginDocument() there and centralize the logic for copying the CSP policy from our parent. (In reply to comment #13) > (From update of attachment 155746 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=155746&action=review > > > Source/WebCore/loader/DocumentWriter.cpp:145 > > + if (document->isPluginDocument() && m_frame->ownerElement()) > > + document->contentSecurityPolicy()->copyStateFrom(m_frame->ownerElement()->document()->contentSecurityPolicy()); > > I wonder if we should do this work in Document::initContentSecurityPolicy. We could check for isPluginDocument() there and centralize the logic for copying the CSP policy from our parent. Sounds reasonable enough, and means we won't have to include those extra headers in DocumentWriter. I'll throw a new patch up shortly. Created attachment 155848 [details]
Patch
Comment on attachment 155848 [details]
Patch
Thanks, this looks a bit cleaner.
Comment on attachment 155848 [details]
Patch
Cool. In that case, CQ? :)
Comment on attachment 155848 [details] Patch Clearing flags on attachment: 155848 Committed r124371: <http://trac.webkit.org/changeset/124371> All reviewed patches have been landed. Closing bug. |