diff options
author | Allan Sandfeld Jensen <allan.jensen@qt.io> | 2018-01-29 11:10:45 +0100 |
---|---|---|
committer | Allan Sandfeld Jensen <allan.jensen@qt.io> | 2018-02-01 12:04:10 +0000 |
commit | 176f276a431452598a0f87fdd49066bc4d5bc967 (patch) | |
tree | dd38f8198f371e6f173c54bd0cb97b381d857545 | |
parent | 66a21db86455dc416b39e46e7c8f14015802e521 (diff) | |
download | qtwebengine-chromium-176f276a431452598a0f87fdd49066bc4d5bc967.tar.gz |
[Backport] Fix for URL spoof caused by deletion of speculative RFH
This CL fixes a security issue where a website could succeed in spoofing the
URL of a cross-process navigation by issuing an endless loop of JavaScript
navigations. When the cross-site navigation was ready to commit, a
renderer-initiated navigation would start, causing the deletion of the
speculative RenderFrameHost. However, we would not update the visible URL for
the tab, even though the load of the cross-site navigation had stopped (due to
the deletion of the speculative RFH). This CL ensures that the pending
NavigationEntry is deleted in that case.
BUG=760342
Reviewed-on: https://chromium-review.googlesource.com/808924
Commit-Queue: Charlie Reis <creis@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#522231}(cherry picked from commit 372343377dfdc9736630ba80887bab27e047f4e6)
Reviewed-on: https://chromium-review.googlesource.com/876342
Reviewed-by: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/branch-heads/3282@{#547}
Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840}
(CVE-2018-6040) [1/3]
Change-Id: I1cc18d069095af30674b1e5b8fd2eb8676949b70
Reviewed-by: Michal Klocek <michal.klocek@qt.io>
-rw-r--r-- | chromium/content/browser/frame_host/render_frame_host_manager.cc | 19 | ||||
-rw-r--r-- | chromium/content/browser/frame_host/render_frame_host_manager_browsertest.cc | 98 |
2 files changed, 116 insertions, 1 deletions
diff --git a/chromium/content/browser/frame_host/render_frame_host_manager.cc b/chromium/content/browser/frame_host/render_frame_host_manager.cc index 64799aa81fa..d7d4f30421f 100644 --- a/chromium/content/browser/frame_host/render_frame_host_manager.cc +++ b/chromium/content/browser/frame_host/render_frame_host_manager.cc @@ -720,8 +720,17 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation( // not be recreated if the URL didn't change. So instead of calling // CleanUpNavigation just discard the speculative RenderFrameHost if one // exists. - if (speculative_render_frame_host_) + if (speculative_render_frame_host_) { + // If the speculative RenderFrameHost is trying to commit a navigation, + // inform the NavigationController that the load of the corresponding + // NavigationEntry stopped. + if (speculative_render_frame_host_->navigation_handle()) { + frame_tree_node_->navigator()->DiscardPendingEntryIfNeeded( + speculative_render_frame_host_->navigation_handle() + ->pending_nav_entry_id()); + } DiscardUnusedFrame(UnsetSpeculativeRenderFrameHost()); + } // Short-term solution: avoid creating a WebUI for subframes because // non-PlzNavigate code path doesn't do it and some WebUI pages don't @@ -844,6 +853,14 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation( void RenderFrameHostManager::CleanUpNavigation() { CHECK(IsBrowserSideNavigationEnabled()); if (speculative_render_frame_host_) { + // If the speculative RenderFrameHost is trying to commit a navigation, + // inform the NavigationController that the load of the corresponding + // NavigationEntry stopped. + if (speculative_render_frame_host_->navigation_handle()) { + frame_tree_node_->navigator()->DiscardPendingEntryIfNeeded( + speculative_render_frame_host_->navigation_handle() + ->pending_nav_entry_id()); + } bool was_loading = speculative_render_frame_host_->is_loading(); DiscardUnusedFrame(UnsetSpeculativeRenderFrameHost()); if (was_loading) diff --git a/chromium/content/browser/frame_host/render_frame_host_manager_browsertest.cc b/chromium/content/browser/frame_host/render_frame_host_manager_browsertest.cc index de2720501b1..1e09efe2312 100644 --- a/chromium/content/browser/frame_host/render_frame_host_manager_browsertest.cc +++ b/chromium/content/browser/frame_host/render_frame_host_manager_browsertest.cc @@ -46,6 +46,7 @@ #include "content/public/test/browser_test_utils.h" #include "content/public/test/content_browser_test.h" #include "content/public/test/content_browser_test_utils.h" +#include "content/public/test/controllable_http_response.h" #include "content/public/test/test_frame_navigation_observer.h" #include "content/public/test/test_navigation_observer.h" #include "content/public/test/test_utils.h" @@ -1497,6 +1498,103 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerSpoofingTest, EXPECT_FALSE(contents->GetController().GetVisibleEntry()); } +// Ensures that a pending navigation's URL is no longer visible after the +// speculative RFH is discarded due to a concurrent renderer-initiated +// navigation. See https://crbug.com/760342. +IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest, + ResetVisibleURLOnCrossProcessNavigationInterrupted) { + if (!IsBrowserSideNavigationEnabled()) + return; + const std::string kVictimPath = "/victim.html"; + const std::string kAttackPath = "/attack.html"; + ControllableHttpResponse victim_response(embedded_test_server(), kVictimPath); + ControllableHttpResponse attack_response(embedded_test_server(), kAttackPath); + EXPECT_TRUE(embedded_test_server()->Start()); + + const GURL kVictimURL = embedded_test_server()->GetURL("a.com", kVictimPath); + const GURL kAttackURL = embedded_test_server()->GetURL("b.com", kAttackPath); + + // First navigate to the attacker page. This page will be cross-site compared + // to the next navigations we will attempt. + const GURL kAttackInitialURL = + embedded_test_server()->GetURL("b.com", "/title1.html"); + NavigateToURL(shell(), kAttackInitialURL); + EXPECT_EQ(kAttackInitialURL, shell()->web_contents()->GetVisibleURL()); + + // Now, start a browser-initiated cross-site navigation to a new page that + // will hang at ready to commit stage. + TestNavigationManager victim_navigation(shell()->web_contents(), kVictimURL); + shell()->LoadURL(kVictimURL); + EXPECT_TRUE(victim_navigation.WaitForRequestStart()); + victim_navigation.ResumeNavigation(); + + victim_response.WaitForRequest(); + victim_response.Send( + "HTTP/1.1 200 OK\r\n" + "Content-Type: text/html; charset=utf-8\r\n" + "\r\n"); + EXPECT_TRUE(victim_navigation.WaitForResponse()); + victim_navigation.ResumeNavigation(); + + // The navigation is ready to commit: it has been handed to the speculative + // RenderFrameHost for commit. + RenderFrameHostImpl* speculative_rfh = + static_cast<WebContentsImpl*>(shell()->web_contents()) + ->GetFrameTree() + ->root() + ->render_manager() + ->speculative_frame_host(); + CHECK(speculative_rfh); + EXPECT_TRUE(speculative_rfh->is_loading()); + + // Since we have a browser-initiated pending navigation, the navigation URL is + // showing in the address bar. + EXPECT_EQ(kVictimURL, shell()->web_contents()->GetVisibleURL()); + + // The attacker page requests a navigation to a new document while the + // browser-initiated navigation hasn't committed yet. + TestNavigationManager attack_navigation(shell()->web_contents(), kAttackURL); + EXPECT_TRUE(ExecuteScriptWithoutUserGesture( + shell()->web_contents(), + "location.href = \"" + kAttackURL.spec() + "\";")); + EXPECT_TRUE(attack_navigation.WaitForRequestStart()); + + // This deletes the speculative RenderFrameHost that was supposed to commit + // the browser-initiated navigation. + speculative_rfh = static_cast<WebContentsImpl*>(shell()->web_contents()) + ->GetFrameTree() + ->root() + ->render_manager() + ->speculative_frame_host(); + EXPECT_FALSE(speculative_rfh); + + // The URL of the browser-initiated navigation should no longer be shown in + // the address bar since the RenderFrameHost that was supposed to commit it + // has been discarded. Instead, we should be showing the URL of the current + // page as we do not show the URL of pending navigations when they are + // renderer-initiated. + EXPECT_NE(kVictimURL, shell()->web_contents()->GetVisibleURL()); + EXPECT_EQ(kAttackInitialURL, shell()->web_contents()->GetVisibleURL()); + + // The attacker navigation results in a 204. + attack_navigation.ResumeNavigation(); + attack_response.WaitForRequest(); + attack_response.Send( + "HTTP/1.1 204 OK\r\n" + "Connection: close\r\n" + "\r\n"); + attack_navigation.WaitForNavigationFinished(); + speculative_rfh = static_cast<WebContentsImpl*>(shell()->web_contents()) + ->GetFrameTree() + ->root() + ->render_manager() + ->speculative_frame_host(); + EXPECT_FALSE(speculative_rfh); + + // We are still showing the URL of the current page. + EXPECT_EQ(kAttackInitialURL, shell()->web_contents()->GetVisibleURL()); +} + // Test for crbug.com/9682. We should not show the URL for a pending renderer- // initiated navigation in a new tab if it is not the initial navigation. In // this case, the renderer will not notify us of a modification, so we cannot |