[Backport] CVE-2019-13746 1/2

Fix wrong NavigationRequest / pending NavigationEntry association.

When a new renderer initiated navigations starts it will either:
 1) Create a new pending navigation entry and use it.
 2) Reuse the existing pending navigation entry.

The problem is that (2) is very wrong. There is no reasons to do it.

For instance, when a main frame navigation starts, it creates a pending
NavigationEntry. Then if a subframe navigation starts, is reuses the
same pending NavigationEntry. This doesn't make sense.

Bug: 999932
Change-Id: If0da4fb7824f71ab1a488a616b8eb93c61482e30
Reviewed-by: Michael Brüning <michael.bruning@qt.io>

3 files changed, 62 insertions, 49 deletions

diff --git a/chromium/content/browser/frame_host/navigation_controller_impl_unittest.cc b/chromium/content/browser/frame_host/navigation_controller_impl_unittest.cc
index 190e5f060ba..c46e47d7dbf 100644
--- a/chromium/content/browser/frame_host/navigation_controller_impl_unittest.cc
+++ b/chromium/content/browser/frame_host/navigation_controller_impl_unittest.cc
@@ -3511,11 +3511,35 @@ TEST_F(NavigationControllerTest, SubframeWhilePending) {
   TestRenderFrameHost* subframe = static_cast<TestRenderFrameHost*>(
       contents()->GetFrameTree()->root()->child_at(0)->current_frame_host());
   const GURL url1_sub("http://foo/subframe");
-  NavigationSimulator::NavigateAndCommitFromDocument(url1_sub, subframe);
+
+  auto subframe_navigation =
+      NavigationSimulator::CreateRendererInitiated(url1_sub, subframe);
+  subframe_navigation->Start();
+
+  // Creating the subframe navigation should have no effect on the pending
+  // navigation entry and on the visible URL.
+  EXPECT_EQ(url1, controller.GetLastCommittedEntry()->GetURL());
+  EXPECT_EQ(url2, controller.GetVisibleEntry()->GetURL());
+
+  auto nav_entry_id = [](NavigationSimulator* simulator) {
+    return NavigationRequest::From(simulator->GetNavigationHandle())
+        ->nav_entry_id();
+  };
+
+  // The main frame navigation is still associated with the pending entry, the
+  // subframe one isn't.
+  EXPECT_EQ(controller.GetPendingEntry()->GetUniqueID(),
+            nav_entry_id(main_frame_navigation.get()));
+  EXPECT_EQ(0, nav_entry_id(subframe_navigation.get()));
+
+  subframe_navigation->Commit();
 
   // The subframe navigation should have no effect on the displayed url.
   EXPECT_EQ(url1, controller.GetLastCommittedEntry()->GetURL());
   EXPECT_EQ(url2, controller.GetVisibleEntry()->GetURL());
+
+  EXPECT_EQ(controller.GetPendingEntry()->GetUniqueID(),
+            nav_entry_id(main_frame_navigation.get()));
 }
 
 // Test CopyStateFrom with 2 urls, the first selected and nothing in the target.
diff --git a/chromium/content/browser/frame_host/navigator_impl.cc b/chromium/content/browser/frame_host/navigator_impl.cc
index e8043a64b3a..80901a82ea3 100644
--- a/chromium/content/browser/frame_host/navigator_impl.cc
+++ b/chromium/content/browser/frame_host/navigator_impl.cc
@@ -659,22 +659,9 @@ void NavigatorImpl::OnBeginNavigation(
     return;
   }
 
-  // In all other cases the current navigation, if any, is canceled and a new
-  // NavigationRequest is created for the node.
-  if (frame_tree_node->IsMainFrame()) {
-    // Renderer-initiated main-frame navigations that need to swap processes
-    // will go to the browser via a OpenURL call, and then be handled by the
-    // same code path as browser-initiated navigations. For renderer-initiated
-    // main frame navigation that start via a BeginNavigation IPC, the
-    // RenderFrameHost will not be swapped. Therefore it is safe to call
-    // DidStartMainFrameNavigation with the SiteInstance from the current
-    // RenderFrameHost.
-    DidStartMainFrameNavigation(
-        common_params, frame_tree_node->current_frame_host()->GetSiteInstance(),
-        nullptr);
-    navigation_data_.reset();
-  }
-  NavigationEntryImpl* pending_entry = controller_->GetPendingEntry();
+  NavigationEntryImpl* navigation_entry =
+      GetNavigationEntryForRendererInitiatedNavigation(common_params,
+                                                       frame_tree_node);
   NavigationEntryImpl* current_entry = controller_->GetLastCommittedEntry();
   // Only consult the delegate for override state if there is no current entry,
   // since that state should only apply to newly created tabs (and not cases
@@ -685,7 +672,7 @@ void NavigatorImpl::OnBeginNavigation(
           : delegate_ && delegate_->ShouldOverrideUserAgentInNewTabs();
   frame_tree_node->CreatedNavigationRequest(
       NavigationRequest::CreateRendererInitiated(
-          frame_tree_node, pending_entry, common_params,
+          frame_tree_node, navigation_entry, common_params,
           std::move(begin_params), controller_->GetLastCommittedEntryIndex(),
           controller_->GetEntryCount(), override_user_agent,
           std::move(blob_url_loader_factory), std::move(navigation_client),
@@ -874,10 +861,14 @@ void NavigatorImpl::RecordNavigationMetrics(
   navigation_data_.reset();
 }
 
-void NavigatorImpl::DidStartMainFrameNavigation(
+NavigationEntryImpl*
+NavigatorImpl::GetNavigationEntryForRendererInitiatedNavigation(
     const CommonNavigationParams& common_params,
-    SiteInstanceImpl* site_instance,
-    NavigationHandleImpl* navigation_handle) {
+    FrameTreeNode* frame_tree_node) {
+
+   if (!frame_tree_node->IsMainFrame())
+   return nullptr;
+
   // If there is no browser-initiated pending entry for this navigation and it
   // is not for the error URL, create a pending entry and ensure the address bar
   // updates accordingly.  We don't know the referrer or extra headers at this
@@ -887,6 +878,8 @@ void NavigatorImpl::DidStartMainFrameNavigation(
   NavigationEntryImpl* pending_entry = controller_->GetPendingEntry();
   bool has_browser_initiated_pending_entry =
       pending_entry && !pending_entry->is_renderer_initiated();
+  if (has_browser_initiated_pending_entry)
+    return nullptr;
 
   // A pending navigation entry is created in OnBeginNavigation(). The renderer
   // sends a provisional load notification after that. We don't want to create
@@ -894,33 +887,29 @@ void NavigatorImpl::DidStartMainFrameNavigation(
   bool renderer_provisional_load_to_pending_url =
       pending_entry && pending_entry->is_renderer_initiated() &&
       (pending_entry->GetURL() == common_params.url);
+  if (renderer_provisional_load_to_pending_url)
+    return nullptr;
 
   // If there is a transient entry, creating a new pending entry will result
   // in deleting it, which leads to inconsistent state.
   bool has_transient_entry = !!controller_->GetTransientEntry();
+  if (has_transient_entry)
+    return nullptr;
+
+  std::unique_ptr<NavigationEntryImpl> entry =
+      NavigationEntryImpl::FromNavigationEntry(
+          NavigationController::CreateNavigationEntry(
+              common_params.url, content::Referrer(),
+              common_params.initiator_origin, ui::PAGE_TRANSITION_LINK,
+              true /* is_renderer_initiated */, std::string(),
+              controller_->GetBrowserContext(),
+              nullptr /* blob_url_loader_factory */));
+
+  controller_->SetPendingEntry(std::move(entry));
+  if (delegate_)
+    delegate_->NotifyChangedNavigationState(content::INVALIDATE_TYPE_URL);
 
-  if (!has_browser_initiated_pending_entry && !has_transient_entry &&
-      !renderer_provisional_load_to_pending_url) {
-    std::unique_ptr<NavigationEntryImpl> entry =
-        NavigationEntryImpl::FromNavigationEntry(
-            NavigationController::CreateNavigationEntry(
-                common_params.url, content::Referrer(),
-                common_params.initiator_origin, ui::PAGE_TRANSITION_LINK,
-                true /* is_renderer_initiated */, std::string(),
-                controller_->GetBrowserContext(),
-                nullptr /* blob_url_loader_factory */));
-    // TODO(creis): If there's a pending entry already, find a safe way to
-    // update it instead of replacing it and copying over things like this.
-    // That will allow us to skip the NavigationHandle update below as well.
-    if (pending_entry) {
-      entry->set_should_replace_entry(pending_entry->should_replace_entry());
-      entry->SetRedirectChain(pending_entry->GetRedirectChain());
-    }
-
-    controller_->SetPendingEntry(std::move(entry));
-    if (delegate_)
-      delegate_->NotifyChangedNavigationState(content::INVALIDATE_TYPE_URL);
-  }
+  return controller_->GetPendingEntry();
 }
 
 }  // namespace content
diff --git a/chromium/content/browser/frame_host/navigator_impl.h b/chromium/content/browser/frame_host/navigator_impl.h
index 5a7d89149d2..0f635c7a00b 100644
--- a/chromium/content/browser/frame_host/navigator_impl.h
+++ b/chromium/content/browser/frame_host/navigator_impl.h
@@ -124,12 +124,12 @@ class CONTENT_EXPORT NavigatorImpl : public Navigator {
       const FrameHostMsg_DidCommitProvisionalLoad_Params& params,
       SiteInstance* site_instance);
 
-  // Called when a navigation has started in a main frame, to update the pending
-  // NavigationEntry if the controller does not currently have a
-  // browser-initiated one.
-  void DidStartMainFrameNavigation(const CommonNavigationParams& common_params,
-                                   SiteInstanceImpl* site_instance,
-                                   NavigationHandleImpl* navigation_handle);
+  // Called when a renderer initiated navigation has started. Returns the
+  // pending NavigationEntry to be used. Either null or a new one owned
+  // NavigationController.
+  NavigationEntryImpl* GetNavigationEntryForRendererInitiatedNavigation(
+                                   const CommonNavigationParams& common_params,
+                                   FrameTreeNode* frame_tree_node);
 
   // The NavigationController that will keep track of session history for all
   // RenderFrameHost objects using this NavigatorImpl.