Browser Plugin: Unify Attach IPC

BrowserPluginHostMsg_CreateGuest, and BrowserPluginHostMsg_Attach are two very similar IPCs that served slightly different purposes.

CreateGuest was used to create a new guest renderer.
Attach was used to attach to an existing (unattached) guest renderer.

We might, in the future, wish to expose this as an internal API to enable shims to use BrowserPlugin as their underlying infrastructure.

By unifying the two operations and adding additional security checks in BrowserPluginGuestManager, we reduce the likelihood of abuse for this API and fully define its behavior.

BUG=166165

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

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

6 files changed, 96 insertions, 44 deletions

diff --git a/content/browser/browser_plugin/browser_plugin_embedder.cc b/content/browser/browser_plugin/browser_plugin_embedder.cc
index 10df740f..01c1c34 100644
--- a/content/browser/browser_plugin/browser_plugin_embedder.cc
+++ b/content/browser/browser_plugin/browser_plugin_embedder.cc
@@ -62,7 +62,6 @@ bool BrowserPluginEmbedder::OnMessageReceived(const IPC::Message& message) {
     IPC_MESSAGE_HANDLER(BrowserPluginHostMsg_AllocateInstanceID,
                         OnAllocateInstanceID)
     IPC_MESSAGE_HANDLER(BrowserPluginHostMsg_Attach, OnAttach)
-    IPC_MESSAGE_HANDLER(BrowserPluginHostMsg_CreateGuest, OnCreateGuest)
     IPC_MESSAGE_HANDLER(BrowserPluginHostMsg_PluginAtPositionResponse,
                         OnPluginAtPositionResponse)
     IPC_MESSAGE_UNHANDLED(handled = false)
@@ -97,18 +96,21 @@ void BrowserPluginEmbedder::OnAllocateInstanceID(int request_id) {
 
 void BrowserPluginEmbedder::OnAttach(
     int instance_id,
-    const BrowserPluginHostMsg_CreateGuest_Params& params) {
+    const BrowserPluginHostMsg_Attach_Params& params) {
+  if (!GetBrowserPluginGuestManager()->CanEmbedderAccessInstanceIDMaybeKill(
+          web_contents()->GetRenderProcessHost()->GetID(), instance_id))
+    return;
+
   BrowserPluginGuest* guest =
       GetBrowserPluginGuestManager()->GetGuestByInstanceID(
           instance_id, web_contents()->GetRenderProcessHost()->GetID());
-  if (guest)
+
+  if (guest) {
     guest->Attach(static_cast<WebContentsImpl*>(web_contents()), params);
-}
+    return;
+  }
 
-void BrowserPluginEmbedder::OnCreateGuest(
-    int instance_id,
-    const BrowserPluginHostMsg_CreateGuest_Params& params) {
-  BrowserPluginGuest* guest = GetBrowserPluginGuestManager()->CreateGuest(
+  guest = GetBrowserPluginGuestManager()->CreateGuest(
       web_contents()->GetSiteInstance(), instance_id, params);
   if (guest)
     guest->Initialize(static_cast<WebContentsImpl*>(web_contents()), params);
diff --git a/content/browser/browser_plugin/browser_plugin_embedder.h b/content/browser/browser_plugin/browser_plugin_embedder.h
index ddcf6f1..9c04a1f 100644
--- a/content/browser/browser_plugin/browser_plugin_embedder.h
+++ b/content/browser/browser_plugin/browser_plugin_embedder.h
@@ -19,7 +19,7 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 
-struct BrowserPluginHostMsg_CreateGuest_Params;
+struct BrowserPluginHostMsg_Attach_Params;
 struct BrowserPluginHostMsg_ResizeGuest_Params;
 
 namespace gfx {
@@ -69,9 +69,7 @@ class CONTENT_EXPORT BrowserPluginEmbedder : public WebContentsObserver {
 
   void OnAllocateInstanceID(int request_id);
   void OnAttach(int instance_id,
-                const BrowserPluginHostMsg_CreateGuest_Params& params);
-  void OnCreateGuest(int instance_id,
-                     const BrowserPluginHostMsg_CreateGuest_Params& params);
+                const BrowserPluginHostMsg_Attach_Params& params);
   void OnPluginAtPositionResponse(int instance_id,
                                   int request_id,
                                   const gfx::Point& position);
diff --git a/content/browser/browser_plugin/browser_plugin_guest.cc b/content/browser/browser_plugin/browser_plugin_guest.cc
index 8627e1e..18a25c0 100644
--- a/content/browser/browser_plugin/browser_plugin_guest.cc
+++ b/content/browser/browser_plugin/browser_plugin_guest.cc
@@ -224,7 +224,7 @@ bool BrowserPluginGuest::OnMessageReceivedFromEmbedder(
 
 void BrowserPluginGuest::Initialize(
     WebContentsImpl* embedder_web_contents,
-    const BrowserPluginHostMsg_CreateGuest_Params& params) {
+    const BrowserPluginHostMsg_Attach_Params& params) {
   focused_ = params.focused;
   guest_visible_ = params.visible;
   if (!params.name.empty())
@@ -743,7 +743,15 @@ bool BrowserPluginGuest::OnMessageReceived(const IPC::Message& message) {
 
 void BrowserPluginGuest::Attach(
     WebContentsImpl* embedder_web_contents,
-    BrowserPluginHostMsg_CreateGuest_Params params) {
+    BrowserPluginHostMsg_Attach_Params params) {
+  if (attached())
+    return;
+
+  // Clear parameters that get inherited from the opener.
+  params.storage_partition_id.clear();
+  params.persist_storage = false;
+  params.src.clear();
+
   const std::string target_url = opener()->pending_new_windows_[this];
   if (!GetWebContents()->opener()) {
     // For guests that have a suppressed opener, we navigate now.
diff --git a/content/browser/browser_plugin/browser_plugin_guest.h b/content/browser/browser_plugin/browser_plugin_guest.h
index c335c4b..526d183 100644
--- a/content/browser/browser_plugin/browser_plugin_guest.h
+++ b/content/browser/browser_plugin/browser_plugin_guest.h
@@ -42,7 +42,7 @@
 #include "ui/surface/transport_dib.h"
 
 struct BrowserPluginHostMsg_AutoSize_Params;
-struct BrowserPluginHostMsg_CreateGuest_Params;
+struct BrowserPluginHostMsg_Attach_Params;
 struct BrowserPluginHostMsg_ResizeGuest_Params;
 struct ViewHostMsg_CreateWindow_Params;
 #if defined(OS_MACOSX)
@@ -107,7 +107,7 @@ class CONTENT_EXPORT BrowserPluginGuest : public NotificationObserver,
   bool OnMessageReceivedFromEmbedder(const IPC::Message& message);
 
   void Initialize(WebContentsImpl* embedder_web_contents,
-                  const BrowserPluginHostMsg_CreateGuest_Params& params);
+                  const BrowserPluginHostMsg_Attach_Params& params);
 
   void set_guest_hang_timeout_for_testing(const base::TimeDelta& timeout) {
     guest_hang_timeout_ = timeout;
@@ -220,7 +220,7 @@ class CONTENT_EXPORT BrowserPluginGuest : public NotificationObserver,
   // to an embedder implies that this guest's lifetime is no longer managed
   // by its opener, and it can begin loading resources.
   void Attach(WebContentsImpl* embedder_web_contents,
-              BrowserPluginHostMsg_CreateGuest_Params params);
+              BrowserPluginHostMsg_Attach_Params params);
 
   // Requests geolocation permission through Embedder JavaScript API.
   void AskEmbedderForGeolocationPermission(int bridge_id,
diff --git a/content/browser/browser_plugin/browser_plugin_guest_manager.cc b/content/browser/browser_plugin/browser_plugin_guest_manager.cc
index 6a41b50..081f332 100644
--- a/content/browser/browser_plugin/browser_plugin_guest_manager.cc
+++ b/content/browser/browser_plugin/browser_plugin_guest_manager.cc
@@ -40,14 +40,8 @@ BrowserPluginGuestManager* BrowserPluginGuestManager::Create() {
 BrowserPluginGuest* BrowserPluginGuestManager::CreateGuest(
     SiteInstance* embedder_site_instance,
     int instance_id,
-    const BrowserPluginHostMsg_CreateGuest_Params& params) {
+    const BrowserPluginHostMsg_Attach_Params& params) {
   SiteInstance* guest_site_instance = NULL;
-  int embedder_render_process_id =
-      embedder_site_instance->GetProcess()->GetID();
-  BrowserPluginGuest* guest =
-      GetGuestByInstanceID(instance_id, embedder_render_process_id);
-  CHECK(!guest);
-
   // Validate that the partition id coming from the renderer is valid UTF-8,
   // since we depend on this in other parts of the code, such as FilePath
   // creation. If the validation fails, treat it as a bad message and kill the
@@ -110,22 +104,15 @@ BrowserPluginGuest* BrowserPluginGuestManager::CreateGuest(
 BrowserPluginGuest* BrowserPluginGuestManager::GetGuestByInstanceID(
     int instance_id,
     int embedder_render_process_id) const {
+  if (!CanEmbedderAccessInstanceIDMaybeKill(embedder_render_process_id,
+                                            instance_id)) {
+    return NULL;
+  }
   GuestInstanceMap::const_iterator it =
       guest_web_contents_by_instance_id_.find(instance_id);
   if (it == guest_web_contents_by_instance_id_.end())
     return NULL;
-
-  BrowserPluginGuest* guest =
-      static_cast<WebContentsImpl*>(it->second)->GetBrowserPluginGuest();
-  if (!CanEmbedderAccessGuest(embedder_render_process_id, guest)) {
-    // The embedder process is trying to access a guest it does not own.
-    content::RecordAction(UserMetricsAction("BadMessageTerminate_BPGM"));
-    base::KillProcess(
-        RenderProcessHost::FromID(embedder_render_process_id)->GetHandle(),
-        content::RESULT_CODE_KILLED_BAD_MESSAGE, false);
-    return NULL;
-  }
-  return guest;
+  return static_cast<WebContentsImpl*>(it->second)->GetBrowserPluginGuest();
 }
 
 void BrowserPluginGuestManager::AddGuest(int instance_id,
@@ -141,6 +128,20 @@ void BrowserPluginGuestManager::RemoveGuest(int instance_id) {
   guest_web_contents_by_instance_id_.erase(instance_id);
 }
 
+bool BrowserPluginGuestManager::CanEmbedderAccessInstanceIDMaybeKill(
+    int embedder_render_process_id,
+    int instance_id) const {
+  if (!CanEmbedderAccessInstanceID(embedder_render_process_id, instance_id)) {
+    // The embedder process is trying to access a guest it does not own.
+    content::RecordAction(UserMetricsAction("BadMessageTerminate_BPGM"));
+    base::KillProcess(
+        RenderProcessHost::FromID(embedder_render_process_id)->GetHandle(),
+        content::RESULT_CODE_KILLED_BAD_MESSAGE, false);
+    return false;
+  }
+  return true;
+}
+
 void BrowserPluginGuestManager::OnMessageReceived(const IPC::Message& message,
                                                   int render_process_id) {
   if (BrowserPluginGuest::ShouldForwardToBrowserPluginGuest(message)) {
@@ -160,11 +161,11 @@ void BrowserPluginGuestManager::OnMessageReceived(const IPC::Message& message,
   IPC_END_MESSAGE_MAP()
 }
 
-//static
+// static
 bool BrowserPluginGuestManager::CanEmbedderAccessGuest(
     int embedder_render_process_id,
     BrowserPluginGuest* guest) {
-  // An embedder can access |guest| if the guest has not been attached and its
+  // The embedder can access the guest if it has not been attached and its
   // opener's embedder lives in the same process as the given embedder.
   if (!guest->attached()) {
     if (!guest->opener())
@@ -179,6 +180,31 @@ bool BrowserPluginGuestManager::CanEmbedderAccessGuest(
       guest->embedder_web_contents()->GetRenderProcessHost()->GetID();
 }
 
+bool BrowserPluginGuestManager::CanEmbedderAccessInstanceID(
+    int embedder_render_process_id,
+    int instance_id) const {
+  // The embedder is trying to access a guest with a negative or zero
+  // instance ID.
+  if (instance_id <= browser_plugin::kInstanceIDNone)
+    return false;
+
+  // The embedder is trying to access an instance ID that has not yet been
+  // allocated by BrowserPluginGuestManager. This could cause instance ID
+  // collisions in the future, and potentially give one embedder access to a
+  // guest it does not own.
+  if (instance_id > next_instance_id_)
+    return false;
+
+  GuestInstanceMap::const_iterator it =
+      guest_web_contents_by_instance_id_.find(instance_id);
+  if (it == guest_web_contents_by_instance_id_.end())
+    return true;
+  BrowserPluginGuest* guest =
+      static_cast<WebContentsImpl*>(it->second)->GetBrowserPluginGuest();
+
+  return CanEmbedderAccessGuest(embedder_render_process_id, guest);
+}
+
 SiteInstance* BrowserPluginGuestManager::GetGuestSiteInstance(
     const GURL& guest_site) {
   for (GuestInstanceMap::const_iterator it =
diff --git a/content/browser/browser_plugin/browser_plugin_guest_manager.h b/content/browser/browser_plugin/browser_plugin_guest_manager.h
index 4f5bb9b..4e1ba28 100644
--- a/content/browser/browser_plugin/browser_plugin_guest_manager.h
+++ b/content/browser/browser_plugin/browser_plugin_guest_manager.h
@@ -14,7 +14,7 @@
 #include "content/common/content_export.h"
 #include "ipc/ipc_message.h"
 
-struct BrowserPluginHostMsg_CreateGuest_Params;
+struct BrowserPluginHostMsg_Attach_Params;
 struct BrowserPluginHostMsg_ResizeGuest_Params;
 class GURL;
 
@@ -58,19 +58,27 @@ class CONTENT_EXPORT BrowserPluginGuestManager :
   BrowserPluginGuest* CreateGuest(
       SiteInstance* embedder_site_instance,
       int instance_id,
-      const BrowserPluginHostMsg_CreateGuest_Params& params);
+      const BrowserPluginHostMsg_Attach_Params& params);
 
-  // Returns a BrowserPluginGuest given an instance ID. Returns NULL if the
-  // guest wasn't found.  If the guest doesn't belong to the given embedder,
-  // then NULL is returned and the embedder is killed.
+  // Returns a BrowserPluginGuest given an |instance_id|. Returns NULL if the
+  // guest wasn't found.  If the embedder is not permitted to access the given
+  // |instance_id|, the embedder is killed, and NULL is returned.
   BrowserPluginGuest* GetGuestByInstanceID(
       int instance_id,
       int embedder_render_process_id) const;
 
   // Adds a new |guest_web_contents| to the embedder (overridable in test).
   virtual void AddGuest(int instance_id, WebContentsImpl* guest_web_contents);
+
+  // Removes the guest with the given |instance_id| from this
+  // BrowserPluginGuestManager.
   void RemoveGuest(int instance_id);
 
+  // Returns whether the specified embedder is permitted to access the given
+  // |instance_id|, and kills the embedder if not.
+  bool CanEmbedderAccessInstanceIDMaybeKill(int embedder_render_process_id,
+                                            int instance_id) const;
+
   void OnMessageReceived(const IPC::Message& message, int render_process_id);
 
  private:
@@ -78,10 +86,20 @@ class CONTENT_EXPORT BrowserPluginGuestManager :
 
   BrowserPluginGuestManager();
 
-  // Returns whether the given embedder process is allowed to access |guest|.
+  // Returns whether the given embedder process is allowed to access the
+  // provided |guest|.
   static bool CanEmbedderAccessGuest(int embedder_render_process_id,
                                      BrowserPluginGuest* guest);
 
+  // Returns whether the given embedder process is allowed to use the provided
+  // |instance_id| or access the guest associated with the |instance_id|. If the
+  // embedder can, the method returns true. If the guest does not exist but the
+  // embedder can use that |instance_id|, then it returns true. If the embedder
+  // is not permitted to use that instance ID or access the associated guest,
+  // then it returns false.
+  bool CanEmbedderAccessInstanceID(int embedder_render_process_id,
+                                   int instance_id) const;
+
   // Returns an existing SiteInstance if the current profile has a guest of the
   // given |guest_site|.
   SiteInstance* GetGuestSiteInstance(const GURL& guest_site);