Only allow insecure object-src directives for whitelisted mime types

Only allow insecure object-src directives for whitelisted mime types

This CL only allows insecure object-src directives in the CSP of an extension if a set of whitelisted mime types are also specified in the CSP. This is to prevent plugins that aren't fully sandboxed from loading up arbitrary URLs in an extension and maliciously gaining control of the extension.

The set of plugins that are whitelisted should be those that are fully sandboxed.

Committed: https://crrev.com/3c02e4d34f7be4ea737688a1a011efb820a4ddd2
Cr-Commit-Position: refs/heads/master@{#305761}

[raymes, 2014-11-25 13:34:43]

raymes@chromium.org changed reviewers:
+ kalman@chromium.org, mkwst@chromium.org

[Mike West, 2014-11-25 13:42:36]

mkwst@chromium.org changed required reviewers:
+ kalman@chromium.org

[Mike West, 2014-11-25 13:42:37]

Conceptually, this LGTM. Thanks for taking another pass. The details look
reasonable, but please get kalman@'s feedback to ensure it's in line with how
he'd like the extension bits to work.

https://codereview.chromium.org/760513003/diff/80001/extensions/common/csp_va...
File extensions/common/csp_validator.cc (right):

https://codereview.chromium.org/760513003/diff/80001/extensions/common/csp_va...
extensions/common/csp_validator.cc:214: only_sandboxed_plugins_specified =
false;
This is probably equally clear if you just return false here and true at the
bottom of the function, rather than creating the local variable.

https://codereview.chromium.org/760513003/diff/80001/extensions/common/manife...
File extensions/common/manifest_handlers/csp_info.cc (right):

https://codereview.chromium.org/760513003/diff/80001/extensions/common/manife...
extensions/common/manifest_handlers/csp_info.cc:61: // allowing this publicly.
What conditions will have to be met to allow this publicly? If we consider PDF
and PNaCl "safe enough" for component extensions, we should probably consider
them safe enough for normal extensions as well.

That's a policy question for kalman@ et al, however. Not something that this CL
needs to address.

[raymes, 2014-11-25 13:51:05]

https://codereview.chromium.org/760513003/diff/80001/extensions/common/csp_va...
File extensions/common/csp_validator.cc (right):

https://codereview.chromium.org/760513003/diff/80001/extensions/common/csp_va...
extensions/common/csp_validator.cc:214: only_sandboxed_plugins_specified =
false;
On 2014/11/25 13:42:36, Mike West wrote:
> This is probably equally clear if you just return false here and true at the
> bottom of the function, rather than creating the local variable.

Done.

https://codereview.chromium.org/760513003/diff/80001/extensions/common/manife...
File extensions/common/manifest_handlers/csp_info.cc (right):

https://codereview.chromium.org/760513003/diff/80001/extensions/common/manife...
extensions/common/manifest_handlers/csp_info.cc:61: // allowing this publicly.
On 2014/11/25 13:42:36, Mike West wrote:
> What conditions will have to be met to allow this publicly? If we consider PDF
> and PNaCl "safe enough" for component extensions, we should probably consider
> them safe enough for normal extensions as well.
> 
> That's a policy question for kalman@ et al, however. Not something that this
CL
> needs to address.

From my understanding of things, this would be reasonable to allow publicly
as-is but it would probably be best for someone on security to sign off on that
:)

[Mike West, 2014-11-25 13:52:15]

mkwst@chromium.org changed reviewers:
+ jschuh@chromium.org

[Mike West, 2014-11-25 13:52:16]

+jschuh, who loves plugins.

[not at google - send to devlin, 2014-11-25 17:08:46]

lgtm

https://codereview.chromium.org/760513003/diff/120001/extensions/common/csp_v...
File extensions/common/csp_validator.cc (right):

https://codereview.chromium.org/760513003/diff/120001/extensions/common/csp_v...
extensions/common/csp_validator.cc:211: for (size_t i = 0; i <
plugin_types.size(); ++i) {
Perhaps use newfangled for-loop syntax, if you like?

[raymes, 2014-11-25 22:31:16]

Thanks all. I also added another mime type for the internal pdf plugin which is
what actually gets used by the extension.

I'm going to go ahead and land this.

jschuh@ if you're happy that this is safe to enable publicly, I'll remove the
flag in a separate CL.

https://codereview.chromium.org/760513003/diff/120001/extensions/common/csp_v...
File extensions/common/csp_validator.cc (right):

https://codereview.chromium.org/760513003/diff/120001/extensions/common/csp_v...
extensions/common/csp_validator.cc:211: for (size_t i = 0; i <
plugin_types.size(); ++i) {
On 2014/11/25 17:08:46, kalman wrote:
> Perhaps use newfangled for-loop syntax, if you like?

Done.

[raymes, 2014-11-26 01:05:23]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-11-26 01:06:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/760513003/140001

[commit-bot: I haz the power, 2014-11-26 02:28:04]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-11-26 02:29:18]

Patchset 8 (id:??) landed as
https://crrev.com/3c02e4d34f7be4ea737688a1a011efb820a4ddd2
Cr-Commit-Position: refs/heads/master@{#305761}