Bug 92675 – CSP should correctly block plugin resources rendered in PluginDocuments.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Mike West]

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. :)

[Mike West]

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.

[Adam Barth]

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 :)

[Adam Barth]

Perhaps an easier solution is to have a plugin document inherit the parent's CSP policy like we do for srcdoc documents?

[Mike West]

(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. :)

[Mike West]

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.

[Mike West]

Created attachment 155508 [details]
Patch

[Mike West]

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?

[Mike West]

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?

[Mike West]

(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.

[Mike West]

Created attachment 155746 [details]
Patch

[Adam Barth]

> 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.

[Adam Barth]

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.

[Mike West]

(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.

[Mike West]

Created attachment 155848 [details]
Patch

[Adam Barth]

Comment on attachment 155848 [details]
Patch

Thanks, this looks a bit cleaner.

[Mike West]

Comment on attachment 155848 [details]
Patch

Cool. In that case, CQ? :)

[WebKit Review Bot]

Comment on attachment 155848 [details]
Patch

Clearing flags on attachment: 155848

Committed r124371: <http://trac.webkit.org/changeset/124371>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.