Avoid UAF on ConnectionFilter impls

An incoming service connection may outlive any ConnectionFilter
which added interfaces to it, so a ConnectionFilter impl must not
register any interface binders which hold unsafe references to
itself.

This fixes cases where that was being done.

BUG=639650
TBR=ben@chromium.org

Review-Url: https://codereview.chromium.org/2268603002
Cr-Commit-Position: refs/heads/master@{#413462}

content/child/child_thread_impl.cc

@@ -257,12 +257,10 @@ void InitializeMojoIPCChannel() {
   mojo::edk::SetParentPipeHandle(std::move(platform_channel));
 }
 
-class ChannelBootstrapFilter
-    : public ConnectionFilter,
-      public shell::InterfaceFactory<IPC::mojom::ChannelBootstrap> {
+class ChannelBootstrapFilter : public ConnectionFilter {
  public:
   explicit ChannelBootstrapFilter(IPC::mojom::ChannelBootstrapPtrInfo bootstrap)
-      : bootstrap_(std::move(bootstrap)) {}
+      : bootstrap_(std::move(bootstrap)), weak_factory_(this) {}
 
  private:
   // ConnectionFilter:
@@ -272,18 +270,18 @@ class ChannelBootstrapFilter
     if (remote_identity.name() != kBrowserMojoApplicationName)
       return false;
 
-    registry->AddInterface<IPC::mojom::ChannelBootstrap>(this);
+    registry->AddInterface(base::Bind(&ChannelBootstrapFilter::CreateBootstrap,
+                                      weak_factory_.GetWeakPtr()));
     return true;
   }
 
-  // shell::InterfaceFactory<IPC::mojom::ChannelBootstrap>:
-  void Create(const shell::Identity& remote_identity,
-              IPC::mojom::ChannelBootstrapRequest request) override {
+  void CreateBootstrap(IPC::mojom::ChannelBootstrapRequest request) {
     DCHECK(bootstrap_.is_valid());
     mojo::FuseInterface(std::move(request), std::move(bootstrap_));
   }
 
   IPC::mojom::ChannelBootstrapPtrInfo bootstrap_;
+  base::WeakPtrFactory<ChannelBootstrapFilter> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ChannelBootstrapFilter);
 };

content/public/common/connection_filter.h

@@ -32,6 +32,9 @@ class CONTENT_EXPORT ConnectionFilter {
   //
   // If a ConnectionFilter is not interested in an incoming connection, it
   // should return |false|.
+  //
+  // NOTE: This ConnectionFilter is NOT guaranteed to outlive |registry|, so you
+  // must not attach unsafe references to |this|, e.g., via AddInterface().
   virtual bool OnConnect(const shell::Identity& remote_identity,
                          shell::InterfaceRegistry* registry,
                          shell::Connector* connector) = 0;

content/renderer/mus/render_widget_window_tree_client_factory.cc

@@ -10,6 +10,7 @@
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
 #include "base/sequenced_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "content/common/render_widget_window_tree_client_factory.mojom.h"
@@ -38,11 +39,9 @@ void BindMusConnectionOnMainThread(
 // registered with it.
 class RenderWidgetWindowTreeClientFactoryImpl
     : public ConnectionFilter,
-      public shell::InterfaceFactory<
-          mojom::RenderWidgetWindowTreeClientFactory>,
       public mojom::RenderWidgetWindowTreeClientFactory {
  public:
-  RenderWidgetWindowTreeClientFactoryImpl() {
+  RenderWidgetWindowTreeClientFactoryImpl() : weak_factory_(this) {
     main_thread_task_runner_ = base::ThreadTaskRunnerHandle::Get();
   }
 
@@ -53,17 +52,12 @@ class RenderWidgetWindowTreeClientFactoryImpl
   bool OnConnect(const shell::Identity& remote_identity,
                  shell::InterfaceRegistry* registry,
                  shell::Connector* connector) override {
-    registry->AddInterface<mojom::RenderWidgetWindowTreeClientFactory>(this);
+    registry->AddInterface(
+        base::Bind(&RenderWidgetWindowTreeClientFactoryImpl::CreateFactory,
+                   weak_factory_.GetWeakPtr()));
     return true;
   }
 
-  // shell::InterfaceFactory<mojom::RenderWidgetWindowTreeClientFactory>:
-  void Create(const shell::Identity& remote_identity,
-              mojo::InterfaceRequest<mojom::RenderWidgetWindowTreeClientFactory>
-                  request) override {
-    bindings_.AddBinding(this, std::move(request));
-  }
-
   // mojom::RenderWidgetWindowTreeClientFactory implementation.
   void CreateWindowTreeClientForRenderWidget(
       uint32_t routing_id,
@@ -73,8 +67,14 @@ class RenderWidgetWindowTreeClientFactoryImpl
                               base::Passed(&request)));
   }
 
+  void CreateFactory(
+      mojom::RenderWidgetWindowTreeClientFactoryRequest request) {
+    bindings_.AddBinding(this, std::move(request));
+  }
+
   scoped_refptr<base::SequencedTaskRunner> main_thread_task_runner_;
   mojo::BindingSet<mojom::RenderWidgetWindowTreeClientFactory> bindings_;
+  base::WeakPtrFactory<RenderWidgetWindowTreeClientFactoryImpl> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(RenderWidgetWindowTreeClientFactoryImpl);
 };

services/navigation/navigation.cc

@@ -29,7 +29,8 @@ void CreateViewOnViewTaskRunner(
 
 Navigation::Navigation()
     : view_task_runner_(base::ThreadTaskRunnerHandle::Get()),
-      ref_factory_(base::MessageLoop::QuitWhenIdleClosure()) {
+      ref_factory_(base::MessageLoop::QuitWhenIdleClosure()),
+      weak_factory_(this) {
   bindings_.set_connection_error_handler(
       base::Bind(&Navigation::ViewFactoryLost, base::Unretained(this)));
 }
@@ -46,16 +47,11 @@ bool Navigation::OnConnect(const shell::Identity& remote_identity,
   }
   client_user_id_ = remote_user_id;
 
-  registry->AddInterface<mojom::ViewFactory>(this);
+  registry->AddInterface(
+      base::Bind(&Navigation::CreateViewFactory, weak_factory_.GetWeakPtr()));
   return true;
 }
 
-void Navigation::Create(const shell::Identity& remote_identity,
-                        mojom::ViewFactoryRequest request) {
-  bindings_.AddBinding(this, std::move(request));
-  refs_.insert(ref_factory_.CreateRef());
-}
-
 void Navigation::CreateView(mojom::ViewClientPtr client,
                             mojom::ViewRequest request) {
   std::unique_ptr<shell::Connector> new_connector = connector_->Clone();
@@ -68,6 +64,11 @@ void Navigation::CreateView(mojom::ViewClientPtr client,
                  base::Passed(&context_ref)));
 }
 
+void Navigation::CreateViewFactory(mojom::ViewFactoryRequest request) {
+  bindings_.AddBinding(this, std::move(request));
+  refs_.insert(ref_factory_.CreateRef());
+}
+
 void Navigation::ViewFactoryLost() {
   refs_.erase(refs_.begin());
 }

services/navigation/navigation.h

@@ -6,6 +6,7 @@
 #define SERVICES_NAVIGATION_NAVIGATION_H_
 
 #include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
 #include "base/sequenced_task_runner.h"
 #include "content/public/common/connection_filter.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
@@ -20,9 +21,7 @@ class BrowserContext;
 
 namespace navigation {
 
-class Navigation : public content::ConnectionFilter,
-                   public shell::InterfaceFactory<mojom::ViewFactory>,
-                   public mojom::ViewFactory {
+class Navigation : public content::ConnectionFilter, public mojom::ViewFactory {
  public:
   Navigation();
   ~Navigation() override;
@@ -33,14 +32,11 @@ class Navigation : public content::ConnectionFilter,
                  shell::InterfaceRegistry* registry,
                  shell::Connector* connector) override;
 
-  // shell::InterfaceFactory<mojom::ViewFactory>:
-  void Create(const shell::Identity& remote_identity,
-              mojom::ViewFactoryRequest request) override;
-
   // mojom::ViewFactory:
   void CreateView(mojom::ViewClientPtr client,
                   mojom::ViewRequest request) override;
 
+  void CreateViewFactory(mojom::ViewFactoryRequest request);
   void ViewFactoryLost();
 
   scoped_refptr<base::SequencedTaskRunner> view_task_runner_;
@@ -53,6 +49,8 @@ class Navigation : public content::ConnectionFilter,
 
   mojo::BindingSet<mojom::ViewFactory> bindings_;
 
+  base::WeakPtrFactory<Navigation> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(Navigation);
 };