Upload crash reports when ExtensionFunctions generate bad messages.

extensions/browser/extension_function_dispatcher.cc

Index: extensions/browser/extension_function_dispatcher.cc
diff --git a/extensions/browser/extension_function_dispatcher.cc b/extensions/browser/extension_function_dispatcher.cc
index 93a8b24c3b660e06693e848ba34715164f215b49..7bb79bdd625c16061edff7ce141b9314c0df18e6 100644
--- a/extensions/browser/extension_function_dispatcher.cc
+++ b/extensions/browser/extension_function_dispatcher.cc
@@ -31,6 +31,7 @@
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/common/result_codes.h"
 #include "extensions/browser/api_activity_monitor.h"
+#include "extensions/browser/bad_message.h"
 #include "extensions/browser/extension_function_registry.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
@@ -79,43 +80,19 @@ struct Static {
 };
 base::LazyInstance<Static> g_global_io_data = LAZY_INSTANCE_INITIALIZER;
 
-// Kills the specified process because it sends us a malformed message.
-// Track the specific function's |histogram_value|, as this may indicate a bug
-// in that API's implementation on the renderer.
-void KillBadMessageSender(const base::Process& process,
-                          functions::HistogramValue histogram_value) {
-  // The renderer has done validation before sending extension api requests.
-  // Therefore, we should never receive a request that is invalid in a way
-  // that JSON validation in the renderer should have caught. It could be an
-  // attacker trying to exploit the browser, so we crash the renderer instead.
-  LOG(ERROR) << "Terminating renderer because of malformed extension message.";
-  if (content::RenderProcessHost::run_renderer_in_process()) {
-    // In single process mode it is better if we don't suicide but just crash.
-    CHECK(false);
-    return;
-  }
-
-  NOTREACHED();
+void LogBadMessage(functions::HistogramValue histogram_value) {
   content::RecordAction(base::UserMetricsAction("BadMessageTerminate_EFD"));
+  // Track the specific function's |histogram_value|, as this may indicate a
+  // bug in that API's implementation on the renderer.

[Devlin, 2017/01/13 02:45:50]

s/on the renderer/.  We flag something as a bad message on the browser, so that
could be the result of a bad message from the renderer or a bad indication on
the browser (e.g. liberal use of EXTENSION_FUNCTION_VALIDATE).

[lazyboy, 2017/01/13 03:04:53]

Done.

   UMA_HISTOGRAM_ENUMERATION("Extensions.BadMessageFunctionName",
                             histogram_value, functions::ENUM_BOUNDARY);
-  if (process.IsValid())
-    process.Terminate(content::RESULT_CODE_KILLED_BAD_MESSAGE, false);
 }
 
-void KillBadMessageSenderRPH(content::RenderProcessHost* sender_process_host,
-                             functions::HistogramValue histogram_value) {
-  base::Process peer_process =
-      content::RenderProcessHost::run_renderer_in_process()
-          ? base::Process::Current()
-          : base::Process::DeprecatedGetProcessFromHandle(
-                sender_process_host->GetHandle());
-  KillBadMessageSender(peer_process, histogram_value);
-}
+using BadMessageReceivedFunc = base::Callback<void(functions::HistogramValue)>;
 
 void CommonResponseCallback(IPC::Sender* ipc_sender,

[Devlin, 2017/01/13 02:45:50]

instead of BadMessageReceivedFunc, could we just have something like
template <class T>
CommonResponseCallback(..., T sender) {
  ...
  if (bad message) {
    LogBadMessage(histogram_value);
    bad_message::ReceivedBadMessage(sender, EFD_BAD_MESSAGE);
  }
  ...
}
?

[lazyboy, 2017/01/13 03:04:53]

Ty, Done.

                             int routing_id,
-                            const base::Process& peer_process,
+                            const BadMessageReceivedFunc& bad_message_func,
                             int request_id,
                             ExtensionFunction::ResponseType type,
                             const base::ListValue& results,
@@ -124,7 +101,7 @@ void CommonResponseCallback(IPC::Sender* ipc_sender,
   DCHECK(ipc_sender);
 
   if (type == ExtensionFunction::BAD_MESSAGE) {
-    KillBadMessageSender(peer_process, histogram_value);
+    bad_message_func.Run(histogram_value);
     return;
   }
 
@@ -133,6 +110,18 @@ void CommonResponseCallback(IPC::Sender* ipc_sender,
       error));
 }
 
+void SendBadMessageToFilter(content::BrowserMessageFilter* filter,
+                            functions::HistogramValue histogram_value) {
+  LogBadMessage(histogram_value);
+  bad_message::ReceivedBadMessage(filter, bad_message::EFD_BAD_MESSAGE);
+}
+
+void SendBadMessageToProcess(content::RenderProcessHost* rph,
+                             functions::HistogramValue histogram_value) {
+  LogBadMessage(histogram_value);
+  bad_message::ReceivedBadMessage(rph, bad_message::EFD_BAD_MESSAGE);
+}
+
 void IOThreadResponseCallback(
     const base::WeakPtr<IOThreadExtensionMessageFilter>& ipc_sender,
     int routing_id,
@@ -144,10 +133,13 @@ void IOThreadResponseCallback(
   if (!ipc_sender.get())
     return;
 
-  base::Process peer_process =
-      base::Process::DeprecatedGetProcessFromHandle(ipc_sender->PeerHandle());
-  CommonResponseCallback(ipc_sender.get(), routing_id, peer_process, request_id,
-                         type, results, error, histogram_value);
+  // Extract the pointer |filter_ptr| to make the base::Bind call below happy.
+  IOThreadExtensionMessageFilter* filter_ptr = ipc_sender.get();
+  BadMessageReceivedFunc bad_message_received_func =
+      base::Bind(&SendBadMessageToFilter, filter_ptr);
+  CommonResponseCallback(ipc_sender.get(), routing_id,
+                         bad_message_received_func, request_id, type, results,
+                         error, histogram_value);
 }
 
 }  // namespace
@@ -195,15 +187,12 @@ class ExtensionFunctionDispatcher::UIThreadResponseCallbackWrapper
                                     const base::ListValue& results,
                                     const std::string& error,
                                     functions::HistogramValue histogram_value) {
-    base::Process process =
-        content::RenderProcessHost::run_renderer_in_process()
-            ? base::Process::Current()
-            : base::Process::DeprecatedGetProcessFromHandle(
-                  render_frame_host_->GetProcess()->GetHandle());
+    BadMessageReceivedFunc bad_message_received_func =
+        base::Bind(&SendBadMessageToProcess, render_frame_host_->GetProcess());
     CommonResponseCallback(render_frame_host_,
                            render_frame_host_->GetRoutingID(),
-                           process, request_id, type, results, error,
-                           histogram_value);
+                           bad_message_received_func, request_id, type, results,
+                           error, histogram_value);
   }
 
   base::WeakPtr<ExtensionFunctionDispatcher> dispatcher_;
@@ -266,7 +255,7 @@ class ExtensionFunctionDispatcher::UIThreadWorkerResponseCallbackWrapper
     content::RenderProcessHost* sender =
         content::RenderProcessHost::FromID(render_process_id_);
     if (type == ExtensionFunction::BAD_MESSAGE) {
-      KillBadMessageSenderRPH(sender, histogram_value);
+      SendBadMessageToProcess(sender, histogram_value);
       return;
     }
     DCHECK(sender);