The renderer and plugin processes can send over raw NPObjects valid in the other side's address

space. Basically the way this works is if an NPObject is marshaled over to the other side, an
NPObjectStub is created in the caller address space and a NPObjectProxy is created on the other side.
The NPObjectProxy is passed the raw NPObject pointer which is used as a cookie.

If the original NPObject needs to be passed back we pass the underlying NPObject saved in the NPObjectProxy.
The receiver does not validate whether this NPObject is valid before invoking on it.

While this is mostly fine, in the case of a compromised renderer invalid addresses could be passed back
to the plugin which would invoke on these addresses and crash.

Fix is to never pass raw object pointers across and just pass the corresponding routing id of the NPObjectStub.
The receiver validates this object by invoking a new method GetNPObjectListenerForRoute on the PluginChannelBase.
This method returns the corresponding NPObject listener for the routing id. We then retrieve the underlying NPObject
from the listener and use it.

The map of NPObjectListeners which is maintained by PluginChannelBase has been changed to hold NPObjectBase
pointers instead. NPObjectStub and NPObjectProxy implement the new NPObjectBase interface which provides 
methods to return the underlying NPObject and the IPC::Channel::Listener pointer.

Fixes bug http://code.google.com/p/chromium/issues/detail?id=31880

I verified with the steps outlined in the bug that this fix does address the underlying crash.

Bug=31880
Test=We need a framework to test PluginChannel and NPObjectProxy/Stub. Will add a test case for this
     once we have this in place.

Review URL: http://codereview.chromium.org/548046

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

10 files changed, 135 insertions, 56 deletions

diff --git a/chrome/plugin/npobject_base.h b/chrome/plugin/npobject_base.h
new file mode 100644
index 0000000..4b0d892
--- /dev/null
+++ b/chrome/plugin/npobject_base.h
@@ -0,0 +1,27 @@
+// Copyright (c) 2006-2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// Base interface implemented by NPObjectProxy and NPObjectStub
+
+#ifndef CHROME_PLUGIN_NPOBJECT_BASE_H_
+#define CHROME_PLUGIN_NPOBJECT_BASE_H_
+
+#include "ipc/ipc_channel.h"
+#include "third_party/npapi/bindings/npruntime.h"
+
+struct NPObject;
+
+class NPObjectBase {
+ public:
+  virtual ~NPObjectBase() {}
+
+  // Returns the underlying NPObject handled by this NPObjectBase instance.
+  virtual NPObject* GetUnderlyingNPObject() = 0;
+
+  // Returns the channel listener for this NPObjectBase instance.
+  virtual IPC::Channel::Listener* GetChannelListener() = 0;
+};
+
+#endif  // CHROME_PLUGIN_NPOBJECT_BASE_H_
+
diff --git a/chrome/plugin/npobject_proxy.cc b/chrome/plugin/npobject_proxy.cc
index a55442b..aff1bcd 100644
--- a/chrome/plugin/npobject_proxy.cc
+++ b/chrome/plugin/npobject_proxy.cc
@@ -49,15 +49,13 @@ NPObjectProxy* NPObjectProxy::GetProxy(NPObject* object) {
 NPObjectProxy::NPObjectProxy(
     PluginChannelBase* channel,
     int route_id,
-    intptr_t npobject_ptr,
     gfx::NativeViewId containing_window,
     const GURL& page_url)
     : channel_(channel),
       route_id_(route_id),
-      npobject_ptr_(npobject_ptr),
       containing_window_(containing_window),
       page_url_(page_url) {
-  channel_->AddRoute(route_id, this, true);
+  channel_->AddRoute(route_id, this, this);
 }
 
 NPObjectProxy::~NPObjectProxy() {
@@ -70,13 +68,12 @@ NPObjectProxy::~NPObjectProxy() {
 
 NPObject* NPObjectProxy::Create(PluginChannelBase* channel,
                                 int route_id,
-                                intptr_t npobject_ptr,
                                 gfx::NativeViewId containing_window,
                                 const GURL& page_url) {
   NPObjectWrapper* obj = reinterpret_cast<NPObjectWrapper*>(
       WebBindings::createObject(0, &npclass_proxy_));
   obj->proxy = new NPObjectProxy(
-      channel, route_id, npobject_ptr, containing_window, page_url);
+      channel, route_id, containing_window, page_url);
 
   return reinterpret_cast<NPObject*>(obj);
 }
diff --git a/chrome/plugin/npobject_proxy.h b/chrome/plugin/npobject_proxy.h
index c5f3e597..f372a52 100644
--- a/chrome/plugin/npobject_proxy.h
+++ b/chrome/plugin/npobject_proxy.h
@@ -10,6 +10,7 @@
 
 #include "app/gfx/native_widget_types.h"
 #include "base/ref_counted.h"
+#include "chrome/plugin/npobject_base.h"
 #include "googleurl/src/gurl.h"
 #include "ipc/ipc_channel.h"
 #include "third_party/npapi/bindings/npruntime.h"
@@ -26,13 +27,13 @@ struct NPObject;
 // side translates the IPC messages into calls to the actual NPObject, and
 // returns the marshalled result.
 class NPObjectProxy : public IPC::Channel::Listener,
-                      public IPC::Message::Sender {
+                      public IPC::Message::Sender,
+                      public NPObjectBase {
  public:
   ~NPObjectProxy();
 
   static NPObject* Create(PluginChannelBase* channel,
                           int route_id,
-                          intptr_t npobject_ptr,
                           gfx::NativeViewId containing_window,
                           const GURL& page_url);
 
@@ -41,10 +42,6 @@ class NPObjectProxy : public IPC::Channel::Listener,
   int route_id() { return route_id_; }
   PluginChannelBase* channel() { return channel_; }
 
-  // Returns the real NPObject's pointer (obviously only valid in the other
-  // process).
-  intptr_t npobject_ptr() { return npobject_ptr_; }
-
   // The next 9 functions are called on NPObjects from the plugin and browser.
   static bool NPHasMethod(NPObject *obj,
                           NPIdentifier name);
@@ -92,10 +89,18 @@ class NPObjectProxy : public IPC::Channel::Listener,
   static NPObjectProxy* GetProxy(NPObject* object);
   static const NPClass* npclass() { return &npclass_proxy_; }
 
+  // NPObjectBase implementation.
+  virtual NPObject* GetUnderlyingNPObject() {
+    return NULL;
+  }
+
+  IPC::Channel::Listener* GetChannelListener() {
+    return static_cast<IPC::Channel::Listener*>(this);
+  }
+
  private:
   NPObjectProxy(PluginChannelBase* channel,
                 int route_id,
-                intptr_t npobject_ptr,
                 gfx::NativeViewId containing_window,
                 const GURL& page_url);
 
@@ -112,7 +117,6 @@ class NPObjectProxy : public IPC::Channel::Listener,
 
   scoped_refptr<PluginChannelBase> channel_;
   int route_id_;
-  intptr_t npobject_ptr_;
   gfx::NativeViewId containing_window_;
 
   // The url of the main frame hosting the plugin.
diff --git a/chrome/plugin/npobject_stub.cc b/chrome/plugin/npobject_stub.cc
index a23f2dc..ec69c43 100644
--- a/chrome/plugin/npobject_stub.cc
+++ b/chrome/plugin/npobject_stub.cc
@@ -27,7 +27,7 @@ NPObjectStub::NPObjectStub(
       route_id_(route_id),
       containing_window_(containing_window),
       page_url_(page_url) {
-  channel_->AddRoute(route_id, this, true);
+  channel_->AddRoute(route_id, this, this);
 
   // We retain the object just as PluginHost does if everything was in-process.
   WebBindings::retainObject(npobject_);
@@ -124,13 +124,19 @@ void NPObjectStub::OnInvoke(bool is_default,
   NPVariant result_var;
 
   VOID_TO_NPVARIANT(result_var);
+  result_param.type = NPVARIANT_PARAM_VOID;
 
   int arg_count = static_cast<int>(args.size());
   NPVariant* args_var = new NPVariant[arg_count];
   for (int i = 0; i < arg_count; ++i) {
-    CreateNPVariant(
-        args[i], local_channel, &(args_var[i]), containing_window_,
-        page_url_);
+    if (!CreateNPVariant(
+            args[i], local_channel, &(args_var[i]), containing_window_,
+            page_url_)) {
+      NPObjectMsg_Invoke::WriteReplyParams(reply_msg, result_param,
+                                           return_value);
+      local_channel->Send(reply_msg);
+      return;
+    }
   }
 
   if (is_default) {
@@ -210,13 +216,17 @@ void NPObjectStub::OnGetProperty(const NPIdentifier_Param& name,
 void NPObjectStub::OnSetProperty(const NPIdentifier_Param& name,
                                  const NPVariant_Param& property,
                                  IPC::Message* reply_msg) {
-  bool result;
+  bool result = false;
   NPVariant result_var;
   VOID_TO_NPVARIANT(result_var);
   NPIdentifier id = CreateNPIdentifier(name);
   NPVariant property_var;
-  CreateNPVariant(
-      property, channel_, &property_var, containing_window_, page_url_);
+  if (!CreateNPVariant(
+          property, channel_, &property_var, containing_window_, page_url_)) {
+    NPObjectMsg_SetProperty::WriteReplyParams(reply_msg, result);
+    channel_->Send(reply_msg);
+    return;
+  }
 
   if (IsPluginProcess()) {
     if (npobject_->_class->setProperty) {
@@ -314,8 +324,14 @@ void NPObjectStub::OnConstruct(const std::vector<NPVariant_Param>& args,
   int arg_count = static_cast<int>(args.size());
   NPVariant* args_var = new NPVariant[arg_count];
   for (int i = 0; i < arg_count; ++i) {
-    CreateNPVariant(
-        args[i], local_channel, &(args_var[i]), containing_window_, page_url_);
+    if (!CreateNPVariant(
+           args[i], local_channel, &(args_var[i]), containing_window_,
+           page_url_)) {
+      NPObjectMsg_Invoke::WriteReplyParams(reply_msg, result_param,
+                                           return_value);
+      local_channel->Send(reply_msg);
+      return;
+    }
   }
 
   if (IsPluginProcess()) {
diff --git a/chrome/plugin/npobject_stub.h b/chrome/plugin/npobject_stub.h
index a2e2022..2d3b190 100644
--- a/chrome/plugin/npobject_stub.h
+++ b/chrome/plugin/npobject_stub.h
@@ -13,6 +13,7 @@
 #include "app/gfx/native_widget_types.h"
 #include "base/ref_counted.h"
 #include "base/weak_ptr.h"
+#include "chrome/plugin/npobject_base.h"
 #include "googleurl/src/gurl.h"
 #include "ipc/ipc_channel.h"
 
@@ -26,7 +27,8 @@ struct NPVariant_Param;
 // more information.
 class NPObjectStub : public IPC::Channel::Listener,
                      public IPC::Message::Sender,
-                     public base::SupportsWeakPtr<NPObjectStub> {
+                     public base::SupportsWeakPtr<NPObjectStub>,
+                     public NPObjectBase {
  public:
   NPObjectStub(NPObject* npobject,
                PluginChannelBase* channel,
@@ -43,6 +45,15 @@ class NPObjectStub : public IPC::Channel::Listener,
   // window script object on destruction to avoid leaks.
   void OnPluginDestroyed();
 
+  // NPObjectBase implementation.
+  virtual NPObject* GetUnderlyingNPObject() {
+    return npobject_;
+  }
+
+  IPC::Channel::Listener* GetChannelListener() {
+    return static_cast<IPC::Channel::Listener*>(this);
+  }
+
  private:
   // IPC::Channel::Listener implementation:
   void OnMessageReceived(const IPC::Message& message);
diff --git a/chrome/plugin/npobject_util.cc b/chrome/plugin/npobject_util.cc
index 229a984..c0c3d10f 100644
--- a/chrome/plugin/npobject_util.cc
+++ b/chrome/plugin/npobject_util.cc
@@ -176,12 +176,12 @@ void CreateNPVariantParam(const NPVariant& variant,
                                    variant.value.stringValue.UTF8Length);
       }
       break;
-    case NPVariantType_Object:
-    {
+    case NPVariantType_Object: {
       if (variant.value.objectValue->_class == NPObjectProxy::npclass()) {
-        param->type = NPVARIANT_PARAM_OBJECT_POINTER;
-        param->npobject_pointer =
-            NPObjectProxy::GetProxy(variant.value.objectValue)->npobject_ptr();
+        param->type = NPVARIANT_PARAM_RECEIVER_OBJECT_ROUTING_ID;
+        NPObjectProxy* proxy =
+            NPObjectProxy::GetProxy(variant.value.objectValue);
+        param->npobject_routing_id = proxy->route_id();
         // Don't release, because our original variant is the same as our proxy.
         release = false;
       } else {
@@ -191,14 +191,12 @@ void CreateNPVariantParam(const NPVariant& variant,
           // NPObjectStub adds its own reference to the NPObject it owns, so if
           // we were supposed to release the corresponding variant
           // (release==true), we should still do that.
-          param->type = NPVARIANT_PARAM_OBJECT_ROUTING_ID;
+          param->type = NPVARIANT_PARAM_SENDER_OBJECT_ROUTING_ID;
           int route_id = channel->GenerateRouteID();
           new NPObjectStub(
               variant.value.objectValue, channel, route_id, containing_window,
               page_url);
           param->npobject_routing_id = route_id;
-          param->npobject_pointer =
-              reinterpret_cast<intptr_t>(variant.value.objectValue);
         } else {
           param->type = NPVARIANT_PARAM_VOID;
         }
@@ -213,7 +211,7 @@ void CreateNPVariantParam(const NPVariant& variant,
     WebBindings::releaseVariantValue(const_cast<NPVariant*>(&variant));
 }
 
-void CreateNPVariant(const NPVariant_Param& param,
+bool CreateNPVariant(const NPVariant_Param& param,
                      PluginChannelBase* channel,
                      NPVariant* result,
                      gfx::NativeViewId containing_window,
@@ -244,22 +242,32 @@ void CreateNPVariant(const NPVariant_Param& param,
       result->value.stringValue.UTF8Length =
           static_cast<int>(param.string_value.size());
       break;
-    case NPVARIANT_PARAM_OBJECT_ROUTING_ID:
+    case NPVARIANT_PARAM_SENDER_OBJECT_ROUTING_ID:
       result->type = NPVariantType_Object;
       result->value.objectValue =
           NPObjectProxy::Create(channel,
                                 param.npobject_routing_id,
-                                param.npobject_pointer,
                                 containing_window,
                                 page_url);
       break;
-    case NPVARIANT_PARAM_OBJECT_POINTER:
+    case NPVARIANT_PARAM_RECEIVER_OBJECT_ROUTING_ID: {
+      NPObjectBase* npobject_base =
+          channel->GetNPObjectListenerForRoute(param.npobject_routing_id);
+      if (!npobject_base) {
+        DLOG(WARNING) << "Invalid routing id passed in"
+                      << param.npobject_routing_id;
+        return false;
+      }
+
+      DCHECK(npobject_base->GetUnderlyingNPObject() != NULL);
+
       result->type = NPVariantType_Object;
-      result->value.objectValue =
-          reinterpret_cast<NPObject*>(param.npobject_pointer);
+      result->value.objectValue = npobject_base->GetUnderlyingNPObject();
       WebBindings::retainObject(result->value.objectValue);
       break;
+    }
     default:
       NOTREACHED();
   }
+  return true;
 }
diff --git a/chrome/plugin/npobject_util.h b/chrome/plugin/npobject_util.h
index 3fcc701..1334e79 100644
--- a/chrome/plugin/npobject_util.h
+++ b/chrome/plugin/npobject_util.h
@@ -53,7 +53,8 @@ void CreateNPVariantParam(const NPVariant& variant,
                           const GURL& page_url);
 
 // Creates an NPVariant from the marshalled object.
-void CreateNPVariant(const NPVariant_Param& param,
+// Returns true on success.
+bool CreateNPVariant(const NPVariant_Param& param,
                      PluginChannelBase* channel,
                      NPVariant* result,
                      gfx::NativeViewId containing_window,
diff --git a/chrome/plugin/plugin_channel_base.cc b/chrome/plugin/plugin_channel_base.cc
index 026b9e5..f104fea 100644
--- a/chrome/plugin/plugin_channel_base.cc
+++ b/chrome/plugin/plugin_channel_base.cc
@@ -95,6 +95,15 @@ void PluginChannelBase::CleanupChannels() {
   g_plugin_channels_.clear();
 }
 
+NPObjectBase* PluginChannelBase::GetNPObjectListenerForRoute(int route_id) {
+  ListenerMap::iterator iter = npobject_listeners_.find(route_id);
+  if (iter == npobject_listeners_.end()) {
+    DLOG(WARNING) << "Invalid route id passed in:" << route_id;
+    return NULL;
+  }
+  return iter->second;
+}
+
 bool PluginChannelBase::Init(MessageLoop* ipc_message_loop,
                              bool create_pipe_now) {
   channel_.reset(new IPC::SyncChannel(
@@ -154,9 +163,9 @@ void PluginChannelBase::OnChannelConnected(int32 peer_pid) {
 
 void PluginChannelBase::AddRoute(int route_id,
                                  IPC::Channel::Listener* listener,
-                                 bool npobject) {
+                                 NPObjectBase* npobject) {
   if (npobject) {
-    npobject_listeners_[route_id] = listener;
+    npobject_listeners_[route_id] = npobject;
   } else {
     plugin_count_++;
   }
@@ -190,8 +199,12 @@ void PluginChannelBase::RemoveRoute(int route_id) {
     AutoReset auto_reset_in_remove_route(&in_remove_route_, true);
     for (ListenerMap::iterator npobj_iter = npobject_listeners_.begin();
          npobj_iter != npobject_listeners_.end(); ++npobj_iter) {
-      if (npobj_iter->second)
-        npobj_iter->second->OnChannelError();
+      if (npobj_iter->second) {
+        IPC::Channel::Listener* channel_listener =
+            npobj_iter->second->GetChannelListener();
+        DCHECK(channel_listener != NULL);
+        channel_listener->OnChannelError();
+      }
     }
 
     for (PluginChannelMap::iterator iter = g_plugin_channels_.begin();
diff --git a/chrome/plugin/plugin_channel_base.h b/chrome/plugin/plugin_channel_base.h
index f4aab85..455ca12 100644
--- a/chrome/plugin/plugin_channel_base.h
+++ b/chrome/plugin/plugin_channel_base.h
@@ -14,6 +14,7 @@
 #include "base/ref_counted.h"
 #include "base/scoped_ptr.h"
 #include "chrome/common/message_router.h"
+#include "chrome/plugin/npobject_base.h"
 #include "ipc/ipc_sync_channel.h"
 
 // Encapsulates an IPC channel between a renderer and a plugin process.
@@ -28,7 +29,8 @@ class PluginChannelBase : public IPC::Channel::Listener,
   // lifetime of this object (by passing true for npobject) because we don't
   // want a leak of an NPObject in a plugin to keep the channel around longer
   // than necessary.
-  void AddRoute(int route_id, IPC::Channel::Listener* listener, bool npobject);
+  void AddRoute(int route_id, IPC::Channel::Listener* listener,
+                NPObjectBase* npobject);
   void RemoveRoute(int route_id);
 
   // IPC::Message::Sender implementation:
@@ -55,6 +57,10 @@ class PluginChannelBase : public IPC::Channel::Listener,
 
   static void CleanupChannels();
 
+  // Returns the NPObjectBase object for the route id passed in.
+  // Returns NULL on failure.
+  NPObjectBase* GetNPObjectListenerForRoute(int route_id);
+
  protected:
   typedef PluginChannelBase* (*PluginChannelFactory)();
 
@@ -107,7 +113,7 @@ class PluginChannelBase : public IPC::Channel::Listener,
 
   // Keep track of all the registered NPObjects proxies/stubs so that when the
   // channel is closed we can inform them.
-  typedef base::hash_map<int, IPC::Channel::Listener*> ListenerMap;
+  typedef base::hash_map<int, NPObjectBase*> ListenerMap;
   ListenerMap npobject_listeners_;
 
   // Used to implement message routing functionality to WebPlugin[Delegate]
diff --git a/chrome/plugin/webplugin_proxy.cc b/chrome/plugin/webplugin_proxy.cc
index 8142185..34091e9 100644
--- a/chrome/plugin/webplugin_proxy.cc
+++ b/chrome/plugin/webplugin_proxy.cc
@@ -151,14 +151,14 @@ NPObject* WebPluginProxy::GetWindowScriptNPObject() {
 
   int npobject_route_id = channel_->GenerateRouteID();
   bool success = false;
-  intptr_t npobject_ptr;
+  intptr_t npobject_ptr = NULL;
   Send(new PluginHostMsg_GetWindowScriptNPObject(
       route_id_, npobject_route_id, &success, &npobject_ptr));
   if (!success)
     return NULL;
 
   window_npobject_ = NPObjectProxy::Create(
-      channel_, npobject_route_id, npobject_ptr, containing_window_, page_url_);
+      channel_, npobject_route_id, containing_window_, page_url_);
 
   return window_npobject_;
 }
@@ -169,14 +169,14 @@ NPObject* WebPluginProxy::GetPluginElement() {
 
   int npobject_route_id = channel_->GenerateRouteID();
   bool success = false;
-  intptr_t npobject_ptr;
+  intptr_t npobject_ptr = NULL;
   Send(new PluginHostMsg_GetPluginElement(
       route_id_, npobject_route_id, &success, &npobject_ptr));
   if (!success)
     return NULL;
 
   plugin_element_ = NPObjectProxy::Create(
-      channel_, npobject_route_id, npobject_ptr, containing_window_, page_url_);
+      channel_, npobject_route_id, containing_window_, page_url_);
 
   return plugin_element_;
 }
@@ -321,10 +321,8 @@ bool WebPluginProxy::GetDragData(struct NPObject* event, bool add_data,
     return false;
 
   NPVariant_Param event_param;
-  event_param.type = NPVARIANT_PARAM_OBJECT_POINTER;
-  event_param.npobject_pointer = proxy->npobject_ptr();
-  if (!event_param.npobject_pointer)
-    return false;
+  event_param.type = NPVARIANT_PARAM_RECEIVER_OBJECT_ROUTING_ID;
+  event_param.npobject_routing_id = proxy->route_id();
 
   std::vector<NPVariant_Param> values;
   bool success = false;
@@ -353,10 +351,8 @@ bool WebPluginProxy::SetDropEffect(struct NPObject* event, int effect) {
     return false;
 
   NPVariant_Param event_param;
-  event_param.type = NPVARIANT_PARAM_OBJECT_POINTER;
-  event_param.npobject_pointer = proxy->npobject_ptr();
-  if (!event_param.npobject_pointer)
-    return false;
+  event_param.type = NPVARIANT_PARAM_RECEIVER_OBJECT_ROUTING_ID;
+  event_param.npobject_routing_id = proxy->route_id();
 
   bool success = false;
   Send(new PluginHostMsg_SetDropEffect(route_id_, event_param, effect,