Don't update PageState for a SiteInstance mismatch.

BUG=766262
TEST=See bug for repro.

Change-Id: Ifb087b687acd40d8963ef436c9ea82ca2d960117
Reviewed-on: https://chromium-review.googlesource.com/674808
Commit-Queue: Charlie Reis (OOO until 9/25) <creis@chromium.org>
Reviewed-by: Ɓukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503297}
diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc
index e0aef405..822104b 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -936,8 +936,14 @@
   active_entry->SetTimestamp(timestamp);
   active_entry->SetHttpStatusCode(params.http_status_code);
 
+  // Grab the corresponding FrameNavigationEntry for a few updates, but only if
+  // the SiteInstance matches (to avoid updating the wrong entry by mistake).
+  // A mismatch can occur if the renderer lies or due to a unique name collision
+  // after a race with an OOPIF (see https://crbug.com/616820).
   FrameNavigationEntry* frame_entry =
       active_entry->GetFrameEntry(rfh->frame_tree_node());
+  if (frame_entry && frame_entry->site_instance() != rfh->GetSiteInstance())
+    frame_entry = nullptr;
   // Update the frame-specific PageState and RedirectChain
   // We may not find a frame_entry in some cases; ignore the PageState if so.
   // TODO(creis): Remove the "if" once https://crbug.com/522193 is fixed.
diff --git a/content/browser/security_exploit_browsertest.cc b/content/browser/security_exploit_browsertest.cc
index 79c9415..e41a5d8 100644
--- a/content/browser/security_exploit_browsertest.cc
+++ b/content/browser/security_exploit_browsertest.cc
@@ -39,6 +39,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/test_navigation_observer.h"
 #include "content/public/test/test_utils.h"
 #include "content/shell/browser/shell.h"
 #include "content/test/content_browser_test_utils_internal.h"
@@ -629,4 +630,83 @@
   EXPECT_FALSE(exit_observer.did_exit_normally());
 }
 
+// Test that forging a frame's unique name and commit won't allow changing the
+// PageState of a cross-process FrameNavigationEntry.
+// See https://crbug.com/766262.
+IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest, PageStateToWrongEntry) {
+  IsolateAllSitesForTesting(base::CommandLine::ForCurrentProcess());
+
+  // Commit a page with nested iframes and a separate cross-process iframe.
+  GURL main_url(embedded_test_server()->GetURL(
+      "a.com", "/cross_site_iframe_factory.html?a(a(a),b)"));
+  EXPECT_TRUE(NavigateToURL(shell(), main_url));
+  NavigationEntryImpl* back_entry = static_cast<NavigationEntryImpl*>(
+      shell()->web_contents()->GetController().GetLastCommittedEntry());
+  int nav_entry_id = back_entry->GetUniqueID();
+
+  FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
+                            ->GetFrameTree()
+                            ->root();
+  FrameTreeNode* child0_0 = root->child_at(0)->child_at(0);
+  std::string child0_0_unique_name = child0_0->unique_name();
+  FrameTreeNode* child1 = root->child_at(1);
+  GURL child1_url = child1->current_url();
+  int child1_pid = child1->current_frame_host()->GetProcess()->GetID();
+  PageState child1_page_state = back_entry->GetFrameEntry(child1)->page_state();
+
+  // Add a history item in the nested frame.  It's important to do it there and
+  // not the main frame for the repro to work, since we don't walk the subtree
+  // when navigating back/forward between same document items.
+  TestNavigationObserver fragment_observer(shell()->web_contents());
+  EXPECT_TRUE(ExecuteScript(child0_0, "location.href = '#foo';"));
+  fragment_observer.Wait();
+
+  // Simulate a name change IPC from the nested iframe, matching the cross-site
+  // iframe's unique name.
+  child0_0->SetFrameName("foo", child1->unique_name());
+
+  // Simulate a back navigation from the now renamed nested iframe, which would
+  // put a PageState on the cross-site iframe's FrameNavigationEntry.  Forge a
+  // data URL within the PageState that differs from child1_url.
+  std::unique_ptr<FrameHostMsg_DidCommitProvisionalLoad_Params> params =
+      base::MakeUnique<FrameHostMsg_DidCommitProvisionalLoad_Params>();
+  params->nav_entry_id = nav_entry_id;
+  params->frame_unique_name = "";
+  params->did_create_new_entry = false;
+  params->url = GURL("about:blank");
+  params->transition = ui::PAGE_TRANSITION_AUTO_SUBFRAME;
+  params->should_update_history = false;
+  params->gesture = NavigationGestureAuto;
+  params->was_within_same_document = false;
+  params->method = "GET";
+  params->page_state = PageState::CreateFromURL(GURL("data:text/html,foo"));
+  params->origin = url::Origin(GURL("about:blank"));
+
+  static_cast<mojom::FrameHost*>(child0_0->current_frame_host())
+      ->DidCommitProvisionalLoad(std::move(params));
+
+  // Make sure we haven't changed the FrameNavigationEntry.  An attack would
+  // modify the PageState but leave the SiteInstance as it was.
+  EXPECT_EQ(child1->current_frame_host()->GetSiteInstance(),
+            back_entry->GetFrameEntry(child1)->site_instance());
+  EXPECT_EQ(child1_page_state, back_entry->GetFrameEntry(child1)->page_state());
+
+  // Put the frame's unique name back.
+  child0_0->SetFrameName("bar", child0_0_unique_name);
+
+  // Go forward after the fake back navigation.
+  TestNavigationObserver forward_observer(shell()->web_contents());
+  shell()->web_contents()->GetController().GoForward();
+  forward_observer.Wait();
+
+  // Go back to the possibly corrupted entry and ensure we didn't load the data
+  // URL in the previous process.  A test failure here would appear as a failure
+  // of the URL check and not the process ID check.
+  TestNavigationObserver back_observer(shell()->web_contents());
+  shell()->web_contents()->GetController().GoBack();
+  back_observer.Wait();
+  EXPECT_EQ(child1_pid, child1->current_frame_host()->GetProcess()->GetID());
+  ASSERT_EQ(child1_url, child1->current_url());
+}
+
 }  // namespace content