Prevent unauthorized commits of the Chrome Web Store URL.

content/browser/renderer_host/render_view_host_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/renderer_host/render_view_host_impl.h"
 
 #include <set>
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 1184 matching lines @@
       main_frame_id_ = validated_params.frame_id;
     } else {
       // TODO(nasko): We plan to remove the usage of frame_id in navigation
       // and move to routing ids. This is in place to ensure that a
       // renderer is not misbehaving and sending us incorrect data.
       DCHECK_EQ(main_frame_id_, validated_params.frame_id);
     }
   }
   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 =
       ChildProcessSecurityPolicyImpl::GetInstance();
   // Without this check, an evil renderer can trick the browser into creating
   // a navigation entry for a banned URL.  If the user clicks the back button
   // followed by the forward button (or clicks reload, or round-trips through
   // session restore, etc), we'll think that the browser commanded the
   // renderer to load the URL and grant the renderer the privileges to request
   // the URL.  To prevent this attack, we block the renderer from inserting
@@ skipping 468 matching lines @@
 #endif
 
 void RenderViewHostImpl::SendOrientationChangeEvent(int orientation) {
   Send(new ViewMsg_OrientationChangeEvent(GetRoutingID(), orientation));
 }
 
 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,
                                    GURL* url) {
   if (empty_allowed && url->is_empty())
     return;
 
   // The browser process should never hear the swappedout:// URL from any
   // of the renderer's messages.  Check for this in debug builds, but don't
   // let it crash a release browser.
@@ skipping 360 matching lines @@
       webkit_glue::FilePathsFromHistoryState(state);
   for (std::vector<base::FilePath>::const_iterator file = file_paths.begin();
        file != file_paths.end(); ++file) {
     if (!policy->CanReadFile(GetProcess()->GetID(), *file))
       return false;
   }
   return true;
 }
 
 }  // namespace content