Extensions: Some IPC validation and cleanup for messaging code.

This CL:
- Adds some browser validation for the ExtensionHostMsg_CloseMessagePort
IPC.
- Removes some un-needed code in ExtensionMessagePort::ClosePort.
- Adds some code improvements to PortContext.

Bug: 1227812

Change-Id: I00bb1899b36dadffc9556fd553398f646eb8cacd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3004140
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#900168}

extensions/browser/api/messaging/channel_endpoint.cc

@@ -19,17 +19,15 @@ ChannelEndpoint::ChannelEndpoint(content::BrowserContext* browser_context,
       render_process_id_(render_process_id),
       port_context_(port_context) {
   // Context must be exclusive to render frame or worker.
-  DCHECK_NE(port_context.is_for_service_worker(),
-            port_context.is_for_render_frame());
+  DCHECK(port_context.is_for_service_worker() ^
+         port_context.is_for_render_frame());
 }
 
 // For native message endpoint.
 ChannelEndpoint::ChannelEndpoint(content::BrowserContext* browser_context)
     : browser_context_(browser_context),
-      render_process_id_(content::ChildProcessHost::kInvalidUniqueID) {
-  DCHECK(!port_context_.is_for_render_frame() &&
-         !port_context_.is_for_service_worker());
-}
+      render_process_id_(content::ChildProcessHost::kInvalidUniqueID),
+      port_context_(PortContext::ForNativeHost()) {}
 
 bool ChannelEndpoint::is_for_service_worker() const {
   return port_context_.is_for_service_worker();

extensions/browser/api/messaging/channel_endpoint.h

@@ -17,12 +17,12 @@ class RenderFrameHost;
 namespace extensions {
 
 // Represents an endpoint (tab, frame or worker) of a message channel in a
-// render process.
+// render process or a native messaging host.
 // TODO(crbug.com/939594): Consolidate all classes/structs around extension
 // message ports.
 class ChannelEndpoint {
  public:
-  // An endpoint for a PortContext.
+  // An endpoint for a renderer PortContext.
   ChannelEndpoint(content::BrowserContext* browser_context,
                   int render_process_id,
                   const PortContext& port_context);

extensions/browser/api/messaging/extension_message_port.cc

@@ -390,22 +390,13 @@ void ExtensionMessagePort::ClosePort(int process_id,
                                      int routing_id,
                                      int worker_thread_id) {
   const bool is_for_service_worker = worker_thread_id != kMainThreadId;
-  if (!is_for_service_worker && routing_id == MSG_ROUTING_NONE) {
-    // The only non-frame-specific message is the response to an unhandled
-    // onConnect event in the extension process.
-    DCHECK(for_all_extension_contexts_);
-    ClearFrames();
-    if (!HasReceivers())
-      CloseChannel();
-    return;
-  }
+  DCHECK(is_for_service_worker || routing_id != MSG_ROUTING_NONE);
 
   if (is_for_service_worker) {
     UnregisterWorker(process_id, worker_thread_id);
-  } else {
-    DCHECK_NE(MSG_ROUTING_NONE, routing_id);
-    if (auto* rfh = content::RenderFrameHost::FromID(process_id, routing_id))
-      UnregisterFrame(rfh);
+  } else if (auto* rfh =
+                 content::RenderFrameHost::FromID(process_id, routing_id)) {
+    UnregisterFrame(rfh);
   }
 }
 

extensions/browser/api/messaging/extension_message_port.h

@@ -49,7 +49,7 @@ class ExtensionMessagePort : public MessagePort {
                        bool include_child_frames);
 
   // Create a port that is tied to all frames and service workers of an
-  // extension.
+  // extension. Should only be used for a receiver port.
   static std::unique_ptr<ExtensionMessagePort> CreateForExtension(
       base::WeakPtr<ChannelDelegate> channel_delegate,
       const PortId& port_id,

extensions/browser/bad_message.h

@@ -41,6 +41,7 @@ enum BadMessageReason {
   EMF_INVALID_EXTENSION_ID_FOR_EXTENSION_SOURCE = 15,
   EMF_INVALID_EXTENSION_ID_FOR_CONTENT_SCRIPT = 16,
   EMF_INVALID_EXTENSION_ID_FOR_WORKER_CONTEXT = 17,
+  EMF_INVALID_PORT_CONTEXT = 18,
   // Please add new elements here. The naming convention is abbreviated class
   // name (e.g. ExtensionHost becomes EH) plus a unique description of the
   // reason. After making changes, you MUST update histograms.xml by running:

extensions/browser/extension_message_filter.cc

@@ -373,6 +373,14 @@ void ExtensionMessageFilter::OnCloseMessagePort(const PortContext& port_context,
   if (!browser_context_)
     return;
 
+  // Note, we need to add more stringent IPC validation here.
+  if (!port_context.is_for_render_frame() &&
+      !port_context.is_for_service_worker()) {
+    bad_message::ReceivedBadMessage(render_process_id_,
+                                    bad_message::EMF_INVALID_PORT_CONTEXT);
+    return;
+  }
+
   MessageService::Get(browser_context_)
       ->ClosePort(port_id, render_process_id_, port_context, force_close);
 }

extensions/common/api/messaging/port_context.cc

@@ -6,23 +6,21 @@
 
 namespace extensions {
 
+PortContext::PortContext() = default;
+PortContext::~PortContext() = default;
+PortContext::PortContext(const PortContext& other) = default;
+
+PortContext::FrameContext::FrameContext(int routing_id)
+    : routing_id(routing_id) {}
+PortContext::FrameContext::FrameContext() = default;
+
 PortContext::WorkerContext::WorkerContext(int thread_id,
                                           int64_t version_id,
                                           const std::string& extension_id)
     : thread_id(thread_id),
       version_id(version_id),
       extension_id(extension_id) {}
-
-PortContext::FrameContext::FrameContext(int routing_id)
-    : routing_id(routing_id) {}
-
 PortContext::WorkerContext::WorkerContext() = default;
-PortContext::FrameContext::FrameContext() = default;
-
-PortContext::PortContext() = default;
-PortContext::~PortContext() = default;
-
-PortContext::PortContext(const PortContext& other) = default;
 
 PortContext PortContext::ForFrame(int routing_id) {
   PortContext context;
@@ -38,4 +36,8 @@ PortContext PortContext::ForWorker(int thread_id,
   return context;
 }
 
+PortContext PortContext::ForNativeHost() {
+  return PortContext();
+}
+
 }  // namespace extensions

extensions/common/api/messaging/port_context.h

@@ -13,12 +13,14 @@
 
 namespace extensions {
 
-// Specifies the renderer context that is tied to a message Port.
 // A port can refer to a RenderFrame (FrameContext) or a Service Worker
-// (WorkerContext).
+// (WorkerContext) or a native messaging host.
 struct PortContext {
  public:
+  // This constructor is needed by our IPC code and tests. Clients should use
+  // factory functions instead.
   PortContext();
+
   ~PortContext();
   PortContext(const PortContext& other);
 
@@ -47,9 +49,11 @@ struct PortContext {
   static PortContext ForWorker(int thread_id,
                                int64_t version_id,
                                const std::string& extension_id);
+  static PortContext ForNativeHost();
 
   bool is_for_render_frame() const { return frame.has_value(); }
   bool is_for_service_worker() const { return worker.has_value(); }
+  bool is_for_native_host() const { return !frame && !worker; }
 
   absl::optional<FrameContext> frame;
   absl::optional<WorkerContext> worker;

tools/metrics/histograms/enums.xml

@@ -7506,6 +7506,7 @@ Called by update_bad_message_reasons.py.-->
   <int value="15" label="EMF_INVALID_EXTENSION_ID_FOR_EXTENSION_SOURCE"/>
   <int value="16" label="EMF_INVALID_EXTENSION_ID_FOR_CONTENT_SCRIPT"/>
   <int value="17" label="EMF_INVALID_EXTENSION_ID_FOR_WORKER_CONTEXT"/>
+  <int value="18" label="EMF_INVALID_PORT_CONTEXT"/>
 </enum>
 
 <enum name="BadMessageReasonGuestView">