Merge CanCommitOrigin() and CanCommitURL().

This combines the origin and URL checks to remove some duplicate and
unnecessary checks. This will also make it easier to update the URL
checks to handle non-standard URLs in a follow-up CL.

- Inlines CanCommitURL() at the beginning and end of CanCommitOrigin()
  and removes redundant checks.
- Rename CanCommitOrigin() to CanCommitOriginAndUrl()
- Move kDisableWebSecurity check to the top of the function so it
  skips URL checks as well. This is the only functional change.

Bug: 991607
Change-Id: Id8b9f52b594994c1a4c1c38a709ba3e4c0daffc2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1742836
Commit-Queue: Aaron Colwell <acolwell@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#685408}

content/browser/frame_host/render_frame_host_impl.cc

@@ -396,7 +396,7 @@ void LogRendererKillCrashKeys(const GURL& site_url) {
 
 base::Optional<url::Origin> GetOriginForURLLoaderFactoryUnchecked(
     NavigationRequest* navigation_request) {
-  // Return a safe unique origin when there is no |navigation_request| (e.g.
+  // Return a safe opaque origin when there is no |navigation_request| (e.g.
   // when RFHI::CommitNavigation is called via RFHI::NavigateToInterstitialURL).
   if (!navigation_request)
     return url::Origin();
@@ -422,14 +422,14 @@ base::Optional<url::Origin> GetOriginForURLLoaderFactoryUnchecked(
     CHECK(navigation_request->browser_initiated());
 
     // loadDataWithBaseUrl submits a data: |common_params.url| (which has a
-    // unique origin), but commits that URL as if it came from
+    // opaque origin), but commits that URL as if it came from
     // |common_params.base_url_for_data_url|.  See also
     // https://crbug.com/976253.
     return url::Origin::Create(common_params.base_url_for_data_url);
   }
 
-  // MHTML frames should commit as unique origin (and should not be able to make
-  // network requests on behalf of the real origin).
+  // MHTML frames should commit as a opaque origin (and should not be able to
+  // make network requests on behalf of the real origin).
   //
   // TODO(lukasza): Cover MHTML main frames here.
   if (navigation_request->IsForMhtmlSubframe())
@@ -4403,37 +4403,88 @@ void RenderFrameHostImpl::ResetWaitingState() {
   network_service_connection_error_handler_holder_.reset();
 }
 
-bool RenderFrameHostImpl::CanCommitOrigin(
+RenderFrameHostImpl::CanCommitStatus RenderFrameHostImpl::CanCommitOriginAndUrl(
     const url::Origin& origin,
     const GURL& url) {
   // If the --disable-web-security flag is specified, all bets are off and the
   // renderer process can send any origin it wishes.
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kDisableWebSecurity)) {
-    return true;
+    return CanCommitStatus::CAN_COMMIT_ORIGIN_AND_URL;
   }
 
-  // It is safe to commit into a unique origin, regardless of the URL, as it is
+  // Renderer-debug URLs can never be committed.
+  if (IsRendererDebugURL(url))
+    return CanCommitStatus::CANNOT_COMMIT_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.
+
+  // MHTML subframes can supply URLs at commit time that do not match the
+  // process lock. For example, it can be either "cid:..." or arbitrary URL at
+  // which the frame was at the time of generating the MHTML
+  // (e.g. "http://localhost"). In such cases, don't verify the URL, but require
+  // the URL to commit in the process of the main frame.
+  if (!frame_tree_node()->IsMainFrame()) {
+    RenderFrameHostImpl* main_frame =
+        frame_tree_node()->frame_tree()->GetMainFrame();
+    if (main_frame->is_mhtml_document()) {
+      if (IsSameSiteInstance(main_frame))
+        return CanCommitStatus::CAN_COMMIT_ORIGIN_AND_URL;
+
+      // If an MHTML subframe commits in a different process (even one that
+      // appears correct for the subframe's URL), then we aren't correctly
+      // loading it from the archive and should kill the renderer.
+      base::debug::SetCrashKeyString(
+          base::debug::AllocateCrashKeyString(
+              "oopif_in_mhtml_page", base::debug::CrashKeySize::Size32),
+          is_mhtml_document() ? "is_mhtml_doc" : "not_mhtml_doc");
+      return CanCommitStatus::CANNOT_COMMIT_URL;
+    }
+  }
+
+  // Give the client a chance to disallow URLs from committing.
+  if (!GetContentClient()->browser()->CanCommitURL(GetProcess(), url))
+    return CanCommitStatus::CANNOT_COMMIT_URL;
+
+  // TODO(nasko): This check should be updated to apply to all URLs, not just
+  // standard ones.
+  auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
+  if (url.IsStandard() &&
+      !policy->CanAccessDataForOrigin(GetProcess()->GetID(), url)) {
+    return CanCommitStatus::CANNOT_COMMIT_URL;
+  }
+
+  // It is safe to commit into a opaque origin, regardless of the URL, as it is
   // restricted from accessing other origins.
   if (origin.opaque())
-    return true;
+    return CanCommitStatus::CAN_COMMIT_ORIGIN_AND_URL;
 
   // Standard URLs must match the reported origin.
   if (url.IsStandard() && !origin.IsSameOriginWith(url::Origin::Create(url)))
-    return false;
+    return CanCommitStatus::CANNOT_COMMIT_ORIGIN;
 
-  // A non-unique origin must be a valid URL, which allows us to safely do a
+  // A non-opaque origin must be a valid URL, which allows us to safely do a
   // conversion to GURL.
   GURL origin_url = origin.GetURL();
 
-  auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
   if (!policy->CanAccessDataForOrigin(GetProcess()->GetID(), origin))
-    return false;
+    return CanCommitStatus::CANNOT_COMMIT_ORIGIN;
 
   // Verify that the origin is allowed to commit in this process.
   // Note: This also handles non-standard cases for |url|, such as
   // about:blank, data, and blob URLs.
-  return CanCommitURL(origin_url);
+
+  // Renderer-debug URLs can never be committed.
+  if (IsRendererDebugURL(origin_url))
+    return CanCommitStatus::CANNOT_COMMIT_ORIGIN;
+
+  // Give the client a chance to disallow URLs from committing.
+  if (!GetContentClient()->browser()->CanCommitURL(GetProcess(), origin_url))
+    return CanCommitStatus::CANNOT_COMMIT_ORIGIN;
+
+  return CanCommitStatus::CAN_COMMIT_ORIGIN_AND_URL;
 }
 
 void RenderFrameHostImpl::NavigateToInterstitialURL(const GURL& data_url) {
@@ -5517,53 +5568,6 @@ void RenderFrameHostImpl::ClearFocusedElement() {
   Send(new FrameMsg_ClearFocusedElement(GetRoutingID()));
 }
 
-bool RenderFrameHostImpl::CanCommitURL(const GURL& url) {
-  // Renderer-debug URLs can never be committed.
-  if (IsRendererDebugURL(url))
-    return false;
-
-  // 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.
-
-  // MHTML subframes can supply URLs at commit time that do not match the
-  // process lock. For example, it can be either "cid:..." or arbitrary URL at
-  // which the frame was at the time of generating the MHTML
-  // (e.g. "http://localhost"). In such cases, don't verify the URL, but require
-  // the URL to commit in the process of the main frame.
-  if (!frame_tree_node()->IsMainFrame()) {
-    RenderFrameHostImpl* main_frame =
-        frame_tree_node()->frame_tree()->GetMainFrame();
-    if (main_frame->is_mhtml_document()) {
-      if (IsSameSiteInstance(main_frame))
-        return true;
-
-      // If an MHTML subframe commits in a different process (even one that
-      // appears correct for the subframe's URL), then we aren't correctly
-      // loading it from the archive and should kill the renderer.
-      base::debug::SetCrashKeyString(
-          base::debug::AllocateCrashKeyString(
-              "oopif_in_mhtml_page", base::debug::CrashKeySize::Size32),
-          is_mhtml_document() ? "is_mhtml_doc" : "not_mhtml_doc");
-      return false;
-    }
-  }
-
-  // Give the client a chance to disallow URLs from committing.
-  if (!GetContentClient()->browser()->CanCommitURL(GetProcess(), url))
-    return false;
-
-  // TODO(nasko): This check should be updated to apply to all URLs, not just
-  // standard ones.
-  auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
-  if (url.IsStandard() &&
-      !policy->CanAccessDataForOrigin(GetProcess()->GetID(), url)) {
-    return false;
-  }
-
-  return true;
-}
-
 void RenderFrameHostImpl::BlockRequestsForFrame() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
@@ -6469,8 +6473,8 @@ bool RenderFrameHostImpl::ValidateDidCommitParams(
   RenderProcessHost* process = GetProcess();
 
   // Error pages may sometimes commit a URL in the wrong process, which requires
-  // an exception for the CanCommitURL checks.  This is ok as long as the origin
-  // is unique.
+  // an exception for the CanCommitOriginAndUrl() checks.  This is ok as long
+  // as the origin is opaque.
   bool bypass_checks_for_error_page = false;
   if (SiteIsolationPolicy::IsErrorPageIsolationEnabled(
           frame_tree_node_->IsMainFrame())) {
@@ -6519,21 +6523,22 @@ bool RenderFrameHostImpl::ValidateDidCommitParams(
     // Attempts to commit certain off-limits URL should be caught more strictly
     // than our FilterURL checks.  If a renderer violates this policy, it
     // should be killed.
-    if (!CanCommitURL(validated_params->url)) {
+    switch (CanCommitOriginAndUrl(validated_params->origin,
+                                  validated_params->url)) {
+      case CanCommitStatus::CAN_COMMIT_ORIGIN_AND_URL:
+        // The origin and URL are safe to commit.
+        break;
+      case CanCommitStatus::CANNOT_COMMIT_URL:
         VLOG(1) << "Blocked URL " << validated_params->url.spec();
         LogCannotCommitUrlCrashKeys(validated_params->url,
                                     is_same_document_navigation,
                                     navigation_request);
 
         // Kills the process.
-      bad_message::ReceivedBadMessage(process,
-                                      bad_message::RFH_CAN_COMMIT_URL_BLOCKED);
+        bad_message::ReceivedBadMessage(
+            process, bad_message::RFH_CAN_COMMIT_URL_BLOCKED);
         return false;
-    }
-
-    // Verify that the origin passed from the renderer process is valid and can
-    // be allowed to commit in this RenderFrameHost.
-    if (!CanCommitOrigin(validated_params->origin, validated_params->url)) {
+      case CanCommitStatus::CANNOT_COMMIT_ORIGIN:
         DEBUG_ALIAS_FOR_ORIGIN(origin_debug_alias, validated_params->origin);
         LogCannotCommitOriginCrashKeys(is_same_document_navigation,
                                        navigation_request);

content/browser/frame_host/render_frame_host_impl.h

@@ -769,11 +769,6 @@ class CONTENT_EXPORT RenderFrameHostImpl
 
   void ClearFocusedElement();
 
-  // Returns whether the given URL is allowed to commit in the current process.
-  // This is a more conservative check than RenderProcessHost::FilterURL, since
-  // it will be used to kill processes that commit unauthorized URLs.
-  bool CanCommitURL(const GURL& url);
-
   // Returns the PreviewsState of the last successful navigation
   // that made a network request. The PreviewsState is a bitmask of potentially
   // several Previews optimizations.
@@ -1339,12 +1334,18 @@ class CONTENT_EXPORT RenderFrameHostImpl
   // relevant.
   void ResetWaitingState();
 
-  // Returns whether the given origin is allowed to commit in the current
-  // RenderFrameHost. The |url| is used to ensure it matches the origin in cases
-  // where it is applicable. This is a more conservative check than
+  // Returns whether the given origin and URL is allowed to commit in the
+  // current RenderFrameHost. The |url| is used to ensure it matches the origin
+  // in cases where it is applicable. This is a more conservative check than
   // RenderProcessHost::FilterURL, since it will be used to kill processes that
   // commit unauthorized origins.
-  bool CanCommitOrigin(const url::Origin& origin, const GURL& url);
+  enum class CanCommitStatus {
+    CAN_COMMIT_ORIGIN_AND_URL,
+    CANNOT_COMMIT_ORIGIN,
+    CANNOT_COMMIT_URL
+  };
+  CanCommitStatus CanCommitOriginAndUrl(const url::Origin& origin,
+                                        const GURL& url);
 
   // Asserts that the given RenderFrameHostImpl is part of the same browser
   // context (and crashes if not), then returns whether the given frame is