Non-web-accessible extension URLs should not load in non-extension processes

Non-web-accessible extension URLs should not load in non-extension processes

Blocking of URL loads in the browser process cannot be as extensive as the one done in the renderer process, as we don't have the frame URL and page URL at resource load time. I've tried to do a similar check with the data available on the browser side.

The main part which I'm not entirely happy about is the check for DevTools pages. Currently, if an extension has DevTools page, all of its resources can be loaded by a compromised renderer. I think this can be tightened up with a follow up CL, if we find a good way of distinguishing DevTools processes or permissioning those requests.

BUG=173688
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184245

[nasko, 2013-02-20 19:28:36]

Hey Chris,
Can you review this CL for me? It is not ideal, but I think it is a good first
step. The main piece I'm not totally happy is the DevTools check.
Thanks,
Nasko

[nasko, 2013-02-21 18:54:12]

Hey Matt,
Can you review this change?
Thanks,
Nasko

[Matt Perry, 2013-02-21 20:24:43]

+creis who wrote ResourceRequestPolicy::CanRequestResource.

https://codereview.chromium.org/12218064/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_protocols.cc (left):

https://codereview.chromium.org/12218064/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/extension_protocols.cc:358: extension)) &&
I don't understand what this was trying to do, but your patch changes the
behavior. If an extension is manifest v1 and has no web_accessible_resources
key, then we previously wouldn't send a CORS header. With your patch, we will.

It seems like your change is correct, but I'm confused how this worked before...

Looking at the original bug
(https://code.google.com/p/chromium/issues/detail?id=109686) I wonder if we were
purposely being more restrictive for manifest v1.

https://codereview.chromium.org/12218064/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_protocols.cc (right):

https://codereview.chromium.org/12218064/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/extension_protocols.cc:287: // renderer process,
performed by ResourceRequestPolicy::CanRequestResource.
Can you refactor that method to somewhere in chrome/common so the checks are
shared? (web_accessible_resources_handler might be a good plaec). I know you
don't have frame_url here, but it looks like you could give it page_url for
both.

[nasko, 2013-02-21 21:41:06]

https://codereview.chromium.org/12218064/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_protocols.cc (left):

https://codereview.chromium.org/12218064/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/extension_protocols.cc:358: extension)) &&
On 2013/02/21 20:24:43, Matt Perry wrote:
> I don't understand what this was trying to do, but your patch changes the
> behavior. If an extension is manifest v1 and has no web_accessible_resources
> key, then we previously wouldn't send a CORS header. With your patch, we will.
> 
> It seems like your change is correct, but I'm confused how this worked
before...
> 
> Looking at the original bug
> (https://code.google.com/p/chromium/issues/detail?id=109686) I wonder if we
were
> purposely being more restrictive for manifest v1.

I tried to simplify the code, but it looks I've made a logic mistake. Reverting
this change.

https://codereview.chromium.org/12218064/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_protocols.cc (right):

https://codereview.chromium.org/12218064/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/extension_protocols.cc:287: // renderer process,
performed by ResourceRequestPolicy::CanRequestResource.
On 2013/02/21 20:24:43, Matt Perry wrote:
> Can you refactor that method to somewhere in chrome/common so the checks are
> shared? (web_accessible_resources_handler might be a good plaec). I know you
> don't have frame_url here, but it looks like you could give it page_url for
> both.

I actually tried to do that first, but it didn't end up sharing much code. The
only checks that could be reused verbatim were the page transition and hosted
app.

Where would I get the page URL from? I've looked at the both URLRequest and
ResourceRequestInfo, but none of them have the page or frame URL. We have frame
IDs, but we don't have a mapping between those and URLs that I'm aware of.

[Charlie Reis, 2013-02-21 21:44:25]

On 2013/02/21 20:24:43, Matt Perry wrote:
> +creis who wrote ResourceRequestPolicy::CanRequestResource.

I don't remember writing this.  :)  Did I forget?  From SVN annotate, looks like
aa@ moved it from the browser to the renderer, and cdn@ may have done some of
the earlier work.

[Matt Perry, 2013-02-21 21:45:21]

On 2013/02/21 21:44:25, creis wrote:
> On 2013/02/21 20:24:43, Matt Perry wrote:
> > +creis who wrote ResourceRequestPolicy::CanRequestResource.
> 
> I don't remember writing this.  :)  Did I forget?  From SVN annotate, looks
like
> aa@ moved it from the browser to the renderer, and cdn@ may have done some of
> the earlier work.

Sorry, I guess I misremembered :). Ignore me.

[Matt Perry, 2013-02-21 21:50:07]

lgtm

https://codereview.chromium.org/12218064/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_protocols.cc (right):

https://codereview.chromium.org/12218064/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/extension_protocols.cc:287: // renderer process,
performed by ResourceRequestPolicy::CanRequestResource.
On 2013/02/21 21:41:06, nasko wrote:
> On 2013/02/21 20:24:43, Matt Perry wrote:
> > Can you refactor that method to somewhere in chrome/common so the checks are
> > shared? (web_accessible_resources_handler might be a good plaec). I know you
> > don't have frame_url here, but it looks like you could give it page_url for
> > both.
> 
> I actually tried to do that first, but it didn't end up sharing much code. The
> only checks that could be reused verbatim were the page transition and hosted
> app.
> 
> Where would I get the page URL from? I've looked at the both URLRequest and
> ResourceRequestInfo, but none of them have the page or frame URL. We have
frame
> IDs, but we don't have a mapping between those and URLs that I'm aware of.

Interesting.. I could have sworn we had access to it, but I can't find it now.

Nevermind this then.

[nasko, 2013-02-21 22:20:03]

Adding jochen@ for chrome/ OWNERs review.

[jochen (gone - plz use gerrit), 2013-02-21 22:26:13]

lgtm

[Charlie Reis, 2013-02-22 02:45:17]

https://codereview.chromium.org/12218064/diff/17001/chrome/browser/chrome_sec...
File chrome/browser/chrome_security_exploit_browsertest.cc (right):

https://codereview.chromium.org/12218064/diff/17001/chrome/browser/chrome_sec...
chrome/browser/chrome_security_exploit_browsertest.cc:59: GURL
foo("http://foo.com/files/chrome_extension_resource.html");
Please mention that this page tries to load a chrome-extension:// image via an
XHR, which should cause it the process to be killed.

https://codereview.chromium.org/12218064/diff/17001/chrome/browser/extensions...
File chrome/browser/extensions/extension_protocols.cc (right):

https://codereview.chromium.org/12218064/diff/17001/chrome/browser/extensions...
chrome/browser/extensions/extension_protocols.cc:286: // The following checks
are meant to replicate similar set of checks in the
Should we add a similar warning to resource_request_policy.cc, so that people
who make changes there know they need to look here as well?  I would never be
able to find both places on my own.  :)

https://codereview.chromium.org/12218064/diff/17001/chrome/browser/extensions...
chrome/browser/extensions/extension_protocols.cc:288: // These are not exact
equivalent, because we don't have the same bits of
nit: exact -> exactly

https://codereview.chromium.org/12218064/diff/17001/chrome/browser/extensions...
chrome/browser/extensions/extension_protocols.cc:304: // Should we succeed or
fail here?
Can we decide this, rather than leaving it as a TODO?

If there's no extension found for the request's URL, then it seems safe to
return true here.

https://codereview.chromium.org/12218064/diff/17001/chrome/browser/extensions...
chrome/browser/extensions/extension_protocols.cc:311: // launchers.
Duplicating this code from resource_request_policy.cc makes me a bit
uncomfortable, but I guess it's a small enough chunk that it's not worth
factoring out, given your comment above.

https://codereview.chromium.org/12218064/diff/17001/chrome/browser/extensions...
chrome/browser/extensions/extension_protocols.cc:412: extension, resource_path))
{
nit: Technically this is legal by the style guide, so we probably shouldn't add
the code churn.  Just makes svn annotate pages harder to follow.

https://codereview.chromium.org/12218064/diff/17001/chrome/test/data/chrome_e...
File chrome/test/data/chrome_extension_resource.html (right):

https://codereview.chromium.org/12218064/diff/17001/chrome/test/data/chrome_e...
chrome/test/data/chrome_extension_resource.html:9:
'chrome-extension://eemcgdkfndhakfknompkggombfjjjeno/main.html');
Are we testing whether this is allowed?  Maybe add a comment saying that this is
allowed due to --disable-web-security in the test?

(I'm wondering what will happen if we disallow this pushState call later.)

[nasko, 2013-02-22 17:14:35]

Fixes for Charlie's comments.

https://codereview.chromium.org/12218064/diff/17001/chrome/browser/chrome_sec...
File chrome/browser/chrome_security_exploit_browsertest.cc (right):

https://codereview.chromium.org/12218064/diff/17001/chrome/browser/chrome_sec...
chrome/browser/chrome_security_exploit_browsertest.cc:59: GURL
foo("http://foo.com/files/chrome_extension_resource.html");
On 2013/02/22 02:45:17, creis wrote:
> Please mention that this page tries to load a chrome-extension:// image via an
> XHR, which should cause it the process to be killed.

This doesn't actually kill the renderer. It just denies the request. The
FilterURL CL will be killing renderers.

https://codereview.chromium.org/12218064/diff/17001/chrome/browser/extensions...
File chrome/browser/extensions/extension_protocols.cc (right):

https://codereview.chromium.org/12218064/diff/17001/chrome/browser/extensions...
chrome/browser/extensions/extension_protocols.cc:286: // The following checks
are meant to replicate similar set of checks in the
On 2013/02/22 02:45:17, creis wrote:
> Should we add a similar warning to resource_request_policy.cc, so that people
> who make changes there know they need to look here as well?  I would never be
> able to find both places on my own.  :)

Done.

https://codereview.chromium.org/12218064/diff/17001/chrome/browser/extensions...
chrome/browser/extensions/extension_protocols.cc:288: // These are not exact
equivalent, because we don't have the same bits of
On 2013/02/22 02:45:17, creis wrote:
> nit: exact -> exactly

Done.

https://codereview.chromium.org/12218064/diff/17001/chrome/browser/extensions...
chrome/browser/extensions/extension_protocols.cc:304: // Should we succeed or
fail here?
On 2013/02/22 02:45:17, creis wrote:
> Can we decide this, rather than leaving it as a TODO?
> 
> If there's no extension found for the request's URL, then it seems safe to
> return true here.

Why allow it if there is no existing extension for what is being requested? I'm
actually for a fail-safe behavior, so I've changed it to return false.

https://codereview.chromium.org/12218064/diff/17001/chrome/browser/extensions...
chrome/browser/extensions/extension_protocols.cc:311: // launchers.
On 2013/02/22 02:45:17, creis wrote:
> Duplicating this code from resource_request_policy.cc makes me a bit
> uncomfortable, but I guess it's a small enough chunk that it's not worth
> factoring out, given your comment above.

Done.

https://codereview.chromium.org/12218064/diff/17001/chrome/browser/extensions...
chrome/browser/extensions/extension_protocols.cc:412: extension, resource_path))
{
On 2013/02/22 02:45:17, creis wrote:
> nit: Technically this is legal by the style guide, so we probably shouldn't
add
> the code churn.  Just makes svn annotate pages harder to follow.

Done.

https://codereview.chromium.org/12218064/diff/17001/chrome/test/data/chrome_e...
File chrome/test/data/chrome_extension_resource.html (right):

https://codereview.chromium.org/12218064/diff/17001/chrome/test/data/chrome_e...
chrome/test/data/chrome_extension_resource.html:9:
'chrome-extension://eemcgdkfndhakfknompkggombfjjjeno/main.html');
On 2013/02/22 02:45:17, creis wrote:
> Are we testing whether this is allowed?  Maybe add a comment saying that this
is
> allowed due to --disable-web-security in the test?

Done.

> (I'm wondering what will happen if we disallow this pushState call later.)

This is one of the outstanding items we need to cover - behavior of pushState in
general.

[Charlie Reis, 2013-02-22 17:57:47]

Great.  I'll wait for the new patch based what you found in our offline chat
(allow disabled extensions in WebUI processes and check
chrome-extension-resource:// as well).

[Matt Perry, 2013-02-22 18:56:40]

https://codereview.chromium.org/12218064/diff/17001/chrome/browser/extensions...
File chrome/browser/extensions/extension_protocols.cc (right):

https://codereview.chromium.org/12218064/diff/17001/chrome/browser/extensions...
chrome/browser/extensions/extension_protocols.cc:304: // Should we succeed or
fail here?
On 2013/02/22 17:14:35, nasko wrote:
> On 2013/02/22 02:45:17, creis wrote:
> > Can we decide this, rather than leaving it as a TODO?
> > 
> > If there's no extension found for the request's URL, then it seems safe to
> > return true here.
> 
> Why allow it if there is no existing extension for what is being requested?
I'm
> actually for a fail-safe behavior, so I've changed it to return false.

It likely means the extension has been unloaded, so it's fine to disallow the
request. It will have failed anyway.

[nasko, 2013-02-22 19:03:14]

On 2013/02/22 18:56:40, Matt Perry wrote:
>
https://codereview.chromium.org/12218064/diff/17001/chrome/browser/extensions...
> File chrome/browser/extensions/extension_protocols.cc (right):
> 
>
https://codereview.chromium.org/12218064/diff/17001/chrome/browser/extensions...
> chrome/browser/extensions/extension_protocols.cc:304: // Should we succeed or
> fail here?
> On 2013/02/22 17:14:35, nasko wrote:
> > On 2013/02/22 02:45:17, creis wrote:
> > > Can we decide this, rather than leaving it as a TODO?
> > > 
> > > If there's no extension found for the request's URL, then it seems safe to
> > > return true here.
> > 
> > Why allow it if there is no existing extension for what is being requested?
> I'm
> > actually for a fail-safe behavior, so I've changed it to return false.
> 
> It likely means the extension has been unloaded, so it's fine to disallow the
> request. It will have failed anyway.

Actually, it allows loading of icons for unloaded extensions. My understanding
is that it is for the Extensions Settings page to display them even for disabled
ones.

I've looked into fixing that, but it is more elaborate than a few lines of code,
so I'll file a separate bug and fix it there.

Let me know if the latest is still good.

[Matt Perry, 2013-02-22 19:21:53]

On 2013/02/22 19:03:14, nasko wrote:
> On 2013/02/22 18:56:40, Matt Perry wrote:
> >
>
https://codereview.chromium.org/12218064/diff/17001/chrome/browser/extensions...
> > File chrome/browser/extensions/extension_protocols.cc (right):
> > 
> >
>
https://codereview.chromium.org/12218064/diff/17001/chrome/browser/extensions...
> > chrome/browser/extensions/extension_protocols.cc:304: // Should we succeed
or
> > fail here?
> > On 2013/02/22 17:14:35, nasko wrote:
> > > On 2013/02/22 02:45:17, creis wrote:
> > > > Can we decide this, rather than leaving it as a TODO?
> > > > 
> > > > If there's no extension found for the request's URL, then it seems safe
to
> > > > return true here.
> > > 
> > > Why allow it if there is no existing extension for what is being
requested?
> > I'm
> > > actually for a fail-safe behavior, so I've changed it to return false.
> > 
> > It likely means the extension has been unloaded, so it's fine to disallow
the
> > request. It will have failed anyway.
> 
> Actually, it allows loading of icons for unloaded extensions. My understanding
> is that it is for the Extensions Settings page to display them even for
disabled
> ones.
> 
> I've looked into fixing that, but it is more elaborate than a few lines of
code,
> so I'll file a separate bug and fix it there.
> 
> Let me know if the latest is still good.

Ah, good catch. The latest LGTM.

[commit-bot: I haz the power, 2013-02-22 19:30:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/12218064/11003

[commit-bot: I haz the power, 2013-02-23 01:20:26]

Change committed as 184245