Prevent bindings escalation on an existing NavigationEntry (attempt 3).

BUG=173672
TEST=See bug for repro steps.


Review URL: https://chromiumcodereview.appspot.com/12313067

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@184264 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 103 insertions, 6 deletions

diff --git a/content/browser/web_contents/navigation_controller_impl.cc b/content/browser/web_contents/navigation_controller_impl.cc
index b649dfa..3cb9009c 100644
--- a/content/browser/web_contents/navigation_controller_impl.cc
+++ b/content/browser/web_contents/navigation_controller_impl.cc
@@ -929,6 +929,11 @@ bool NavigationControllerImpl::RendererDidNavigate(
   // The active entry's SiteInstance should match our SiteInstance.
   CHECK(active_entry->site_instance() == web_contents_->GetSiteInstance());
 
+  // Remember the bindings the renderer process has at this point, so that
+  // we do not grant this entry additional bindings if we come back to it.
+  active_entry->SetBindings(
+      web_contents_->GetRenderViewHost()->GetEnabledBindings());
+
   // Now prep the rest of the details for the notification and broadcast.
   details->entry = active_entry;
   details->is_main_frame =
diff --git a/content/browser/web_contents/navigation_controller_impl_unittest.cc b/content/browser/web_contents/navigation_controller_impl_unittest.cc
index 3cda178..cc0b669 100644
--- a/content/browser/web_contents/navigation_controller_impl_unittest.cc
+++ b/content/browser/web_contents/navigation_controller_impl_unittest.cc
@@ -300,6 +300,8 @@ TEST_F(NavigationControllerTest, LoadURL) {
   EXPECT_FALSE(controller.CanGoBack());
   EXPECT_FALSE(controller.CanGoForward());
   EXPECT_EQ(contents()->GetMaxPageID(), 0);
+  EXPECT_EQ(0, NavigationEntryImpl::FromNavigationEntry(
+      controller.GetLastCommittedEntry())->bindings());
 
   // The timestamp should have been set.
   EXPECT_FALSE(controller.GetActiveEntry()->GetTimestamp().is_null());
@@ -865,6 +867,54 @@ TEST_F(NavigationControllerTest, LoadURL_RedirectAbortDoesntShowPendingURL) {
   contents()->SetDelegate(NULL);
 }
 
+// Ensure that NavigationEntries track which bindings their RenderViewHost had
+// at the time they committed.  http://crbug.com/173672.
+TEST_F(NavigationControllerTest, LoadURL_WithBindings) {
+  NavigationControllerImpl& controller = controller_impl();
+  TestNotificationTracker notifications;
+  RegisterForAllNavNotifications(&notifications, &controller);
+
+  const GURL url1("http://foo1");
+  const GURL url2("http://foo2");
+
+  // Navigate to a first, unprivileged URL.
+  controller.LoadURL(url1, Referrer(), PAGE_TRANSITION_TYPED, std::string());
+  EXPECT_EQ(NavigationEntryImpl::kInvalidBindings,
+            NavigationEntryImpl::FromNavigationEntry(
+                controller.GetPendingEntry())->bindings());
+
+  // Commit.
+  TestRenderViewHost* orig_rvh = static_cast<TestRenderViewHost*>(test_rvh());
+  orig_rvh->SendNavigate(0, url1);
+  EXPECT_EQ(controller.GetEntryCount(), 1);
+  EXPECT_EQ(0, controller.GetLastCommittedEntryIndex());
+  EXPECT_EQ(0, NavigationEntryImpl::FromNavigationEntry(
+      controller.GetLastCommittedEntry())->bindings());
+
+  // Navigate to a second URL, simulate the beforeunload ack for the cross-site
+  // transition, and set bindings on the pending RenderViewHost to simulate a
+  // privileged url.
+  controller.LoadURL(url2, Referrer(), PAGE_TRANSITION_TYPED, std::string());
+  orig_rvh->SendShouldCloseACK(true);
+  contents()->GetPendingRenderViewHost()->AllowBindings(1);
+  static_cast<TestRenderViewHost*>(
+      contents()->GetPendingRenderViewHost())->SendNavigate(1, url2);
+
+  // The second load should be committed, and bindings should be remembered.
+  EXPECT_EQ(controller.GetEntryCount(), 2);
+  EXPECT_EQ(1, controller.GetLastCommittedEntryIndex());
+  EXPECT_TRUE(controller.CanGoBack());
+  EXPECT_EQ(1, NavigationEntryImpl::FromNavigationEntry(
+      controller.GetLastCommittedEntry())->bindings());
+
+  // Going back, the first entry should still appear unprivileged.
+  controller.GoBack();
+  orig_rvh->SendNavigate(0, url1);
+  EXPECT_EQ(0, controller.GetLastCommittedEntryIndex());
+  EXPECT_EQ(0, NavigationEntryImpl::FromNavigationEntry(
+      controller.GetLastCommittedEntry())->bindings());
+}
+
 TEST_F(NavigationControllerTest, Reload) {
   NavigationControllerImpl& controller = controller_impl();
   TestNotificationTracker notifications;
diff --git a/content/browser/web_contents/navigation_entry_impl.cc b/content/browser/web_contents/navigation_entry_impl.cc
index 750480c2..dafc2e0 100644
--- a/content/browser/web_contents/navigation_entry_impl.cc
+++ b/content/browser/web_contents/navigation_entry_impl.cc
@@ -21,6 +21,8 @@ static int GetUniqueIDInConstructor() {
 
 namespace content {
 
+int NavigationEntryImpl::kInvalidBindings = -1;
+
 NavigationEntry* NavigationEntry::Create() {
   return new NavigationEntryImpl();
 }
@@ -37,6 +39,7 @@ NavigationEntryImpl* NavigationEntryImpl::FromNavigationEntry(
 NavigationEntryImpl::NavigationEntryImpl()
     : unique_id_(GetUniqueIDInConstructor()),
       site_instance_(NULL),
+      bindings_(kInvalidBindings),
       page_type_(PAGE_TYPE_NORMAL),
       update_virtual_url_with_url_(false),
       page_id_(-1),
@@ -59,6 +62,7 @@ NavigationEntryImpl::NavigationEntryImpl(SiteInstanceImpl* instance,
                                          bool is_renderer_initiated)
     : unique_id_(GetUniqueIDInConstructor()),
       site_instance_(instance),
+      bindings_(kInvalidBindings),
       page_type_(PAGE_TYPE_NORMAL),
       url_(url),
       referrer_(referrer),
@@ -149,6 +153,13 @@ void NavigationEntryImpl::set_site_instance(SiteInstanceImpl* site_instance) {
   site_instance_ = site_instance;
 }
 
+void NavigationEntryImpl::SetBindings(int bindings) {
+  // Ensure this is set to a valid value, and that it stays the same once set.
+  CHECK_NE(bindings, kInvalidBindings);
+  CHECK(bindings_ == kInvalidBindings || bindings_ == bindings);
+  bindings_ = bindings;
+}
+
 const string16& NavigationEntryImpl::GetTitleForDisplay(
     const std::string& languages) const {
   // Most pages have real titles. Don't even bother caching anything if this is
diff --git a/content/browser/web_contents/navigation_entry_impl.h b/content/browser/web_contents/navigation_entry_impl.h
index c23e2f0..62234bf 100644
--- a/content/browser/web_contents/navigation_entry_impl.h
+++ b/content/browser/web_contents/navigation_entry_impl.h
@@ -20,6 +20,9 @@ class CONTENT_EXPORT NavigationEntryImpl
  public:
   static NavigationEntryImpl* FromNavigationEntry(NavigationEntry* entry);
 
+  // The value of bindings() before it is set during commit.
+  static int kInvalidBindings;
+
   NavigationEntryImpl();
   NavigationEntryImpl(SiteInstanceImpl* instance,
                       int page_id,
@@ -96,6 +99,14 @@ class CONTENT_EXPORT NavigationEntryImpl
     return site_instance_.get();
   }
 
+  // Remember the set of bindings granted to this NavigationEntry at the time
+  // of commit, to ensure that we do not grant it additional bindings if we
+  // navigate back to it in the future.  This can only be changed once.
+  void SetBindings(int bindings);
+  int bindings() const {
+    return bindings_;
+  }
+
   void set_page_type(PageType page_type) {
     page_type_ = page_type;
   }
@@ -190,6 +201,8 @@ class CONTENT_EXPORT NavigationEntryImpl
   // See the accessors above for descriptions.
   int unique_id_;
   scoped_refptr<SiteInstanceImpl> site_instance_;
+  // TODO(creis): Persist bindings_. http://crbug.com/173672.
+  int bindings_;
   PageType page_type_;
   GURL url_;
   Referrer referrer_;
diff --git a/content/browser/web_contents/render_view_host_manager.cc b/content/browser/web_contents/render_view_host_manager.cc
index 4971206..74b9323 100644
--- a/content/browser/web_contents/render_view_host_manager.cc
+++ b/content/browser/web_contents/render_view_host_manager.cc
@@ -23,6 +23,7 @@
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
+#include "content/public/browser/user_metrics.h"
 #include "content/public/browser/web_contents_view.h"
 #include "content/public/browser/web_ui_controller.h"
 #include "content/public/common/content_switches.h"
@@ -97,6 +98,23 @@ RenderWidgetHostView* RenderViewHostManager::GetRenderWidgetHostView() const {
   return render_view_host_->GetView();
 }
 
+void RenderViewHostManager::SetPendingWebUI(const NavigationEntryImpl& entry) {
+  pending_web_ui_.reset(
+      delegate_->CreateWebUIForRenderManager(entry.GetURL()));
+  pending_and_current_web_ui_.reset();
+
+  // If we have assigned (zero or more) bindings to this NavigationEntry in the
+  // past, make sure we're not granting it different bindings than it had
+  // before.  If so, note it and don't give it any bindings, to avoid a
+  // potential privilege escalation.
+  if (pending_web_ui_.get() &&
+      entry.bindings() != NavigationEntryImpl::kInvalidBindings &&
+      pending_web_ui_->GetBindings() != entry.bindings()) {
+    RecordAction(UserMetricsAction("ProcessSwapBindingsMismatch_RVHM"));
+    pending_web_ui_.reset();
+  }
+}
+
 RenderViewHostImpl* RenderViewHostManager::Navigate(
     const NavigationEntryImpl& entry) {
   // Create a pending RenderViewHost. It will give us the one we should use
@@ -812,9 +830,7 @@ RenderViewHostImpl* RenderViewHostManager::UpdateRendererStateForNavigate(
     // It must also happen after the above conditional call to CancelPending(),
     // otherwise CancelPending may clear the pending_web_ui_ and the page will
     // not have its bindings set appropriately.
-    pending_web_ui_.reset(
-        delegate_->CreateWebUIForRenderManager(entry.GetURL()));
-    pending_and_current_web_ui_.reset();
+    SetPendingWebUI(entry);
 
     // Ensure that we have created RVHs for the new RVH's opener chain if
     // we are staying in the same BrowsingInstance. This allows the pending RVH
@@ -879,9 +895,7 @@ RenderViewHostImpl* RenderViewHostManager::UpdateRendererStateForNavigate(
       pending_web_ui_.reset();
       pending_and_current_web_ui_ = web_ui_->AsWeakPtr();
     } else {
-      pending_and_current_web_ui_.reset();
-      pending_web_ui_.reset(
-          delegate_->CreateWebUIForRenderManager(entry.GetURL()));
+      SetPendingWebUI(entry);
     }
 
     if (pending_web_ui() && render_view_host_->IsRenderViewLive())
diff --git a/content/browser/web_contents/render_view_host_manager.h b/content/browser/web_contents/render_view_host_manager.h
index 087d61c..96cb269 100644
--- a/content/browser/web_contents/render_view_host_manager.h
+++ b/content/browser/web_contents/render_view_host_manager.h
@@ -141,6 +141,10 @@ class CONTENT_EXPORT RenderViewHostManager
                                    pending_and_current_web_ui_.get();
   }
 
+  // Sets the pending Web UI for the pending navigation, ensuring that the
+  // bindings are appropriate for the given NavigationEntry.
+  void SetPendingWebUI(const NavigationEntryImpl& entry);
+
   // Called when we want to instruct the renderer to navigate to the given
   // navigation entry. It may create a new RenderViewHost or re-use an existing
   // one. The RenderViewHost to navigate will be returned. Returns NULL if one