`chrome-extension` resources should bypass Content Security Policy.

`chrome-extension` resources should bypass Content Security Policy.

The Content Security Policy specification[1] notes that "Enforcing a CSP
policy should not interfere with the operation of user-supplied scripts
such as third-party user-agent add-ons and JavaScript bookmarklets."
To that end, this CL allows `chrome-extension` resources to bypass
their containing document's policy, meaning that (for example)
`chrome-extension://[ID]/image.png` can be injected into a page, even
if that page has an `img-src 'none';` CSP directive.

[1]: https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html#processing-model

BUG=133223
TBR=thakis@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149627

[Mike West, 2012-07-15 18:38:08]

Hi Aaron, Adam,

I'd appreciate you both taking a look at this CL. I think I've found the correct
spot to register the scheme (e.g. it passes the test), but I'm not sure. WDYT?

Also: jschuh@, tsepez@. One impact of this change that should be considered is
that it negates some of the protection that's been added to the various
`chrome://` pages (settings and so forth). I don't think there's a good
mechanism for enabling extension resources to bypass CSP on the web without
allowing them to bypass CSP for those pages as well. I'd love to be wrong about
that, however.

Thanks, all!

-mike

[abarth-chromium, 2012-07-16 01:16:48]

I didn't review the tests, but the code change LGTM.  It's true that this
weakens the protections for the chrome scheme somewhat, but an attacker would
need to have already installed an extension for that to be a problem, so I don't
think we should worry about it too much.

[jschuh, 2012-07-16 04:09:59]

Stupid question, but what happens with script injected into the origin of an
extension or app?

[abarth-chromium, 2012-07-16 04:15:50]

On 2012/07/16 04:09:59, Justin Schuh wrote:
> Stupid question, but what happens with script injected into the origin of an
> extension or app?

I'm not sure I understand your question.  This patch makes it act as if
chrome-extension and chrome-extension-resource were always whitelisted as an
allowed source in every CSP policy.

[Tom Sepez, 2012-07-16 18:35:27]

Mike, it looks like this policy will be applied to all pages.  I think that's
perhaps too broad if it results in:
- WebUI pages being xss-able using resources from any extension, or
- Ordinary pages being xss-able using resources from extensions whose permission
model would prohibit access to the page.

For example, if I have a stupid extension whose scripts open up a type 3 (dom
based) XSS, but the extension permissions is restricted only to stupid.com, it
would be a shame to allow that to propogate onto other pages using this
mechanism.

[Mike West, 2012-07-17 02:28:59]

On 2012/07/16 18:35:27, Tom Sepez wrote:
> Mike, it looks like this policy will be applied to all pages.  I think that's
> perhaps too broad if it results in:
> - WebUI pages being xss-able using resources from any extension, or
> - Ordinary pages being xss-able using resources from extensions whose
permission
> model would prohibit access to the page.

I think that's a very reasonable goal, but I also think that CSP is the wrong
place for the browser to make that sort of promise. Extensions, generally
speaking, violate the wishes of websites intentionally in favor of (ideally) the
user's wishes. That's their power, and the reason that users install them in the
first place.

If we want to ensure that an extension's resources can only be loaded by pages
for which it has permissions, we should make that promise in the extension
system, not outside of it. Aaron, what do you think about that? Could the
`web_accessible_resources` code be somehow modified to additionally check that
the requesting origin is permitted?

That said, WebUI is possibly a different story. I agree with Adam that it's not
a huge concern, but if anyone sees a good mechanism for making this change only
applicable to the open web, I'm all for it. I don't, unfortunately.

[Mike West, 2012-07-19 05:01:06]

Ideally, I'd like this to be in for M22, which, in theory, means it has to land
by Sunday.

I'm out on vacation (again, in theory) at the moment, so I'd appreciate it if
you poor folks who are trapped in the office all day this week could make a
decision together about whether this is a good mechanism to enact the change.
Perhaps Justin, Adam, and Tom could get together and give a collective thumbs
up/down? :)

Thanks!

[Mike West, 2012-07-26 07:12:08]

On 2012/07/19 05:01:06, Mike West (chromium) wrote:
> Ideally, I'd like this to be in for M22, which, in theory, means it has to
land
> by Sunday.
> 
> I'm out on vacation (again, in theory) at the moment, so I'd appreciate it if
> you poor folks who are trapped in the office all day this week could make a
> decision together about whether this is a good mechanism to enact the change.
> Perhaps Justin, Adam, and Tom could get together and give a collective thumbs
> up/down? :)
> 
> Thanks!

Friendly ping. I'm back in the office, so perhaps it would be useful to sit down
and chat?

[Tom Sepez, 2012-07-26 17:50:25]

Sorry to take so long to get back to you.  I'm of the opinion that it's not a
great mechanism, but I also don't know of reasonable alternatives apart from the
mechanism that you suggested to Aaron.  Did you hear back from him about whether
the extension system can help mitigate the impact of this change?

[Use mkwst_at_chromium.org plz., 2012-07-26 18:35:58]

On 2012/07/26 17:50:25, Tom Sepez wrote:
> Sorry to take so long to get back to you.  I'm of the opinion that it's
not a
> great mechanism, but I also don't know of reasonable alternatives apart from
the
> mechanism that you suggested to Aaron.  Did you hear back from him about
whether
> the extension system can help mitigate the impact of this change?

I haven't. Friendly ping @aa. :)

Regardless, that work would have to happen in a separate CL. Given that context,
and assuming that whatever the extension system could do wouldn't be done in
time for M22, would you be ok with landing this as-is now? Or do you see some
sort of mitigation mechanism for `chrome://*` pages as a blocking issue?

[Aaron Boodman, 2012-07-30 11:42:45]

Apologies.

So we used to do something like this actually, and I reverted it after fighting
through several subtle issues. I apparently meant to get back to it, but never
did. See: http://codereview.chromium.org/10533086/ and
http://code.google.com/p/chromium/issues/detail?id=72407.

It looks like the last issue was that somehow content scripts weren't getting
handled -- only explicit host permissions.

We could try reintroducing that part of the patch
(http://codereview.chromium.org/6478019/diff/12/chrome/renderer/extensions/ext...
- line 40), and see how it goes. Now (M23) is a good time to do this because we
have plenty of time to see how it goes on canary.

Mike, do you feel comfortable with this, or do you want someone more familiar to
take it over.

[Mike West, 2012-07-30 12:19:51]

On 2012/07/30 11:42:45, Aaron Boodman wrote:
> Apologies.
> 
> So we used to do something like this actually, and I reverted it after
fighting
> through several subtle issues. I apparently meant to get back to it, but never
> did. See: http://codereview.chromium.org/10533086/ and
> http://code.google.com/p/chromium/issues/detail?id=72407.
> 
> It looks like the last issue was that somehow content scripts weren't getting
> handled -- only explicit host permissions.
> 
> We could try reintroducing that part of the patch
>
(http://codereview.chromium.org/6478019/diff/12/chrome/renderer/extensions/ext...
> - line 40), and see how it goes. Now (M23) is a good time to do this because
we
> have plenty of time to see how it goes on canary.
> 
> Mike, do you feel comfortable with this, or do you want someone more familiar
to
> take it over.

Thanks for the pointer, I'll take a look at that code. If I get lost, I'll ask
for help.

[Mike West, 2012-07-30 14:05:25]

On 2012/07/16 18:35:27, Tom Sepez wrote:

> For example, if I have a stupid extension whose scripts open up a type 3 (dom
> based) XSS, but the extension permissions is restricted only to
http://stupid.com, it
> would be a shame to allow that to propogate onto other pages using this
> mechanism.

Tom: I'm looking at this feature in
https://code.google.com/p/chromium/issues/detail?id=139592.  Assuming it lands
and works, are you ok with me landing this CL?

[Tom Sepez, 2012-07-30 16:44:28]

SGTM.

[Mike West, 2012-08-02 11:20:44]

On 2012/07/30 16:44:28, Tom Sepez wrote:
> SGTM.

A fix for https://code.google.com/p/chromium/issues/detail?id=139592 just
landed. I plan to land this change as well so that both changes are in the next
Canary for evaluation.

Once the M22 branch point happens, I'll revert both from the beta branch to give
them a full cycle to bake on dev.

[Aaron Boodman, 2012-08-02 11:33:14]

LGTM

[commit-bot: I haz the power, 2012-08-02 13:08:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/10792008/28002

[commit-bot: I haz the power, 2012-08-02 13:09:00]

Presubmit check for 10792008-28002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.3s to calculate.

[Mike West, 2012-08-02 13:12:46]

On 2012/08/02 13:09:00, I haz the power (commit-bot) wrote:
> Presubmit check for 10792008-28002 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     chrome
> 
> Presubmit checks took 1.3s to calculate.

Gah. No one's an owner for chrome/renderer/chrome_content_renderer_client.cc.

I'm going to TBR this to make sure it's in the next Canary along with the other
CL. Sorry. :/

[Mike West, 2012-08-02 13:14:15]

Nico, through the magic of picking a name from chrome/OWNERS at random, I've
chosen you to TBR this change against. Apologies.

[commit-bot: I haz the power, 2012-08-02 13:14:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/10792008/28002

[commit-bot: I haz the power, 2012-08-02 15:00:24]

Change committed as 149627

[Nico, 2012-08-02 15:05:32]

lgtm stamp