Prevent webview tags from navigating outside web-safe schemes.

Prevent webview tags from navigating outside web-safe schemes.

This CL removes protocol handlers and avoids granting capabilities or
bindings to webview processes, which prevents navigations to WebUI,
extension, and file URLs.  Web and data URLs are still permitted.

BUG=139397
TEST=Try to visit chrome://settings or other privileged URLs in a <webview>.


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

[Charlie Reis, 2012-10-27 00:00:08]

Tom, can you take a look at the approach?

Fady, can you take a look at the test?

[Fady Samuel, 2012-10-27 00:06:01]

Test: LGTM

[Charlie Reis, 2012-10-27 01:43:41]

Tom, I uploaded a new CL since the simpler FilterURL change blew up in unit
tests.  I'm passing in the RenderProcessHost now instead of the ID, though I was
hoping to avoid that.

(It might be possible to skip the FilterURL check, but I felt like it's worth
having it in there.)

[Charlie Reis, 2012-10-27 01:46:05]

Also adding Will Chan for profile_impl_io_data.cc.  (I was able to skip the
Interceptor stuff since skipping the coarse-grained ProtocolHandlers was what I
was looking for.)

[willchan no longer on Chromium, 2012-10-28 07:58:20]

LGTM

Btw, all the stuff where you switch passing the render process id to passing the
RenderProcessHost object? Make sure you have a proper review for that. I didn't
understand it, so hopefully someone else did. If not, you should make sure
someone reviews it who understands what you're doing :)

[Fady Samuel, 2012-10-29 00:48:09]

Please remove the TODO in  BrowserPluginEmbedder::NavigateGuest. Thanks!

[Charlie Reis, 2012-10-29 16:36:30]

On 2012/10/28 07:58:20, willchan wrote:
> Btw, all the stuff where you switch passing the render process id to passing
the
> RenderProcessHost object? Make sure you have a proper review for that. I
didn't
> understand it, so hopefully someone else did. If not, you should make sure
> someone reviews it who understands what you're doing :)

Yes, I'm waiting on Tom's review for that part.  Thanks!

On 2012/10/29 00:48:09, Fady Samuel wrote:
> Please remove the TODO in  BrowserPluginEmbedder::NavigateGuest. Thanks!

Good catch.  Done.

[Tom Sepez, 2012-10-29 17:59:08]

LGTM with nits.

https://codereview.chromium.org/11313018/diff/16001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/11313018/diff/16001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:490: bool is_guest_process =
(app_id.find("guest-") != std::string::npos);
Not your problem, but is there a better way of finding this out? "guest-"
anywhere in the string? At the least, wouldn't it have to start with "guest-".

https://codereview.chromium.org/11313018/diff/16001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/11313018/diff/16001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.cc:1245: FilterURL(policy,
GetProcess(), false, &validated_params.url);
nit: are we sure the optimizer can remove the redundant calls to GetProcess
(presumably a virtual)?  Also below.

https://codereview.chromium.org/11313018/diff/16001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.cc:1738: if
(!policy->CanRequestURL(process->GetID(), *url) || non_web_url_in_guest) {
nit: Might check non_web_url_in_guest first to avoid call to policy where
possible.

https://codereview.chromium.org/11313018/diff/16001/content/public/browser/re...
File content/public/browser/render_view_host.h (right):

https://codereview.chromium.org/11313018/diff/16001/content/public/browser/re...
content/public/browser/render_view_host.h:69: static void
FilterURL(RenderProcessHost* process,
nit: is this const?

[Charlie Reis, 2012-10-29 18:18:31]

Thanks, Tom!

joth@, can you give an OWNERs review to
intercept_navigation_resource_throttle.cc?  It's just a method param change.

https://codereview.chromium.org/11313018/diff/16001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/11313018/diff/16001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:490: bool is_guest_process =
(app_id.find("guest-") != std::string::npos);
On 2012/10/29 17:59:08, Tom Sepez wrote:
> Not your problem, but is there a better way of finding this out? "guest-"
> anywhere in the string? At the least, wouldn't it have to start with "guest-".

Funny you should mention it-- Albert and Nasko are just fixing that in
https://codereview.chromium.org/11308024/.  :)

https://codereview.chromium.org/11313018/diff/16001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/11313018/diff/16001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.cc:1245: FilterURL(policy,
GetProcess(), false, &validated_params.url);
On 2012/10/29 17:59:08, Tom Sepez wrote:
> nit: are we sure the optimizer can remove the redundant calls to GetProcess
> (presumably a virtual)?  Also below.

Good point.  Fixed.

https://codereview.chromium.org/11313018/diff/16001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.cc:1738: if
(!policy->CanRequestURL(process->GetID(), *url) || non_web_url_in_guest) {
On 2012/10/29 17:59:08, Tom Sepez wrote:
> nit: Might check non_web_url_in_guest first to avoid call to policy where
> possible.

Done.

https://codereview.chromium.org/11313018/diff/16001/content/public/browser/re...
File content/public/browser/render_view_host.h (right):

https://codereview.chromium.org/11313018/diff/16001/content/public/browser/re...
content/public/browser/render_view_host.h:69: static void
FilterURL(RenderProcessHost* process,
On 2012/10/29 17:59:08, Tom Sepez wrote:
> nit: is this const?

Yep.  Updated.

[joth, 2012-10-29 19:36:46]

lgtm

https://codereview.chromium.org/11313018/diff/24001/content/public/browser/re...
File content/public/browser/render_view_host.h (right):

https://codereview.chromium.org/11313018/diff/24001/content/public/browser/re...
content/public/browser/render_view_host.h:40: class RenderViewHostDelegate;
nit: may as well add RenderProcessHost here. I think you're currently getting it
for free from one of the includes.

[Charlie Reis, 2012-10-29 19:41:42]

Thanks!

https://codereview.chromium.org/11313018/diff/24001/content/public/browser/re...
File content/public/browser/render_view_host.h (right):

https://codereview.chromium.org/11313018/diff/24001/content/public/browser/re...
content/public/browser/render_view_host.h:40: class RenderViewHostDelegate;
On 2012/10/29 19:36:46, joth wrote:
> nit: may as well add RenderProcessHost here. I think you're currently getting
it
> for free from one of the includes.

Done.

[commit-bot: I haz the power, 2012-10-29 19:41:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/11313018/19012

[commit-bot: I haz the power, 2012-10-30 00:20:10]

Change committed as 164788