Prevent unauthorized commits of the Chrome Web Store URL.

content/browser/renderer_host/render_view_host_impl.cc

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 322cf35665265843ff616da4ba6bff57e4352d09..b34ce2bafc43df01344167b9ccd6579157733474 100644
--- a/content/browser/renderer_host/render_view_host_impl.cc
+++ b/content/browser/renderer_host/render_view_host_impl.cc
@@ -1202,22 +1202,15 @@ void RenderViewHostImpl::OnNavigate(const IPC::Message& msg) {
   }
   RenderProcessHost* process = GetProcess();
 
-  // If the --site-per-process flag is passed, then the renderer process is
-  // not allowed to request web pages from other sites than the one it is
-  // dedicated to.
-  // Kill the renderer process if it violates this policy.
-  const CommandLine& command_line = *CommandLine::ForCurrentProcess();
-  if (command_line.HasSwitch(switches::kSitePerProcess) &&
-      static_cast<SiteInstanceImpl*>(GetSiteInstance())->HasSite() &&
-      validated_params.url != GURL(chrome::kAboutBlankURL)) {
-    if (!SiteInstance::IsSameWebSite(GetSiteInstance()->GetBrowserContext(),
-                                     GetSiteInstance()->GetSiteURL(),
-                                     validated_params.url) ||
-        static_cast<SiteInstanceImpl*>(GetSiteInstance())->
-            HasWrongProcessForURL(validated_params.url)) {
-      // TODO(nasko): Removed the actual kill process call until out-of-process
-      // iframes is ready to go.
-    }
+  // Attempts to commit certain off-limits URL should be caught more strictly
+  // than our FilterURL checks below.  If a renderer violates this policy, it
+  // should be killed.
+  if (!CanCommitURL(validated_params.url)) {
+    VLOG(1) << "Blocked URL " << validated_params.url.spec();
+    validated_params.url = GURL(chrome::kAboutBlankURL);
+    RecordAction(UserMetricsAction("CanCommitURL_BlockedAndKilled"));
+    // Kills the process.
+    process->ReceivedBadMessage();

[nasko, 2013/05/13 18:14:27]

Do we need to return here? Why proceed with the rest if we just killed the
renderer?

[Charlie Reis, 2013/05/13 20:10:51]

If we returned early here, we would leave the tab spinning and the address bar
would reflect the previous page, which may not even be in the same SiteInstance.


For example, suppose you're showing a.com and navigate to b.com in a different
process.  b.com's process is already hacked by another tab and tries to commit
the CWS.  We would kill the process for b.com but never stop the spinner, commit
the navigation, or show the sad tab, and the user would be left wondering what
happened while a.com is still visible.  Even if we called Stop instead of
committing, I think it makes sense to give some visual evidence that something
went wrong.

By leaving it this way, we'll commit an "about:blank" entry and show the sad
tab, giving the user some chance to report a bug or investigate.

   }
 
   ChildProcessSecurityPolicyImpl* policy =
@@ -1706,6 +1699,15 @@ void RenderViewHostImpl::ToggleSpeechInput() {
   Send(new InputTagSpeechMsg_ToggleSpeechInput(GetRoutingID()));
 }
 
+bool RenderViewHostImpl::CanCommitURL(const GURL& url) {
+  // TODO(creis): We should also check for WebUI pages here.  Also, when the
+  // out-of-process iframes implementation is ready, we should check for
+  // cross-site URLs that are not allowed to commit in this process.
+
+  // Give the client a chance to disallow URLs from committing.
+  return GetContentClient()->browser()->CanCommitURL(GetProcess(), url);
+}
+
 void RenderViewHostImpl::FilterURL(ChildProcessSecurityPolicyImpl* policy,
                                    const RenderProcessHost* process,
                                    bool empty_allowed,