Bug 92675

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 Flags
WIP: Not for review.
none
Patch
none
Dropping FIXME comment.
none
Patch
none
Patch none

Description Mike West 2012-07-30 13:49:50 PDT
For whatever reason, `<object data="/plugins/resources/mock-plugin.pl"></object>` renders very differently from `<object data="data:application/x-webkit-test-netscape,whatever"></object>`. Because the former renders inside a PluginDocument, CSP isn't applied, and it breezes right through `object-src 'none'`. We should fix this.

I'll put up a patch in a bit that's the first thing that comes to mind. Hopefully one of you three can tell me how it should actually be fixed. :)
Comment 1 Mike West 2012-07-30 14:36:10 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 2 Adam Barth 2012-07-30 14:37:45 PDT
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 :)
Comment 3 Adam Barth 2012-07-30 14:38:05 PDT
Perhaps an easier solution is to have a plugin document inherit the parent's CSP policy like we do for srcdoc documents?
Comment 4 Mike West 2012-07-30 14:45:01 PDT
(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. :)
Comment 5 Mike West 2012-07-30 23:15:06 PDT
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.
Comment 6 Mike West 2012-07-31 05:41:53 PDT
Created attachment 155508 [details]
Patch
Comment 7 Mike West 2012-07-31 05:47:37 PDT
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?
Comment 8 Mike West 2012-07-31 05:50:45 PDT
Created attachment 155509 [details]
Dropping FIXME comment.
Comment 9 jochen 2012-08-01 00:50:48 PDT
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?
Comment 10 Mike West 2012-08-01 00:57:43 PDT
(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.
Comment 11 Mike West 2012-08-01 01:15:28 PDT
Created attachment 155746 [details]
Patch
Comment 12 Adam Barth 2012-08-01 11:14:23 PDT
> 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 13 Adam Barth 2012-08-01 11:18:47 PDT
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.
Comment 14 Mike West 2012-08-01 11:35:07 PDT
(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.
Comment 15 Mike West 2012-08-01 11:40:05 PDT
Created attachment 155848 [details]
Patch
Comment 16 Adam Barth 2012-08-01 11:55:28 PDT
Comment on attachment 155848 [details]
Patch

Thanks, this looks a bit cleaner.
Comment 17 Mike West 2012-08-01 11:56:50 PDT
Comment on attachment 155848 [details]
Patch

Cool. In that case, CQ? :)
Comment 18 WebKit Review Bot 2012-08-01 14:34:29 PDT
Comment on attachment 155848 [details]
Patch

Clearing flags on attachment: 155848

Committed r124371: <http://trac.webkit.org/changeset/124371>
Comment 19 WebKit Review Bot 2012-08-01 14:34:34 PDT
All reviewed patches have been landed.  Closing bug.