Remove ExtensionURLInfo, make security decisions in render process

Remove ExtensionURLInfo, make security decisions in render process

When asking if an extension should have access to a given frame, we need to
consider the frame's URL and also if the frame is sandboxed. We check the latter
by asking if the frame's security origin is the unique origin. However, we can
only usefully do this in the render process when examining a frame. In the
browser process or other common code, there's no useful origin to use other than
one that duplicates information in the URL.

This does security checks in the render process before doing any URL-based
lookups and then uses URLs from that point on.

R=abarth, mpcomplete
BUG=259982, 237267
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212302

[abarth-chromium, 2013-06-10 23:12:32]

https://codereview.chromium.org/16625012/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_function_dispatcher.cc (left):

https://codereview.chromium.org/16625012/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_function_dispatcher.cc:361:
WebSecurityOrigin::createFromString(params.source_origin),
Should we check source_origin here?

[jamesr, 2013-06-10 23:17:25]

Matt, Adam - mind taking a look at this change?

[jamesr, 2013-06-10 23:36:46]

https://codereview.chromium.org/16625012/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_function_dispatcher.cc (left):

https://codereview.chromium.org/16625012/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_function_dispatcher.cc:361:
WebSecurityOrigin::createFromString(params.source_origin),
On 2013/06/10 23:12:32, abarth wrote:
> Should we check source_origin here?

Yes. In particular, if the source_origin is unique this check should fail - but
that appears to be the only use of the source_origin IPC parameter.  We could
replace it with a bool or, since all checks will fail on a call from a unique
origin, make the source_url be an empty/invalid GURL when the source origin is
unique

[Charlie Reis, 2013-06-11 00:04:43]

Drive-by question: I'm concerned about the "make security decisions in render
process" part.  Is this something the browser could check if it knew the frame's
URL and whether it were sandboxed?  We're adding some of that type of
information to FrameTreeNode to support out-of-process iframes.

If this would make the security check feasible to put in the browser process,
feel free to file a bug on Nasko and me to fix it when the support is there.

[jamesr, 2013-06-11 00:17:31]

More precisely I'm moving code that makes security decisions to a different
place - I'm not (deliberately) changing any checks.  In particular, code that
needs to know if a given origin is a unique origin (to know whether to inject
extension script into a sandboxed iframe, for instance) used to do so by passing
a WebSecurityOrigin to ExtensionURLInfo.  However, this value was never useful
in the browser process.  So, I moved the calls in the render process to check
for a unique origin before constructing a URL at all.

Adam pointed out one place where the check needs to be a bit better.

[Charlie Reis, 2013-06-11 00:47:59]

On 2013/06/11 00:17:31, jamesr wrote:
> More precisely I'm moving code that makes security decisions to a different
> place - I'm not (deliberately) changing any checks.  In particular, code that
> needs to know if a given origin is a unique origin (to know whether to inject
> extension script into a sandboxed iframe, for instance) used to do so by
passing
> a WebSecurityOrigin to ExtensionURLInfo.  However, this value was never useful
> in the browser process.  So, I moved the calls in the render process to check
> for a unique origin before constructing a URL at all.
> 
> Adam pointed out one place where the check needs to be a bit better.

Ok.  I don't see a big threat to having a compromised renderer change this
decision currently anyway, since it can do worse damage than letting an
extension run on its iframe.  I'll make a note to revisit it with out-of-process
iframes, though.  Thanks.

[jamesr, 2013-06-11 01:50:23]

Yes, in the current state of the code the browser process just has to trust
whatever a renderer claims about the frames loaded into it.  When the browser
knows more about which frames should be in which renderer it can definitely do
better.

[jamesr, 2013-06-11 20:07:17]

On 2013/06/10 23:12:32, abarth wrote:
>
https://codereview.chromium.org/16625012/diff/1/chrome/browser/extensions/ext...
> File chrome/browser/extensions/extension_function_dispatcher.cc (left):
> 
>
https://codereview.chromium.org/16625012/diff/1/chrome/browser/extensions/ext...
> chrome/browser/extensions/extension_function_dispatcher.cc:361:
> WebSecurityOrigin::createFromString(params.source_origin),
> Should we check source_origin here?

Unfortunately, I'm pretty sure the code as exists in PS1 is incorrect, but I
can't find any tests that fail due to this.  I've tried running all of
browser_tests and unit_tests with 'Extensions' in the name.  Anyone know where
else I might find test coverage for this?  If not, I'll try to write up
something new.

[Matt Perry, 2013-06-11 21:17:45]

This feels like decentralizing the IsSandbox check away from ExtensionSet and
into a bunch of different calling code. Why is that desirable?

[jamesr, 2013-06-11 21:21:06]

The browser side won't have access to WebSecurityOrigin soon and it can't do
anything useful with this class today.  Another way to factor this would be to
centralize these checks into a place that's renderer-specific and have browser
side code use a different API to query the extension set, but that seems like
overkill.

[Matt Perry, 2013-06-12 00:13:14]

On 2013/06/11 21:21:06, jamesr wrote:
> Another way to factor this would be to
> centralize these checks into a place that's renderer-specific and have browser
> side code use a different API to query the extension set

Right. I guess I don't feel that strongly about it. LGTM

[asargent_no_longer_on_chrome, 2013-06-12 23:12:21]

On 2013/06/11 20:07:17, jamesr wrote:
> Unfortunately, I'm pretty sure the code as exists in PS1 is incorrect, but I
> can't find any tests that fail due to this.  I've tried running all of
> browser_tests and unit_tests with 'Extensions' in the name.  Anyone know where
> else I might find test coverage for this?  If not, I'll try to write up
> something new.

Maybe try running with --gtest_filter='*App*' ?

[jamesr, 2013-07-10 20:38:58]

https://codereview.chromium.org/16625012/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_function_dispatcher.cc (left):

https://codereview.chromium.org/16625012/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_function_dispatcher.cc:361:
WebSecurityOrigin::createFromString(params.source_origin),
On 2013/06/10 23:36:46, jamesr wrote:
> On 2013/06/10 23:12:32, abarth wrote:
> > Should we check source_origin here?
> 
> Yes. In particular, if the source_origin is unique this check should fail -
but
> that appears to be the only use of the source_origin IPC parameter.  We could
> replace it with a bool or, since all checks will fail on a call from a unique
> origin, make the source_url be an empty/invalid GURL when the source origin is
> unique

Actually, this will be the string "null" if the origin is unique. This field is
populated here:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex...

and toString() on a unique WebSecurityOrigin is defined to return "null", so
this'll never match for a unique origin.  So this code is fine as is.

[jamesr, 2013-07-10 20:39:55]

https://codereview.chromium.org/16625012/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_function_dispatcher.cc (left):

https://codereview.chromium.org/16625012/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_function_dispatcher.cc:361:
WebSecurityOrigin::createFromString(params.source_origin),
Sorry, the source_origin will be "null", so we can just check that.

[jamesr, 2013-07-12 23:22:05]

OK, I believe this code is actually correct without any special origin handling.
 I've documented my confusion and conclusions about sandbox handling here:

https://code.google.com/p/chromium/issues/detail?id=259982

Matt - this is the same patch that you approved earlier, but feel free to take
another look or ask another member of the extension team to double-check that my
understanding of the sandbox code is correct.

Adam - could you sanity check the origin changes?
Nico - could you OWNERS-approve the non-extension changes in chrome?
Tom - could you approve the field removal from extension_messages.h?

[Tom Sepez, 2013-07-12 23:25:51]

Rubberstamp LGTM on removing member,

[abarth-chromium, 2013-07-13 00:25:39]

I didn't see any problems, but I'm not sure I would even if they were there.  I
guess that translates to an LGTM.  :)

https://codereview.chromium.org/16625012/diff/31001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/16625012/diff/31001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1017: GURL effective_frame_url;
Can you call this effective_document_url?  Frames don't have URLs.  They're just
containers.  It's like asking whether a car has brown hair.  It depends who's
driving it.

https://codereview.chromium.org/16625012/diff/31001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1019: effective_frame_url =
UserScriptSlave::GetDataSourceURLForFrame(frame);
This crazy code, huh?

[Nico, 2013-07-14 00:07:13]

On 2013/07/12 23:22:05, jamesr wrote:
> OK, I believe this code is actually correct without any special origin
handling.
>  I've documented my confusion and conclusions about sandbox handling here:
> 
> https://code.google.com/p/chromium/issues/detail?id=259982

Please add BUG=259982 to the CL description.

> 
> Matt - this is the same patch that you approved earlier, but feel free to take
> another look or ask another member of the extension team to double-check that
my
> understanding of the sandbox code is correct.
> 
> Adam - could you sanity check the origin changes?
> Nico - could you OWNERS-approve the non-extension changes in chrome?
> Tom - could you approve the field removal from extension_messages.h?

[Nico, 2013-07-14 00:12:40]

non-extensions chrome/ lgtm (with BUG= line in CL description added as mentioned
above). There's one non-mechanical change below that I hope abarth checked in
his review.

https://codereview.chromium.org/16625012/diff/31001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/16625012/diff/31001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:1143: !opener.isUnique() &&
!!extensions.GetExtensionOrAppByURL(
I assume abarth has checked that !opener.isUnique() is correct here.

(nit: No need for !! here, remove it. If you want to stress the boolness of the
context, add != NULL at the end instead. Yes, you just moved this two lines
down, still.)

[jamesr, 2013-07-15 19:31:11]

https://codereview.chromium.org/16625012/diff/31001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/16625012/diff/31001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:1143: !opener.isUnique() &&
!!extensions.GetExtensionOrAppByURL(
On 2013/07/14 00:12:41, Nico wrote:
> I assume abarth has checked that !opener.isUnique() is correct here.
> 
> (nit: No need for !! here, remove it. If you want to stress the boolness of
the
> context, add != NULL at the end instead. Yes, you just moved this two lines
> down, still.)

The previous code passed the opener to GetExtensionOrAppByURL(), which returned
NULL if .isUnique() was true.  I believe this preserves the behavior, but I'm
not really sure this behavior makes a lot of sense.  I'd rather keep it the same
to be safe.

Fixed the bool coersion.

[Matt Perry, 2013-07-15 21:24:30]

+kalman, could you take a look at the changes to dispatcher.cc?

https://codereview.chromium.org/16625012/diff/43001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/16625012/diff/43001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1024: extension_id, extension_group,
effective_frame_url);
The call to IsSandboxedPage in ClassifyJavaScriptContext will definitely return
false in the case that the security origin is unique. I have no idea if that's
what we want.

Maybe kalman knows.

[not at google - send to devlin, 2013-07-15 22:09:12]

+miket, are you the sandboxed page reference these days?

So long as we actually do set the web-sandboxing bit for extension-sandboxed
pages then it seems like basically the extension system doesn't need to know
about web-sandboxing? Apologies if I'm misunderstanding things here.

https://codereview.chromium.org/16625012/diff/43001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/16625012/diff/43001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:1143: !opener.isUnique() &&
extensions.GetExtensionOrAppByURL(
not totally sure why the isUnique check is needed here, but it does seem like
this condition should be folded into the expression below, because either way
isUnique has nothing to do about whether the opener is an extension URL.

https://codereview.chromium.org/16625012/diff/43001/chrome/renderer/extension...
File chrome/renderer/extensions/app_bindings.cc (right):

https://codereview.chromium.org/16625012/diff/43001/chrome/renderer/extension...
chrome/renderer/extensions/app_bindings.cc:123: return v8::Null();
i don't think it makes sense to check isUnique here

https://codereview.chromium.org/16625012/diff/43001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/16625012/diff/43001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1018: // Frames loaded on a unique
security origin are not accessible to extensions.
I've convinced myself that this seems kind of arbitrary and we should be
allowing it.

https://codereview.chromium.org/16625012/diff/43001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1024: extension_id, extension_group,
effective_frame_url);
On 2013/07/15 21:24:31, Matt Perry wrote:
> The call to IsSandboxedPage in ClassifyJavaScriptContext will definitely
return
> false in the case that the security origin is unique. I have no idea if that's
> what we want.
> 
> Maybe kalman knows.

yes I think this is another place where isUnique isn't needed

https://codereview.chromium.org/16625012/diff/43001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1133: return std::string();
also here

https://codereview.chromium.org/16625012/diff/43001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1439:
UserScriptSlave::GetDataSourceURLForFrame(frame))) {
I don't actually think we should be preventing scripts from running in
web-sandboxed pages, just extension-sandboxed ones (then let host permissions do
the rest).

I think this would prevent extensions from running content scripts on file://
URLs which isn't desirable.

[not at google - send to devlin, 2013-07-15 22:27:09]

https://codereview.chromium.org/16625012/diff/43001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/16625012/diff/43001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1439:
UserScriptSlave::GetDataSourceURLForFrame(frame))) {
On 2013/07/15 22:09:12, kalman wrote:
> I don't actually think we should be preventing scripts from running in
> web-sandboxed pages, just extension-sandboxed ones (then let host permissions
do
> the rest).
> 
> I think this would prevent extensions from running content scripts on file://
> URLs which isn't desirable.

Ok I'm being dense and mixed up host and API permissions.

However it would prevent content scripts from calling any APIs on file:// URLs,
like chrome.runtime.sendMessage, which is nevertheless undesirable.

[jamesr, 2013-07-15 22:55:26]

https://codereview.chromium.org/16625012/diff/43001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/16625012/diff/43001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1439:
UserScriptSlave::GetDataSourceURLForFrame(frame))) {
On 2013/07/15 22:27:10, kalman wrote:
> However it would prevent content scripts from calling any APIs on file://
URLs,
> like chrome.runtime.sendMessage, which is nevertheless undesirable.

It appears that chrome.runtime.sendMessage() fails in content scripts injected
into file:// URLs with or without this patch.  I agree that's bad but am not
sure exactly how it happens.  Removing this call doesn't make
chrome.runtime.sendMessage() work.

[not at google - send to devlin, 2013-07-15 23:00:52]

https://codereview.chromium.org/16625012/diff/43001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/16625012/diff/43001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1439:
UserScriptSlave::GetDataSourceURLForFrame(frame))) {
On 2013/07/15 22:55:27, jamesr wrote:
> On 2013/07/15 22:27:10, kalman wrote:
> > However it would prevent content scripts from calling any APIs on file://
> URLs,
> > like chrome.runtime.sendMessage, which is nevertheless undesirable.
> 
> It appears that chrome.runtime.sendMessage() fails in content scripts injected
> into file:// URLs with or without this patch.  I agree that's bad but am not
> sure exactly how it happens.  Removing this call doesn't make
> chrome.runtime.sendMessage() work.

Yeah looks like there's an existing issue at play here too. However, as
discussed, removing the isUnque check here did make the chrome.storage functions
work?

[jamesr, 2013-07-16 01:06:56]

On 2013/07/15 23:00:52, kalman wrote:
>
https://codereview.chromium.org/16625012/diff/43001/chrome/renderer/extension...
> File chrome/renderer/extensions/dispatcher.cc (right):
> 
>
https://codereview.chromium.org/16625012/diff/43001/chrome/renderer/extension...
> chrome/renderer/extensions/dispatcher.cc:1439:
> UserScriptSlave::GetDataSourceURLForFrame(frame))) {
> On 2013/07/15 22:55:27, jamesr wrote:
> > On 2013/07/15 22:27:10, kalman wrote:
> > > However it would prevent content scripts from calling any APIs on file://
> > URLs,
> > > like chrome.runtime.sendMessage, which is nevertheless undesirable.
> > 
> > It appears that chrome.runtime.sendMessage() fails in content scripts
injected
> > into file:// URLs with or without this patch.  I agree that's bad but am not
> > sure exactly how it happens.  Removing this call doesn't make
> > chrome.runtime.sendMessage() work.
> 
> Yeah looks like there's an existing issue at play here too. However, as
> discussed, removing the isUnque check here did make the chrome.storage
functions
> work?

chrome.storage in a content script injected into a file:/// page works fine with
or without this check.

[not at google - send to devlin, 2013-07-17 23:48:24]

lgtm. had another look at the renderer code, and as I mentioned in the bug, I'll
go through after this and fix up the actual sandboxing logic.

https://codereview.chromium.org/16625012/diff/57001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/16625012/diff/57001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1023: effective_frame_url =
UserScriptSlave::GetDataSourceURLForFrame(frame);
It's not clear to me why this change is equivalent. If you pass in the origin +
the URL separately into ClassifyJavaScriptContext it would be clearer.

https://codereview.chromium.org/16625012/diff/57001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1134: if
(frame->document().securityOrigin().isUnique())
TODO(kalman): delete me.

https://codereview.chromium.org/16625012/diff/57001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1419: if
(extensions_.ExtensionBindingsAllowed(url)) {
and then this would be

if (!origin.isUnique() && ...)
  ...

https://codereview.chromium.org/16625012/diff/57001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1453: if
(frame->document().securityOrigin().isUnique() ||
the isUnique() check here doesn't maintain the existing behaviour.

[jamesr, 2013-07-18 00:12:12]

https://codereview.chromium.org/16625012/diff/57001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/16625012/diff/57001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1023: effective_frame_url =
UserScriptSlave::GetDataSourceURLForFrame(frame);
On 2013/07/17 23:48:24, kalman wrote:
> It's not clear to me why this change is equivalent. If you pass in the origin
+
> the URL separately into ClassifyJavaScriptContext it would be clearer.

Good point.  Adding the parameter

https://codereview.chromium.org/16625012/diff/57001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1134: if
(frame->document().securityOrigin().isUnique())
On 2013/07/17 23:48:24, kalman wrote:
> TODO(kalman): delete me.

Done.

https://codereview.chromium.org/16625012/diff/57001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1419: if
(extensions_.ExtensionBindingsAllowed(url)) {
On 2013/07/17 23:48:24, kalman wrote:
> and then this would be
> 
> if (!origin.isUnique() && ...)
>   ...

..and checked it here, with a TODO(kalman) to take care of this in
IsSandboxedPage() instead

https://codereview.chromium.org/16625012/diff/57001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1453: if
(frame->document().securityOrigin().isUnique() ||
On 2013/07/17 23:48:24, kalman wrote:
> the isUnique() check here doesn't maintain the existing behaviour.

Right, removed.

[commit-bot: I haz the power, 2013-07-18 00:24:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/16625012/63001

[commit-bot: I haz the power, 2013-07-18 09:28:58]

Change committed as 212302