[Backport] CVE-2019-13691

Security: Fixing URL spoof via cross-site iframe top navigation.

Currently, it is possible to spoof the URL of a site by starting a
navigation in the main frame and having a cross-site iframe initiating
another navigation in its parent frame (with extra steps for it to be
user visible).

This can also be used to cancel any browser initiated navigation in the
main frame.

The CL adresses the issue by adding checks similar to what exists in
BeginNavigation on the FrameProxy::OpenURL path.

Bug: 966914
Change-Id: If547c2ef4b30b2e0323141005583412c12bfef8e
Reviewed-by: Camille Lamy <clamy@chromium.org>
Commit-Queue: Arthur Hemery <ahemery@chromium.org>
Cr-Commit-Position: refs/heads/master@{#668435}
Reviewed-by: Michal Klocek <michal.klocek@qt.io>

5 files changed, 105 insertions, 19 deletions

diff --git a/chromium/content/browser/frame_host/navigation_controller_impl_browsertest.cc b/chromium/content/browser/frame_host/navigation_controller_impl_browsertest.cc
index 08aa655fc42..74675948c45 100644
--- a/chromium/content/browser/frame_host/navigation_controller_impl_browsertest.cc
+++ b/chromium/content/browser/frame_host/navigation_controller_impl_browsertest.cc
@@ -8834,4 +8834,65 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerHistoryInterventionBrowserTest,
   EXPECT_EQ(1, controller.GetLastCommittedEntryIndex());
 }
 
+// When running OpenURL to an invalid URL on a frame proxy it should not spoof
+// the url by canceling a main frame navigation.
+// See https://crbug.com/966914.
+IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
+                       CrossProcessIframeToInvalidURLCancelsRedirectSpoof) {
+  const GURL main_frame_url(embedded_test_server()->GetURL(
+      "a.com",
+      "/cross_site_iframe_factory.html?a(b{sandbox-allow-scripts,sandbox-allow-"
+      "top-navigation}())"));
+  const GURL main_frame_url_2(embedded_test_server()->GetURL("/title2.html"));
+
+  // Load the initial page, containing a fully scriptable cross-site iframe.
+  EXPECT_TRUE(NavigateToURL(shell(), main_frame_url));
+
+  // Setup the message trigger on the main frame and the handler on the iframe.
+  FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
+                            ->GetFrameTree()
+                            ->root();
+  std::string script =
+      "var messageUnloadEventSent = false;"
+      "window.addEventListener(\"beforeunload\", function "
+      "onBeforeUnload(event) {"
+      "if(messageUnloadEventSent) {"
+      "return;"
+      "}"
+      "messageUnloadEventSent = true;"
+      "var iframe = document.getElementById(\"child-0\");"
+      "iframe.contentWindow.postMessage(\"\", \"*\");"
+      "});";
+  EXPECT_TRUE(ExecJs(root, script));
+
+  script =
+      "window.addEventListener(\"message\", function (event) {"
+      "parent.location.href = \"chrome-guest://1234\";"
+      "});";
+  EXPECT_TRUE(ExecJs(root->child_at(0), script));
+
+  // This navigation will be raced by a navigation started in the iframe.
+  // It should still be prevalent compared to a non user-initiated render frame
+  // navigation.
+  // TODO(ahemery): Remove first block when https://crbug.com/973415 is fixed.
+  // The navigation started in the iframe should be canceled. When we are not
+  // cross process, this does not happen because has_user_gesture is true. The
+  // navigation is automatic and it should not be the case. This causes no spoof
+  // however, as verified below.
+  GURL check_url;
+  if (!SiteIsolationPolicy::UseDedicatedProcessesForAllSites()) {
+    EXPECT_FALSE(NavigateToURL(shell(), main_frame_url_2));
+    check_url = main_frame_url;
+  } else {
+    EXPECT_TRUE(NavigateToURL(shell(), main_frame_url_2));
+    check_url = main_frame_url_2;
+  }
+
+  // Check that no spoof happened.
+  NavigationControllerImpl& controller = static_cast<NavigationControllerImpl&>(
+      shell()->web_contents()->GetController());
+  EXPECT_EQ(check_url, shell()->web_contents()->GetLastCommittedURL());
+  EXPECT_EQ(check_url, controller.GetVisibleEntry()->GetVirtualURL());
+}
+
 }  // namespace content
diff --git a/chromium/content/browser/frame_host/navigator.h b/chromium/content/browser/frame_host/navigator.h
index dae4b8d56ae..ba1058a22d9 100644
--- a/chromium/content/browser/frame_host/navigator.h
+++ b/chromium/content/browser/frame_host/navigator.h
@@ -131,7 +131,8 @@ class CONTENT_EXPORT Navigator : public base::RefCounted<Navigator> {
       const std::string& method,
       scoped_refptr<network::ResourceRequestBody> post_body,
       const std::string& extra_headers,
-      scoped_refptr<network::SharedURLLoaderFactory> blob_url_loader_factory) {}
+      scoped_refptr<network::SharedURLLoaderFactory> blob_url_loader_factory,
+      bool has_user_gesture) {}
 
   // Called after receiving a BeforeUnloadACK IPC from the renderer. If
   // |frame_tree_node| has a NavigationRequest waiting for the renderer
diff --git a/chromium/content/browser/frame_host/navigator_impl.cc b/chromium/content/browser/frame_host/navigator_impl.cc
index 0937708e149..15ece0db07d 100644
--- a/chromium/content/browser/frame_host/navigator_impl.cc
+++ b/chromium/content/browser/frame_host/navigator_impl.cc
@@ -51,6 +51,31 @@
 
 namespace content {
 
+namespace {
+
+// A renderer-initiated navigation should be ignored iff a) there is an ongoing
+// request b) which is browser initiated and c) the renderer request is not
+// user-initiated.
+bool ShouldIgnoreIncomingRendererRequest(
+    NavigationRequest* ongoing_navigation_request,
+    bool has_user_gesture) {
+  return ongoing_navigation_request &&
+         ongoing_navigation_request->browser_initiated() && !has_user_gesture;
+}
+
+// Informs the RenderFrameImpl associated with the |frame_tree_node| that a
+// navigation it started was dropped.
+void DropNavigation(FrameTreeNode* frame_tree_node) {
+  if (!IsPerNavigationMojoInterfaceEnabled()) {
+    RenderFrameHost* current_frame_host =
+        frame_tree_node->render_manager()->current_frame_host();
+    current_frame_host->Send(
+        new FrameMsg_DroppedNavigation(current_frame_host->GetRoutingID()));
+  }
+}
+
+}  // namespace
+
 struct NavigatorImpl::NavigationMetricsData {
   NavigationMetricsData(base::TimeTicks start_time,
                         GURL url,
@@ -478,7 +503,8 @@ void NavigatorImpl::NavigateFromFrameProxy(
     const std::string& method,
     scoped_refptr<network::ResourceRequestBody> post_body,
     const std::string& extra_headers,
-    scoped_refptr<network::SharedURLLoaderFactory> blob_url_loader_factory) {
+    scoped_refptr<network::SharedURLLoaderFactory> blob_url_loader_factory,
+    bool has_user_gesture) {
   // |method != "POST"| should imply absence of |post_body|.
   if (method != "POST" && post_body) {
     NOTREACHED();
@@ -519,6 +545,13 @@ void NavigatorImpl::NavigateFromFrameProxy(
     is_renderer_initiated = false;
   }
 
+  if (is_renderer_initiated &&
+      ShouldIgnoreIncomingRendererRequest(
+          render_frame_host->frame_tree_node()->navigation_request(),
+          has_user_gesture)) {
+    return;
+  }
+
   GetContentClient()->browser()->OverrideNavigationParams(
       current_site_instance, &page_transition, &is_renderer_initiated,
       &referrer_to_use);
@@ -602,18 +635,10 @@ void NavigatorImpl::OnBeginNavigation(
     frame_tree_node->ResetNavigationRequest(false, true);
   }
 
-  // The renderer-initiated navigation request is ignored iff a) there is an
-  // ongoing request b) which is browser initiated and c) the renderer request
-  // is not user-initiated.
-  if (ongoing_navigation_request &&
-      ongoing_navigation_request->browser_initiated() &&
-      !common_params.has_user_gesture) {
-    if (!IsPerNavigationMojoInterfaceEnabled()) {
-      RenderFrameHost* current_frame_host =
-          frame_tree_node->render_manager()->current_frame_host();
-      current_frame_host->Send(
-          new FrameMsg_DroppedNavigation(current_frame_host->GetRoutingID()));
-    }
+  // Verify this navigation has precedence.
+  if (ShouldIgnoreIncomingRendererRequest(ongoing_navigation_request,
+                                          common_params.has_user_gesture)) {
+    DropNavigation(frame_tree_node);
     return;
   }
 
diff --git a/chromium/content/browser/frame_host/navigator_impl.h b/chromium/content/browser/frame_host/navigator_impl.h
index 6e6daea9d95..365827c8928 100644
--- a/chromium/content/browser/frame_host/navigator_impl.h
+++ b/chromium/content/browser/frame_host/navigator_impl.h
@@ -84,8 +84,8 @@ class CONTENT_EXPORT NavigatorImpl : public Navigator {
       const std::string& method,
       scoped_refptr<network::ResourceRequestBody> post_body,
       const std::string& extra_headers,
-      scoped_refptr<network::SharedURLLoaderFactory> blob_url_loader_factory)
-      override;
+      scoped_refptr<network::SharedURLLoaderFactory> blob_url_loader_factory,
+      bool has_user_gesture) override;
   void OnBeforeUnloadACK(FrameTreeNode* frame_tree_node,
                          bool proceed,
                          const base::TimeTicks& proceed_time) override;
diff --git a/chromium/content/browser/frame_host/render_frame_proxy_host.cc b/chromium/content/browser/frame_host/render_frame_proxy_host.cc
index a5e18f465f7..302f952ef0a 100644
--- a/chromium/content/browser/frame_host/render_frame_proxy_host.cc
+++ b/chromium/content/browser/frame_host/render_frame_proxy_host.cc
@@ -327,8 +327,6 @@ void RenderFrameProxyHost::OnOpenURL(
   // in the current tab.
   DCHECK_EQ(WindowOpenDisposition::CURRENT_TAB, params.disposition);
 
-  // TODO(alexmos, creis): Figure out whether |params.user_gesture| needs to be
-  // passed in as well.
   // TODO(lfg, lukasza): Remove |extra_headers| parameter from
   // RequestTransferURL method once both RenderFrameProxyHost and
   // RenderFrameHostImpl call RequestOpenURL from their OnOpenURL handlers.
@@ -340,7 +338,8 @@ void RenderFrameProxyHost::OnOpenURL(
       params.referrer, ui::PAGE_TRANSITION_LINK,
       params.should_replace_current_entry, params.download_policy,
       params.uses_post ? "POST" : "GET", params.resource_request_body,
-      params.extra_headers, std::move(blob_url_loader_factory));
+      params.extra_headers, std::move(blob_url_loader_factory),
+      params.user_gesture);
 }
 
 void RenderFrameProxyHost::OnCheckCompleted() {