Bug 91919

Created attachment 153652 [details] WIP

Uploaded WIP patch. Can you verify that I'm going in the right direction, Adam, while I fiddle with the FIXMEs? :)

(In reply to comment #2) > Uploaded WIP patch. Can you verify that I'm going in the right direction, Adam, while I fiddle with the FIXMEs? :) (Note the `applet` hard-coding in particular.)

Created attachment 153676 [details] First pass.

Comment on attachment 153676 [details] First pass. View in context: https://bugs.webkit.org/attachment.cgi?id=153676&action=review > Source/WebCore/loader/SubframeLoader.cpp:127 > - if (!m_frame->document()->contentSecurityPolicy()->allowObjectFromSource(url)) > + if (!m_frame->document()->contentSecurityPolicy()->allowObjectFromSource(url) > + || !m_frame->document()->contentSecurityPolicy()->allowPluginType(mimeType, url)) I'm not sure this is sufficient. The actual type that gets instantiated for a plugin is really complicated. Hixie has a crazy diagram that shows how it all works. As an example, we should test <object data="..."></object> with a response that has a Content-Type header that specifies a plugin. If you look through the Chromium code, you'll find an implementation of the "content settings" feature that blocks plugins by type. It might be worth looking at how that code works. > Source/WebCore/page/ContentSecurityPolicy.cpp:820 > + return checkPluginTypeAndReportViolation(type, "Refused to load '" + url.string() + "' (MIME type `" + type + "`) because it violates the following Content Security Policy Directive: "); I'm not sure whether we should use `. Maybe just ' would be better? > Source/WebCore/page/ContentSecurityPolicy.cpp:1008 > + m_pluginTypesDirective = String(begin, end - begin); Isn't this just |value|? If we just store |value| here rather than calling String(), we can avoid a malloc and a memcpy because we'll just take another reference to value. > Source/WebCore/page/ContentSecurityPolicy.cpp:1019 > + while (position < end) { This probably isn't the right way to handle errors here. We need to make the spec more precise as to how to parse this directive. What we should do is first split on whitespace and then process each non-whitespace sequence. For example, that will parse "image/jpg boo text/html" as ["image/jpg", "text/html"] rather than ["image/jpg"], whcih is what this code will do. This code also has a bug in that it empties out m_pluginTypesDirective even if it makes m_pluginTypes non-empty.

jochen knows about blocking plugins by type. It would be good to get his input on this patch.

(In reply to comment #5) > (From update of attachment 153676 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153676&action=review > > > Source/WebCore/loader/SubframeLoader.cpp:127 > > - if (!m_frame->document()->contentSecurityPolicy()->allowObjectFromSource(url)) > > + if (!m_frame->document()->contentSecurityPolicy()->allowObjectFromSource(url) > > + || !m_frame->document()->contentSecurityPolicy()->allowPluginType(mimeType, url)) > > I'm not sure this is sufficient. The actual type that gets instantiated for a plugin is really complicated. Hixie has a crazy diagram that shows how it all works. As an example, we should test <object data="..."></object> with a response that has a Content-Type header that specifies a plugin. > > If you look through the Chromium code, you'll find an implementation of the "content settings" feature that blocks plugins by type. It might be worth looking at how that code works. Alright. I knew it seemed too easy. :) I'll poke the Munich folks about content settings code this week when I'm back in town. > > > Source/WebCore/page/ContentSecurityPolicy.cpp:820 > > + return checkPluginTypeAndReportViolation(type, "Refused to load '" + url.string() + "' (MIME type `" + type + "`) because it violates the following Content Security Policy Directive: "); > > I'm not sure whether we should use `. Maybe just ' would be better? You're right. The backtic is an artifact of my obsession with Markdown; single quotes are probably appropriate here. > > > Source/WebCore/page/ContentSecurityPolicy.cpp:1008 > > + m_pluginTypesDirective = String(begin, end - begin); > > Isn't this just |value|? If we just store |value| here rather than calling String(), we can avoid a malloc and a memcpy because we'll just take another reference to value. Good point. I'll fix this. > > > Source/WebCore/page/ContentSecurityPolicy.cpp:1019 > > + while (position < end) { > > This probably isn't the right way to handle errors here. We need to make the spec more precise as to how to parse this directive. What we should do is first split on whitespace and then process each non-whitespace sequence. For example, that will parse "image/jpg boo text/html" as ["image/jpg", "text/html"] rather than ["image/jpg"], whcih is what this code will do. > > This code also has a bug in that it empties out m_pluginTypesDirective even if it makes m_pluginTypes non-empty. `m_pluginTypesDirective` being empty is sufficient to make the current implementation fail to load any plugin. This implementation takes a draconian approach: given `image/jpg boo text/html`, it would block all plugins, as `boo` isn't recognizably in the form of a MIME type. See the check in CSPDirectiveList::checkPluginTypeAndReportViolation for detail. That's arguably the wrong approach, but it seemed consistent with the route we took regarding `script-nonce` (which boiled down to "That's not a nonce; no script for you."). If that is the route we go down, then you're still right: we should clear out `m_pluginTypes`, which would remove the necessity of emptying the directive text. That's a much better implementation.

> `m_pluginTypesDirective` being empty is sufficient to make the current implementation fail to load any plugin. This implementation takes a draconian approach: given `image/jpg boo text/html`, it would block all plugins, as `boo` isn't recognizably in the form of a MIME type. See the check in CSPDirectiveList::checkPluginTypeAndReportViolation for detail. That's arguably the wrong approach, but it seemed consistent with the route we took regarding `script-nonce` (which boiled down to "That's not a nonce; no script for you."). For source-list, we skip over sources that we don't understand, which gives more scope for future additions to the syntax... > If that is the route we go down, then you're still right: we should clear out `m_pluginTypes`, which would remove the necessity of emptying the directive text. That's a much better implementation. It's not clear to me which behavior is better. We should think for a bit about whether there are future additions we might make to the syntax...

(In reply to comment #8) > > `m_pluginTypesDirective` being empty is sufficient to make the current implementation fail to load any plugin. This implementation takes a draconian approach: given `image/jpg boo text/html`, it would block all plugins, as `boo` isn't recognizably in the form of a MIME type. See the check in CSPDirectiveList::checkPluginTypeAndReportViolation for detail. That's arguably the wrong approach, but it seemed consistent with the route we took regarding `script-nonce` (which boiled down to "That's not a nonce; no script for you."). > > For source-list, we skip over sources that we don't understand, which gives more scope for future additions to the syntax... > > > If that is the route we go down, then you're still right: we should clear out `m_pluginTypes`, which would remove the necessity of emptying the directive text. That's a much better implementation. > > It's not clear to me which behavior is better. We should think for a bit about whether there are future additions we might make to the syntax... I edited the draft 1.1 spec to embody this behavior in https://dvcs.w3.org/hg/content-security-policy/rev/5e6fdb226239, but even though I can't think of any good reason to allow non-MIME-style types in the future, it's certainly a possibility I hadn't considered. I'll drop a note to the list.

Comment on attachment 153676 [details] First pass. View in context: https://bugs.webkit.org/attachment.cgi?id=153676&action=review >>> Source/WebCore/loader/SubframeLoader.cpp:127 >>> + || !m_frame->document()->contentSecurityPolicy()->allowPluginType(mimeType, url)) >> >> I'm not sure this is sufficient. The actual type that gets instantiated for a plugin is really complicated. Hixie has a crazy diagram that shows how it all works. As an example, we should test <object data="..."></object> with a response that has a Content-Type header that specifies a plugin. >> >> If you look through the Chromium code, you'll find an implementation of the "content settings" feature that blocks plugins by type. It might be worth looking at how that code works. > > Alright. I knew it seemed too easy. :) > > I'll poke the Munich folks about content settings code this week when I'm back in town. I'm not sure that matching against the mime type of the plugin that ends up being loaded is achievable at all - we currently just take whether WebKit gives us, and then decide how to render the plugin, but never actually tell webkit what we did (i.e. it might by a click2play place holder that creates a render view to displays its contents).

Comment on attachment 153676 [details] First pass. View in context: https://bugs.webkit.org/attachment.cgi?id=153676&action=review >>>> Source/WebCore/loader/SubframeLoader.cpp:127 >>>> + || !m_frame->document()->contentSecurityPolicy()->allowPluginType(mimeType, url)) >>> >>> I'm not sure this is sufficient. The actual type that gets instantiated for a plugin is really complicated. Hixie has a crazy diagram that shows how it all works. As an example, we should test <object data="..."></object> with a response that has a Content-Type header that specifies a plugin. >>> >>> If you look through the Chromium code, you'll find an implementation of the "content settings" feature that blocks plugins by type. It might be worth looking at how that code works. >> >> Alright. I knew it seemed too easy. :) >> >> I'll poke the Munich folks about content settings code this week when I'm back in town. > > I'm not sure that matching against the mime type of the plugin that ends up being loaded is achievable at all - we currently just take whether WebKit gives us, and then decide how to render the plugin, but never actually tell webkit what we did (i.e. it might by a click2play place holder that creates a render view to displays its contents). Right, neither the actual MIME type nor the actual WebPlugin instance might be what we expect it to be; for example the MIME type could be empty here, in which case we'd try to find a matching plug-in based on the URL extension. However, if the CSP is only used for *whitelisting* certain plug-ins, going by MIME type shouldn't be a problem: If a page specifies a certain MIME type, we refuse to load a plug-in with a different MIME type (to avoid confusion attacks; see AllowMimeTypeMismatch() in Chromium plugin_list.cc). That means, if we have e.g. a CSP that only allows the "application/shockwave-flash" MIME type, we can block all other MIME types here at the Webkit level, and our plug-in implementation already blocks all other MIME types. It's true that we can't tell here which WebPlugin is actually going to handle it (could be a placeholder, or this or that version of Flash), but that's not something that this layer knows about anyway. The only thing we'd need to make sure is that we don't allow a MIME type of application/octet-stream in the CSP, which is the only non-empty MIME type that can be overridden (because that would effectively allow any plug-in again). Adam, I'd be interested in seeing that crazy diagram sometime ;-)

> Adam, I'd be interested in seeing that crazy diagram sometime ;-) I looked around a bit and couldn't lay my hands on it. Someone in #whatwg might be able to find it. Another possibility here is to pass the list of allowed MIME types to the embedder and have it do the work of making sure we don't violate the policy.

(In reply to comment #11) > (From update of attachment 153676 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153676&action=review > > >>>> Source/WebCore/loader/SubframeLoader.cpp:127 > >>>> + || !m_frame->document()->contentSecurityPolicy()->allowPluginType(mimeType, url)) > >>> > >>> I'm not sure this is sufficient. The actual type that gets instantiated for a plugin is really complicated. Hixie has a crazy diagram that shows how it all works. As an example, we should test <object data="..."></object> with a response that has a Content-Type header that specifies a plugin. > >>> > >>> If you look through the Chromium code, you'll find an implementation of the "content settings" feature that blocks plugins by type. It might be worth looking at how that code works. > >> > >> Alright. I knew it seemed too easy. :) > >> > >> I'll poke the Munich folks about content settings code this week when I'm back in town. > > > > I'm not sure that matching against the mime type of the plugin that ends up being loaded is achievable at all - we currently just take whether WebKit gives us, and then decide how to render the plugin, but never actually tell webkit what we did (i.e. it might by a click2play place holder that creates a render view to displays its contents). > > Right, neither the actual MIME type nor the actual WebPlugin instance might be what we expect it to be; for example the MIME type could be empty here, in which case we'd try to find a matching plug-in based on the URL extension. > > However, if the CSP is only used for *whitelisting* certain plug-ins, going by MIME type shouldn't be a problem: If a page specifies a certain MIME type, we refuse to load a plug-in with a different MIME type (to avoid confusion attacks; see AllowMimeTypeMismatch() in Chromium plugin_list.cc). If I understand you correctly, this would cover the case where a developer includes an explicit MIME type in the `object` or `embed` element. Is that correct? If so, I like it. I wouldn't be at all averse to making an explicit declaration of the expected MIME type on the element itself mandatory in the CSP spec (similar to the mandatory `nonce` attribute), as that has nice security properties at a minimal cost to the developer. If possible, I'd prefer not to hand off the MIME types to the embedder. The implementation seems cleaner conceptually if the CSP object retains responsibility for a thumbs up/down on resources it asserts control over. Delegating that responsibility feels wrong.

Ok. I'm sold on coupling this with the strict "type" behavior. Would you be willing to email the working group and edit the spec. :-P

Note: There's already a mechanism in the HTML spec for requiring this strict checking. We just need to tell the HTML spec to require it.

(In reply to comment #14) > Ok. I'm sold on coupling this with the strict "type" behavior. Would you be willing to email the working group and edit the spec. :-P Sure. I'll take a look at this tonight, assuming that the tiny baby keeps me up till 2 again. :)

(In reply to comment #15) > Note: There's already a mechanism in the HTML spec for requiring this strict checking. We just need to tell the HTML spec to require it. You're talking about 'typemustmatch', right? (http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#attr-object-typemustmatch)

(In reply to comment #17) > (In reply to comment #15) > > Note: There's already a mechanism in the HTML spec for requiring this strict checking. We just need to tell the HTML spec to require it. > > You're talking about 'typemustmatch', right? (http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#attr-object-typemustmatch) Yes, exactly.

Comment on attachment 153676 [details] First pass. This patch isn't quite right, as discussed.

Created attachment 156932 [details] Patch

The attached patch implements the check against the declared type of the plugin, as we discussed here. I'll drop a line to the public-webappsec@ group now to weigh out the impacts of that suggestion. Two caveats: 1. Since WebKit doesn't seem to natively support the `typemustmatch` mechanism, I'm working around it by doing the comparison inside of CSP. It's not perfect, but I hope it's good enough for the moment. 2. I'm hardcoding the MIME type of comparisons for the `applet` element to `application/x-java-applet`, which seemed reasonable.

(In reply to comment #21) > The attached patch implements the check against the declared type of the plugin, as we discussed here. I'll drop a line to the public-webappsec@ group now to weigh out the impacts of that suggestion. http://lists.w3.org/Archives/Public/public-webappsec/2012Aug/0000.html I think it's worth reviewing this patch now so that I can clean up any defects, but (assuming there's debate) I'd like to wait to land it until that discussion reaches consensus.

Comment on attachment 156932 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156932&action=review > Source/WebCore/loader/SubframeLoader.cpp:133 > + if (!document()->contentSecurityPolicy()->allowObjectFromSource(url) > + || !m_frame->document()->contentSecurityPolicy()->allowPluginType(mimeType, declaredMimeType, url)) { Why do these two lines use a different way to find the document? > Source/WebCore/loader/SubframeLoader.cpp:301 > + || !element->document()->contentSecurityPolicy()->allowPluginType("application/x-java-applet", "application/x-java-applet", codeBaseURL)) Can you make this string into a named constant? > Source/WebCore/page/ContentSecurityPolicy.cpp:600 > + bool checkPluginType(const String& type, const String& typeAttr) const; typeAttr -> typeAttribute (please use complete words in variable names) > Source/WebCore/page/ContentSecurityPolicy.cpp:628 > + HashSet<String> m_pluginTypes; > + String m_pluginTypesDirective; Should we create a PluginTypesDirective object to hold these pieces of state? > Source/WebCore/page/ContentSecurityPolicy.cpp:729 > + message = makeString("`plugin-types` Content Security Policy directive is empty; all plugins will be blocked.\n"); You don't need to call makeString if you've just got a single string. You can just use the assignment operator. Also, please don't use ` in console messages. We prefer '. > Source/WebCore/page/ContentSecurityPolicy.cpp:1088 > + // `plugin-types ____;` OR `plugin-types;` No ` please. > Source/WebCore/page/ContentSecurityPolicy.cpp:1089 > + if (value.isNull()) { isNull -> isEmpty ? > Source/WebCore/page/ContentSecurityPolicy.cpp:1297 > +template<bool (CSPDirectiveList::*allowed)(const String&, const String&, const KURL&, ContentSecurityPolicy::ReportingStatus) const> I'm not sure its worth having this whole template given that we're probably only going to have one function with this signature.

(In reply to comment #23) > (From update of attachment 156932 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156932&action=review > > > Source/WebCore/loader/SubframeLoader.cpp:133 > > + if (!document()->contentSecurityPolicy()->allowObjectFromSource(url) > > + || !m_frame->document()->contentSecurityPolicy()->allowPluginType(mimeType, declaredMimeType, url)) { > > Why do these two lines use a different way to find the document? 'document()' is the same as 'm_frame->document()'; we should be using the former. Not sure why I didn't here... > > > Source/WebCore/loader/SubframeLoader.cpp:301 > > + || !element->document()->contentSecurityPolicy()->allowPluginType("application/x-java-applet", "application/x-java-applet", codeBaseURL)) > > Can you make this string into a named constant? Sure thing. DEFINE_STATIC_LOCAL, or something publicly accessible on some relevant class? > > > Source/WebCore/page/ContentSecurityPolicy.cpp:600 > > + bool checkPluginType(const String& type, const String& typeAttr) const; > > typeAttr -> typeAttribute (please use complete words in variable names) Done. > > Source/WebCore/page/ContentSecurityPolicy.cpp:628 > > + HashSet<String> m_pluginTypes; > > + String m_pluginTypesDirective; > > Should we create a PluginTypesDirective object to hold these pieces of state? We certainly could. Should we do the same for script-nonce? > > Source/WebCore/page/ContentSecurityPolicy.cpp:729 > > + message = makeString("`plugin-types` Content Security Policy directive is empty; all plugins will be blocked.\n"); > > You don't need to call makeString if you've just got a single string. You can just use the assignment operator. Done. > Also, please don't use ` in console messages. We prefer '. I'm trying to wean myself off them in WebKit. Muscle memory. :( > > Source/WebCore/page/ContentSecurityPolicy.cpp:1089 > > + if (value.isNull()) { > > isNull -> isEmpty ? Done. > > Source/WebCore/page/ContentSecurityPolicy.cpp:1297 > > +template<bool (CSPDirectiveList::*allowed)(const String&, const String&, const KURL&, ContentSecurityPolicy::ReportingStatus) const> > > I'm not sure its worth having this whole template given that we're probably only going to have one function with this signature. Easy enough. I'd done it for consistency, but you're probably right.

(In reply to comment #24) > > > Source/WebCore/page/ContentSecurityPolicy.cpp:628 > > > + HashSet<String> m_pluginTypes; > > > + String m_pluginTypesDirective; > > > > Should we create a PluginTypesDirective object to hold these pieces of state? > > We certainly could. Should we do the same for script-nonce? Actually, now that I look at the code, I'd prefer not to do this until something like https://bugs.webkit.org/show_bug.cgi?id=93205 is in. If I pull the parsing out into a subclass of CSPDirective, then I won't be able to log many of the errors that are currently being generated (as only CSPDirectiveList can directly log errors, etc.).

> > > Source/WebCore/loader/SubframeLoader.cpp:301 > > > + || !element->document()->contentSecurityPolicy()->allowPluginType("application/x-java-applet", "application/x-java-applet", codeBaseURL)) > > > > Can you make this string into a named constant? > > Sure thing. DEFINE_STATIC_LOCAL, or something publicly accessible on some relevant class? I would just use a const char[] rather than a static. > > > > Source/WebCore/page/ContentSecurityPolicy.cpp:628 > > > > + HashSet<String> m_pluginTypes; > > > > + String m_pluginTypesDirective; > > > > > > Should we create a PluginTypesDirective object to hold these pieces of state? > > > > We certainly could. Should we do the same for script-nonce? > > Actually, now that I look at the code, I'd prefer not to do this until something like https://bugs.webkit.org/show_bug.cgi?id=93205 is in. If I pull the parsing out into a subclass of CSPDirective, then I won't be able to log many of the errors that are currently being generated (as only CSPDirectiveList can directly log errors, etc.). Ok.

Created attachment 157001 [details] Adam's feedback.

Comment on attachment 157001 [details] Adam's feedback. Attachment 157001 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13456233 New failing tests: http/tests/security/contentSecurityPolicy/1.1/plugintypes-url-02.html

Created attachment 157015 [details] Archive of layout-test-results from gce-cr-linux-06 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-06 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 157017 [details] Reset test result.

Created attachment 158062 [details] Rebased onto ToT.

(In reply to comment #22) > (In reply to comment #21) > > The attached patch implements the check against the declared type of the plugin, as we discussed here. I'll drop a line to the public-webappsec@ group now to weigh out the impacts of that suggestion. > > http://lists.w3.org/Archives/Public/public-webappsec/2012Aug/0000.html > > I think it's worth reviewing this patch now so that I can clean up any defects, but (assuming there's debate) I'd like to wait to land it until that discussion reaches consensus. New patch rebases this onto ToT. If there continues to be no feedback through tomorrow, I'd like to CQ this. We can always argue about/change the implementation after the fact. :)

> If there continues to be no feedback through tomorrow, I'd like to CQ this. We can always argue about/change the implementation after the fact. :) I think it's fine to land this whenever you like. We should just be careful not to ship CSP_NEXT until we'll happy with the progress in the working group. On that topic, we should probably make sure CSP_NEXT is disabled on the beta channel.

(In reply to comment #33) > > If there continues to be no feedback through tomorrow, I'd like to CQ this. We can always argue about/change the implementation after the fact. :) > > I think it's fine to land this whenever you like. We should just be careful not to ship CSP_NEXT until we'll happy with the progress in the working group. > > On that topic, we should probably make sure CSP_NEXT is disabled on the beta channel. Kerz@ is release manager for Chrome 22. Want me to ping him?

> Kerz@ is release manager for Chrome 22. Want me to ping him? Yeah.

(In reply to comment #35) > > Kerz@ is release manager for Chrome 22. Want me to ping him? > > Yeah. Covered in http://crbug.com/142415 In the meantime, I'll find someone to CQ this. :)

Comment on attachment 158062 [details] Rebased onto ToT. Clearing flags on attachment: 158062 Committed r125531: <http://trac.webkit.org/changeset/125531>

All reviewed patches have been landed. Closing bug.