Allow arbitrary object-src CSP directives for component extensions

Allow arbitrary object-src CSP directives for component extensions

This CL allows component extensions to specify arbitrary object-src CSP
directives. This should be safe because non-NPAPI plugins should load in a
sandboxed process and only allow communication via postMessage. Flash is
an exception since it allows scripting into the embedder page, but even then
it should disallow cross-origin scripting. At some point we may want to consider
allowing this publicly.

The CL refactors the CSP validator slightly to provide an options int to configure
how CSP will be parsed. Tests are added for the changes above.

BUG=416328
Committed: https://crrev.com/f43814b9553177aa71db780ddac7a3a4554a360c
Cr-Commit-Position: refs/heads/master@{#305725}

[raymes, 2014-11-24 03:28:20]

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

[Sam McNally, 2014-11-24 04:40:15]

https://codereview.chromium.org/754713002/diff/20001/extensions/common/csp_va...
File extensions/common/csp_validator.h (right):

https://codereview.chromium.org/754713002/diff/20001/extensions/common/csp_va...
extensions/common/csp_validator.h:21: enum Options {
Comments?

https://codereview.chromium.org/754713002/diff/20001/extensions/common/csp_va...
extensions/common/csp_validator.h:22: NO_OPTIONS = 0x00,
Prefix values with OPTIONS_.

https://codereview.chromium.org/754713002/diff/20001/extensions/common/csp_va...
File extensions/common/csp_validator_unittest.cc (right):

https://codereview.chromium.org/754713002/diff/20001/extensions/common/csp_va...
extensions/common/csp_validator_unittest.cc:180: "script-src 'self'; object-src
*", 0));
NO_OPTIONS.

[not at google - send to devlin, 2014-11-24 18:18:03]

lgtm

https://codereview.chromium.org/754713002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/754713002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:63: this.plugin_ =
document.createElement('embed');
Could you make these changes to pdf in a separate change?

https://codereview.chromium.org/754713002/diff/20001/extensions/common/csp_va...
File extensions/common/csp_validator.h (right):

https://codereview.chromium.org/754713002/diff/20001/extensions/common/csp_va...
extensions/common/csp_validator.h:21: enum Options {
On 2014/11/24 04:40:15, Sam McNally wrote:
> Comments?

when you do: please add a comment to ALLOW_INSECURE_OBJECT_SRC mentioning what
exactly makes it "insecure" (otherwise it's quite open to interpretation).

https://codereview.chromium.org/754713002/diff/20001/extensions/common/csp_va...
extensions/common/csp_validator.h:24: ALLOW_INSECURE_OBJECT_SRC = 0x02,
Let's keep discussing this enum: More typical style is to use (1 << 1), (1 <<
2), etc.

https://codereview.chromium.org/754713002/diff/20001/extensions/common/csp_va...
extensions/common/csp_validator.h:38: const std::string& policy, int options);
Mention that this is a bitmask.

https://codereview.chromium.org/754713002/diff/20001/extensions/common/csp_va...
File extensions/common/csp_validator_unittest.cc (right):

https://codereview.chromium.org/754713002/diff/20001/extensions/common/csp_va...
extensions/common/csp_validator_unittest.cc:193: "script-src *; object-src *;",
ALLOW_INSECURE_OBJECT_SRC));
Could you test somewhere ALLOW_UNSAFE_EVAL|ALLOW_INSECURE_OBJECT_SRC?

[not at google - send to devlin, 2014-11-24 18:19:33]

https://codereview.chromium.org/754713002/diff/20001/extensions/common/csp_va...
File extensions/common/csp_validator_unittest.cc (right):

https://codereview.chromium.org/754713002/diff/20001/extensions/common/csp_va...
extensions/common/csp_validator_unittest.cc:193: "script-src *; object-src *;",
ALLOW_INSECURE_OBJECT_SRC));
On 2014/11/24 18:18:03, kalman wrote:
> Could you test somewhere ALLOW_UNSAFE_EVAL|ALLOW_INSECURE_OBJECT_SRC?

I regret making this comment. Never mind.

[not at google - send to devlin, 2014-11-24 18:26:48]

kalman@chromium.org changed reviewers:
+ mkwst@chromium.org, rob@robwu.nl

[not at google - send to devlin, 2014-11-24 18:26:48]

Be aware of https://codereview.chromium.org/747403002/.

[Mike West, 2014-11-24 18:42:18]

On 2014/11/24 18:26:48, kalman wrote:
> Be aware of https://codereview.chromium.org/747403002/.

Given that flash can hop out of the sandbox with user permission, I'd strongly
suggest limiting plugins to secure origins. Is there a use case for loading
arbitrary plugin data over HTTP?

[raymes, 2014-11-24 23:16:41]

Hey Mike. Thanks for commenting about flash, it's a good point and I forgot
that flash could escape the sandbox. Part of the reason why I limited this
change to component extensions was in case there was something like this
which I hadn't considered.

The reason we need to allow http URLs in object-src is because we have an
extension which acts as a MIME type handler for PDFs. The extension uses a
plugin to help render the PDF and the plugin gets loaded to the URL of the
pdf that is being opened.

One potential solution is to limit the plugins that can be loaded in the
CSP using the plugin-types directive. In other words, if object-src * is
specified, also require plugin-types to be specified and require it to only
have white-listed mime types. The PDF plugin, as well as pnacl objects
should be safe to load to any URL inside an extension.

How would you feel about this approach?

Thanks,
Raymes

On Tue Nov 25 2014 at 5:42:19 AM <mkwst@chromium.org> wrote:

> On 2014/11/24 18:26:48, kalman wrote:
> > Be aware of https://codereview.chromium.org/747403002/.
>
> Given that flash can hop out of the sandbox with user permission, I'd
> strongly
> suggest limiting plugins to secure origins. Is there a use case for loading
> arbitrary plugin data over HTTP?
>
> https://codereview.chromium.org/754713002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[raymes, 2014-11-25 00:52:10]

Actually can we allow this CL to go ahead for now given that we're only
allowing this for component extensions and Flash still requires user
permission to escape the sandbox (which is a BIG hammer regardless of
whether the flash was embedded in an extension or on the drive-by-web)? I
can update the comment to reflect this discussion?

Thanks,
Raymes

On Tue Nov 25 2014 at 10:16:39 AM Raymes Khoury <raymes@chromium.org> wrote:

> Hey Mike. Thanks for commenting about flash, it's a good point and I
> forgot that flash could escape the sandbox. Part of the reason why I
> limited this change to component extensions was in case there was something
> like this which I hadn't considered.
>
> The reason we need to allow http URLs in object-src is because we have an
> extension which acts as a MIME type handler for PDFs. The extension uses a
> plugin to help render the PDF and the plugin gets loaded to the URL of the
> pdf that is being opened.
>
> One potential solution is to limit the plugins that can be loaded in the
> CSP using the plugin-types directive. In other words, if object-src * is
> specified, also require plugin-types to be specified and require it to only
> have white-listed mime types. The PDF plugin, as well as pnacl objects
> should be safe to load to any URL inside an extension.
>
> How would you feel about this approach?
>
> Thanks,
> Raymes
>
> On Tue Nov 25 2014 at 5:42:19 AM <mkwst@chromium.org> wrote:
>
>> On 2014/11/24 18:26:48, kalman wrote:
>> > Be aware of https://codereview.chromium.org/747403002/.
>>
>> Given that flash can hop out of the sandbox with user permission, I'd
>> strongly
>> suggest limiting plugins to secure origins. Is there a use case for
>> loading
>> arbitrary plugin data over HTTP?
>>
>> https://codereview.chromium.org/754713002/
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mike West, 2014-11-25 08:05:33]

On 2014/11/25 00:52:10, raymes (OOO 11-27 to 12-14) wrote:
> Actually can we allow this CL to go ahead for now given that we're only
> allowing this for component extensions and Flash still requires user
> permission to escape the sandbox (which is a BIG hammer regardless of
> whether the flash was embedded in an extension or on the drive-by-web)? I
> can update the comment to reflect this discussion?

Could we adjust the change such that we'd allow `object-src *` if and only if a
`plugin-types` directive was present with some whitelisted set of mime types
(which, I'd hope, wouldn't include Flash)? I'd be happier with that approach.

That said, I am not the right guy to block changes to the extension ecosystem.
I'm concerned about this change, but if kalman@ accepts it, then I'm not going
to block it.

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

Ok, thanks Mike, that sounds reasonable. Since this CL has been reviewed and
it's also holding up another conflicting CL, I'd like to go ahead and land this
now but I have uploaded a separate CL with the changes you suggested:
https://codereview.chromium.org/760513003/

Thanks!

https://codereview.chromium.org/754713002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/754713002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:63: this.plugin_ =
document.createElement('embed');
On 2014/11/24 18:18:02, kalman wrote:
> Could you make these changes to pdf in a separate change?

Done.

https://codereview.chromium.org/754713002/diff/20001/extensions/common/csp_va...
File extensions/common/csp_validator.h (right):

https://codereview.chromium.org/754713002/diff/20001/extensions/common/csp_va...
extensions/common/csp_validator.h:21: enum Options {
On 2014/11/24 18:18:02, kalman wrote:
> On 2014/11/24 04:40:15, Sam McNally wrote:
> > Comments?
> 
> when you do: please add a comment to ALLOW_INSECURE_OBJECT_SRC mentioning what
> exactly makes it "insecure" (otherwise it's quite open to interpretation).

Done.

https://codereview.chromium.org/754713002/diff/20001/extensions/common/csp_va...
extensions/common/csp_validator.h:21: enum Options {
On 2014/11/24 04:40:15, Sam McNally wrote:
> Comments?

Done.

https://codereview.chromium.org/754713002/diff/20001/extensions/common/csp_va...
extensions/common/csp_validator.h:22: NO_OPTIONS = 0x00,
On 2014/11/24 04:40:15, Sam McNally wrote:
> Prefix values with OPTIONS_.

Done.

https://codereview.chromium.org/754713002/diff/20001/extensions/common/csp_va...
extensions/common/csp_validator.h:24: ALLOW_INSECURE_OBJECT_SRC = 0x02,
On 2014/11/24 18:18:02, kalman wrote:
> Let's keep discussing this enum: More typical style is to use (1 << 1), (1 <<
> 2), etc.

Done.

https://codereview.chromium.org/754713002/diff/20001/extensions/common/csp_va...
extensions/common/csp_validator.h:38: const std::string& policy, int options);
On 2014/11/24 18:18:03, kalman wrote:
> Mention that this is a bitmask.

Done.

https://codereview.chromium.org/754713002/diff/20001/extensions/common/csp_va...
File extensions/common/csp_validator_unittest.cc (right):

https://codereview.chromium.org/754713002/diff/20001/extensions/common/csp_va...
extensions/common/csp_validator_unittest.cc:180: "script-src 'self'; object-src
*", 0));
On 2014/11/24 04:40:15, Sam McNally wrote:
> NO_OPTIONS.

Done.

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

LGTM, given the other CL. Thanks!

[not at google - send to devlin, 2014-11-25 16:36:28]

> Could we adjust the change such that we'd allow `object-src *` if and only if
a
> `plugin-types` directive was present with some whitelisted set of mime types
> (which, I'd hope, wouldn't include Flash)? I'd be happier with that approach.

I very much like this idea.

> 
> That said, I am not the right guy to block changes to the extension ecosystem.
> I'm concerned about this change, but if kalman@ accepts it, then I'm not going
> to block it.

There are some other changes flying around here. I'll try and figure out what's
going on.

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

Ah you broke it off into another CL. I don't need to look at this one again,
right?

[raymes, 2014-11-25 22:19:20]

On 2014/11/25 17:04:04, kalman wrote:
> Ah you broke it off into another CL. I don't need to look at this one again,
> right?

Right! :)

[raymes, 2014-11-25 22:19:25]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-11-25 22:20:29]

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

[commit-bot: I haz the power, 2014-11-25 23:25:13]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-11-25 23:26:10]

Patchset 6 (id:??) landed as
https://crrev.com/f43814b9553177aa71db780ddac7a3a4554a360c
Cr-Commit-Position: refs/heads/master@{#305725}