Prevent webview tags from navigating outside web-safe schemes. (issue 11313018)

[cr...@chromium.org]

Reviewers: Tom Sepez, Fady Samuel,

Message:
Tom, can you take a look at the approach?

Fady, can you take a look at the test?

Description:
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>.


Please review this at https://codereview.chromium.org/11313018/

SVN Base: svn://svn.chromium.org/chrome/trunk/src

Affected files:
M chrome/browser/profiles/profile_impl_io_data.cc
M content/browser/browser_plugin/browser_plugin_host_browsertest.cc
M content/browser/renderer_host/render_view_host_impl.cc


Index: chrome/browser/profiles/profile_impl_io_data.cc
diff --git a/chrome/browser/profiles/profile_impl_io_data.cc
b/chrome/browser/profiles/profile_impl_io_data.cc
index
3920aecdebd54c683b05eb1184b83dcb7d11b040..e877b24eb1d228b7c4ce503b7769e96b34645f3d
100644
--- a/chrome/browser/profiles/profile_impl_io_data.cc
+++ b/chrome/browser/profiles/profile_impl_io_data.cc
@@ -556,12 +556,16 @@ ProfileImplIOData::InitializeAppRequestContext(

// Overwrite the job factory that we inherit from the main context so
// that we can later provide our own handles for storage related
protocols.
+ // Install all the usual protocol handlers unless we are in a browser
plugin
+ // guest process, in which case only web-safe schemes are allowed.
scoped_ptr<net::URLRequestJobFactoryImpl> job_factory(
new net::URLRequestJobFactoryImpl());
- SetUpJobFactory(job_factory.get(), protocol_handler_interceptor.Pass(),
- network_delegate(),
- context->ftp_transaction_factory(),
- context->ftp_auth_cache());
+ if (!is_guest_process) {
+ SetUpJobFactory(job_factory.get(), protocol_handler_interceptor.Pass(),
+ network_delegate(),
+ context->ftp_transaction_factory(),
+ context->ftp_auth_cache());
+ }
context->SetJobFactory(job_factory.PassAs<net::URLRequestJobFactory>());

return context;
Index: content/browser/browser_plugin/browser_plugin_host_browsertest.cc
diff --git
a/content/browser/browser_plugin/browser_plugin_host_browsertest.cc
b/content/browser/browser_plugin/browser_plugin_host_browsertest.cc
index
955efb907c21b124b7791380b4a811c5bcb47968..44b8942bdc35879c33e96f27dfc026aaf8c0818b
100644
--- a/content/browser/browser_plugin/browser_plugin_host_browsertest.cc
+++ b/content/browser/browser_plugin/browser_plugin_host_browsertest.cc
@@ -743,19 +743,47 @@ IN_PROC_BROWSER_TEST_F(BrowserPluginHostTest,
LoadAbort) {
const char kEmbedderURL[] = "files/browser_plugin_embedder.html";
StartBrowserPluginTest(kEmbedderURL, "about:blank", true, "");

- const string16 expected_title = ASCIIToUTF16("ERR_EMPTY_RESPONSE");
- content::TitleWatcher title_watcher(test_embedder()->web_contents(),
- expected_title);
+ {
+ // Navigate the guest to "close-socket".
+ const string16 expected_title = ASCIIToUTF16("ERR_EMPTY_RESPONSE");
+ content::TitleWatcher title_watcher(test_embedder()->web_contents(),
+ expected_title);
+ RenderViewHostImpl* rvh = static_cast<RenderViewHostImpl*>(
+ test_embedder()->web_contents()->GetRenderViewHost());
+ GURL test_url = test_server()->GetURL("close-socket");
+ ExecuteSyncJSFunction(rvh, ASCIIToUTF16(
+ StringPrintf("SetSrc('%s');", test_url.spec().c_str())));
+ string16 actual_title = title_watcher.WaitAndGetTitle();
+ EXPECT_EQ(expected_title, actual_title);
+ }

- // Renavigate the guest to "close-socket".
- RenderViewHostImpl* rvh = static_cast<RenderViewHostImpl*>(
- test_embedder()->web_contents()->GetRenderViewHost());
- GURL test_url = test_server()->GetURL("close-socket");
- ExecuteSyncJSFunction(rvh, ASCIIToUTF16(
- StringPrintf("SetSrc('%s');", test_url.spec().c_str())));
+ {
+ // Navigate the guest to an illegal chrome:// URL.
+ const string16 expected_title = ASCIIToUTF16("ERR_FAILED");
+ content::TitleWatcher title_watcher(test_embedder()->web_contents(),
+ expected_title);
+ RenderViewHostImpl* rvh = static_cast<RenderViewHostImpl*>(
+ test_embedder()->web_contents()->GetRenderViewHost());
+ GURL test_url("chrome://newtab");
+ ExecuteSyncJSFunction(rvh, ASCIIToUTF16(
+ StringPrintf("SetSrc('%s');", test_url.spec().c_str())));
+ string16 actual_title = title_watcher.WaitAndGetTitle();
+ EXPECT_EQ(expected_title, actual_title);
+ }

- string16 actual_title = title_watcher.WaitAndGetTitle();
- EXPECT_EQ(expected_title, actual_title);
+ {
+ // Navigate the guest to an illegal file:// URL.
+ const string16 expected_title = ASCIIToUTF16("ERR_ABORTED");
+ content::TitleWatcher title_watcher(test_embedder()->web_contents(),
+ expected_title);
+ RenderViewHostImpl* rvh = static_cast<RenderViewHostImpl*>(
+ test_embedder()->web_contents()->GetRenderViewHost());
+ GURL test_url("file://foo");
+ ExecuteSyncJSFunction(rvh, ASCIIToUTF16(
+ StringPrintf("SetSrc('%s');", test_url.spec().c_str())));
+ string16 actual_title = title_watcher.WaitAndGetTitle();
+ EXPECT_EQ(expected_title, actual_title);
+ }
}

IN_PROC_BROWSER_TEST_F(BrowserPluginHostTest, LoadRedirect) {
Index: content/browser/renderer_host/render_view_host_impl.cc
diff --git a/content/browser/renderer_host/render_view_host_impl.cc
b/content/browser/renderer_host/render_view_host_impl.cc
index
e6cdcc9631e1560aee662b8e7fc96c97bbb26283..64be32434b24ef744d1c109273544c3a9e6b9824
100644
--- a/content/browser/renderer_host/render_view_host_impl.cc
+++ b/content/browser/renderer_host/render_view_host_impl.cc
@@ -268,6 +268,8 @@ bool RenderViewHostImpl::CreateRenderView(

// If it's enabled, tell the renderer to set up the Javascript bindings
for
// sending messages back to the browser.
+ if (GetProcess()->IsGuest())
+ DCHECK_EQ(0, enabled_bindings_);
Send(new ViewMsg_AllowBindings(GetRoutingID(), enabled_bindings_));
// Let our delegate know that we created a RenderView.
delegate_->RenderViewCreated(this);
@@ -289,14 +291,18 @@ void RenderViewHostImpl::SyncRendererPrefs() {
}

void RenderViewHostImpl::Navigate(const ViewMsg_Navigate_Params& params) {
- ChildProcessSecurityPolicyImpl::GetInstance()->GrantRequestURL(
- GetProcess()->GetID(), params.url);
- if (params.url.SchemeIs(chrome::kDataScheme) &&
- params.base_url_for_data_url.SchemeIs(chrome::kFileScheme)) {
- // If 'data:' is used, and we have a 'file:' base url, grant access to
- // local files.
+ // Browser plugin guests are not allowed to navigate outside web-safe
schemes,
+ // so do not grant them the ability to request additional URLs.
+ if (!GetProcess()->IsGuest()) {
ChildProcessSecurityPolicyImpl::GetInstance()->GrantRequestURL(
- GetProcess()->GetID(), params.base_url_for_data_url);
+ GetProcess()->GetID(), params.url);
+ if (params.url.SchemeIs(chrome::kDataScheme) &&
+ params.base_url_for_data_url.SchemeIs(chrome::kFileScheme)) {
+ // If 'data:' is used, and we have a 'file:' base url, grant access
to
+ // local files.
+ ChildProcessSecurityPolicyImpl::GetInstance()->GrantRequestURL(
+ GetProcess()->GetID(), params.base_url_for_data_url);
+ }
}

ViewMsg_Navigate* nav_message = new ViewMsg_Navigate(GetRoutingID(),
params);
@@ -817,6 +823,12 @@ void RenderViewHostImpl::AllowBindings(int
bindings_flags) {
return;
}

+ // Never grant any bindings to browser plugin guests.
+ if (GetProcess()->IsGuest()) {
+ NOTREACHED() << "Never grant bindings to a guest process.";
+ return;
+ }
+
if (bindings_flags & BINDINGS_POLICY_WEB_UI) {
ChildProcessSecurityPolicyImpl::GetInstance()->GrantWebUIBindings(
GetProcess()->GetID());
@@ -1720,7 +1732,13 @@ void
RenderViewHostImpl::FilterURL(ChildProcessSecurityPolicyImpl* policy,
*url = GURL(chrome::kAboutBlankURL);
}

- if (!policy->CanRequestURL(renderer_id, *url)) {
+ // Do not allow browser plugin guests to navigate to non-web URLs, since
they
+ // cannot swap processes or grant bindings.
+ bool non_web_url_in_guest =
+ RenderProcessHost::FromID(renderer_id)->IsGuest() &&
+ !(url->is_valid() && policy->IsWebSafeScheme(url->scheme()));
+
+ if (!policy->CanRequestURL(renderer_id, *url) || non_web_url_in_guest) {
// If this renderer is not permitted to request this URL, we
invalidate the
// URL. This prevents us from storing the blocked URL and becoming
confused
// later.

[fsa...@chromium.org]

Test: LGTM

https://codereview.chromium.org/11313018/

[cr...@chromium.org]

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.)

https://codereview.chromium.org/11313018/

[cr...@chromium.org]

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.)

https://codereview.chromium.org/11313018/

[will...@chromium.org]

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 :)

https://codereview.chromium.org/11313018/