inspector: allow whitelisting inspector domain

src/inspector/main_thread_interface.cc

@@ -194,8 +194,12 @@ class CrossThreadInspectorSession : public InspectorSession {
 
 class ThreadSafeDelegate : public InspectorSessionDelegate {
  public:
-  ThreadSafeDelegate(std::shared_ptr<MainThreadHandle> thread, int object_id)
-                     : thread_(thread), delegate_(thread, object_id) {}
+  ThreadSafeDelegate(std::shared_ptr<MainThreadHandle> thread,
+                     int object_id,
+                     bool is_trusted)
+      : thread_(thread),
+        delegate_(thread, object_id),
+        is_trusted_(is_trusted) {}
 
   void SendMessageToFrontend(const v8_inspector::StringView& message) override {
     delegate_.Call(
@@ -205,9 +209,14 @@ class ThreadSafeDelegate : public InspectorSessionDelegate {
     });
   }
 
+  bool IsSessionTrusted() override {
+    return is_trusted_;
+  }
+
  private:
   std::shared_ptr<MainThreadHandle> thread_;
   AnotherThreadObjectReference<InspectorSessionDelegate> delegate_;
+  bool is_trusted_;
 };
 }  // namespace
 
@@ -343,9 +352,10 @@ std::unique_ptr<InspectorSessionDelegate>
 MainThreadHandle::MakeDelegateThreadSafe(
     std::unique_ptr<InspectorSessionDelegate> delegate) {
   int id = newObjectId();
+  bool is_trusted = delegate->IsSessionTrusted();
   main_thread_->AddObject(id, WrapInDeletable(std::move(delegate)));
   return std::unique_ptr<InspectorSessionDelegate>(
-      new ThreadSafeDelegate(shared_from_this(), id));
+      new ThreadSafeDelegate(shared_from_this(), id, is_trusted));
 }
 
 bool MainThreadHandle::Expired() {

src/inspector/worker_agent.cc

@@ -12,8 +12,11 @@ class NodeWorkers
     : public std::enable_shared_from_this<NodeWorkers> {
  public:
   explicit NodeWorkers(std::weak_ptr<NodeWorker::Frontend> frontend,
-                      std::shared_ptr<MainThreadHandle> thread)
-                      : frontend_(frontend), thread_(thread) {}
+                       std::shared_ptr<MainThreadHandle> thread,
+                       bool is_session_trusted)
+                       : frontend_(frontend),
+                         thread_(thread),
+                         is_session_trusted_(is_session_trusted) {}
   void WorkerCreated(const std::string& title,
                      const std::string& url,
                      bool waiting,
@@ -22,9 +25,14 @@ class NodeWorkers
   void Send(const std::string& id, const std::string& message);
   void Detached(const std::string& id);
 
+  bool is_session_trusted() {
+    return is_session_trusted_;
+  }
+
  private:
   std::weak_ptr<NodeWorker::Frontend> frontend_;
   std::shared_ptr<MainThreadHandle> thread_;
+  bool is_session_trusted_;
   std::unordered_map<std::string, std::unique_ptr<InspectorSession>> sessions_;
   int next_target_id_ = 0;
 };
@@ -61,6 +69,10 @@ class ParentInspectorSessionDelegate : public InspectorSessionDelegate {
     workers_->Send(id_, message);
   }
 
+  bool IsSessionTrusted() override {
+    return workers_->is_session_trusted();
+  }
+
  private:
   std::string id_;
   std::shared_ptr<NodeWorkers> workers_;
@@ -77,17 +89,17 @@ std::unique_ptr<NodeWorker::WorkerInfo> WorkerInfo(const std::string& id,
 }
 }  // namespace
 
-WorkerAgent::WorkerAgent(std::weak_ptr<WorkerManager> manager)
-                         : manager_(manager) {}
-
+WorkerAgent::WorkerAgent(std::weak_ptr<WorkerManager> manager,
+                         bool is_session_trusted)
+    : manager_(manager), is_session_trusted_(is_session_trusted) {}
 
 void WorkerAgent::Wire(UberDispatcher* dispatcher) {
   frontend_.reset(new NodeWorker::Frontend(dispatcher->channel()));
   NodeWorker::Dispatcher::wire(dispatcher, this);
   auto manager = manager_.lock();
   CHECK_NOT_NULL(manager);
-  workers_ =
-      std::make_shared<NodeWorkers>(frontend_, manager->MainThread());
+  workers_ = std::make_shared<NodeWorkers>(
+      frontend_, manager->MainThread(), is_session_trusted_);
 }
 
 DispatchResponse WorkerAgent::sendMessageToWorker(const String& message,

src/inspector/worker_agent.h

@@ -15,7 +15,8 @@ class NodeWorkers;
 
 class WorkerAgent : public NodeWorker::Backend {
  public:
-  explicit WorkerAgent(std::weak_ptr<WorkerManager> manager);
+  explicit WorkerAgent(std::weak_ptr<WorkerManager> manager,
+                       bool is_session_trusted);
   ~WorkerAgent() override = default;
 
   void Wire(UberDispatcher* dispatcher);
@@ -31,6 +32,7 @@ class WorkerAgent : public NodeWorker::Backend {
   std::weak_ptr<WorkerManager> manager_;
   std::unique_ptr<WorkerManagerEventHandle> event_handle_;
   std::shared_ptr<NodeWorkers> workers_;
+  bool is_session_trusted_;
 };
 }  // namespace protocol
 }  // namespace inspector

src/inspector_agent.cc

@@ -86,6 +86,18 @@ void StartIoInterrupt(Isolate* isolate, void* agent) {
   static_cast<Agent*>(agent)->StartIoThread();
 }
 
+std::string BuildMethodNotFoundErrorJSON(
+    int call_id, const std::string& method) {
+  auto error = protocol::DictionaryValue::create();
+  std::ostringstream message;
+  error->setInteger("code", -32601);  // Inspector magic number
+  message << '\'' << method << "\' wasn't found";
+  error->setString("message", message.str());
+  const auto& response = protocol::DictionaryValue::create();
+  response->setInteger("id", call_id);
+  response->setObject("error", std::move(error));
+  return response->toJSONString();
+}
 
 #ifdef __POSIX__
 static void StartIoThreadWakeup(int signo) {
@@ -221,15 +233,19 @@ class ChannelImpl final : public v8_inspector::V8Inspector::Channel,
                        std::shared_ptr<WorkerManager> worker_manager,
                        std::unique_ptr<InspectorSessionDelegate> delegate,
                        std::shared_ptr<MainThreadHandle> main_thread_,
+                       std::unordered_set<std::string> domain_whitelist,
                        bool prevent_shutdown)
-      : delegate_(std::move(delegate)), prevent_shutdown_(prevent_shutdown),
+      : delegate_(std::move(delegate)),
+        domain_whitelist_(domain_whitelist),
+        prevent_shutdown_(prevent_shutdown),
         retaining_context_(false) {
     session_ = inspector->connect(1, this, StringView());
     node_dispatcher_ = std::make_unique<protocol::UberDispatcher>(this);
     tracing_agent_ =
         std::make_unique<protocol::TracingAgent>(env, main_thread_);
     tracing_agent_->Wire(node_dispatcher_.get());
-    worker_agent_ = std::make_unique<protocol::WorkerAgent>(worker_manager);
+    worker_agent_ = std::make_unique<protocol::WorkerAgent>(
+        worker_manager, delegate_->IsSessionTrusted());
     worker_agent_->Wire(node_dispatcher_.get());
     runtime_agent_ = std::make_unique<protocol::RuntimeAgent>(env);
     runtime_agent_->Wire(node_dispatcher_.get());
@@ -252,6 +268,10 @@ class ChannelImpl final : public v8_inspector::V8Inspector::Channel,
     int call_id;
     std::string method;
     node_dispatcher_->parseCommand(value.get(), &call_id, &method);
+    if (!methodWhitelisted(method)) {
+      sendMessageToFrontend(BuildMethodNotFoundErrorJSON(call_id, method));
+      return;
+    }
     if (v8_inspector::V8InspectorSession::canDispatchMethod(
             Utf8ToStringView(method)->string())) {
       session_->dispatchProtocolMessage(message);
@@ -318,12 +338,33 @@ class ChannelImpl final : public v8_inspector::V8Inspector::Channel,
     DCHECK(false);
   }
 
+  bool methodWhitelisted(const std::string& method) {
+    if (domain_whitelist_.empty() || delegate_->IsSessionTrusted()) {
+      return true;
+    }
+    // "Runtime" domain as a whole is unsafe. It includes capabilities like
+    // arbitrary code execution through the "Runtime.evaluate".
+    // "Runtime.runIfWaitingForDebugger" should still be allowed so that
+    // "--inspect-brk" flag could be used and the frontends were given a chance
+    // to connect and configure inspector before code is executed.
+    if (method == "Runtime.runIfWaitingForDebugger") {
+      return true;
+    }
+    const auto& domainAndMethod = SplitString(method, '.');
+    if (domainAndMethod.size() < 2) {
+      return true;  // Dispatcher will send a proper error
+    }
+    return
+        domain_whitelist_.find(domainAndMethod[0]) != domain_whitelist_.end();
+  }
+
   std::unique_ptr<protocol::RuntimeAgent> runtime_agent_;
   std::unique_ptr<protocol::TracingAgent> tracing_agent_;
   std::unique_ptr<protocol::WorkerAgent> worker_agent_;
   std::unique_ptr<InspectorSessionDelegate> delegate_;
   std::unique_ptr<v8_inspector::V8InspectorSession> session_;
   std::unique_ptr<protocol::UberDispatcher> node_dispatcher_;
+  std::unordered_set<std::string> domain_whitelist_;
   bool prevent_shutdown_;
   bool retaining_context_;
 };
@@ -448,8 +489,12 @@ bool IsFilePath(const std::string& path) {
 
 class NodeInspectorClient : public V8InspectorClient {
  public:
-  explicit NodeInspectorClient(node::Environment* env, bool is_main)
-      : env_(env), is_main_(is_main) {
+  explicit NodeInspectorClient(
+      node::Environment* env,
+      bool is_main,
+      const std::vector<std::string>& domains_whitelist)
+      : env_(env), is_main_(is_main),
+        domains_whitelist_(domains_whitelist.begin(), domains_whitelist.end()) {
     client_ = V8Inspector::create(env->isolate(), this);
     // TODO(bnoordhuis) Make name configurable from src/node.cc.
     std::string name =
@@ -523,6 +568,7 @@ class NodeInspectorClient : public V8InspectorClient {
                                                           getWorkerManager(),
                                                           std::move(delegate),
                                                           getThreadHandle(),
+                                                          domains_whitelist_,
                                                           prevent_shutdown);
     return session_id;
   }
@@ -719,6 +765,7 @@ class NodeInspectorClient : public V8InspectorClient {
   // Allows accessing Inspector from non-main threads
   std::unique_ptr<MainThreadInterface> interface_;
   std::shared_ptr<WorkerManager> worker_manager_;
+  std::unordered_set<std::string> domains_whitelist_;
 };
 
 Agent::Agent(Environment* env)
@@ -744,7 +791,8 @@ bool Agent::Start(const std::string& path,
   CHECK_NOT_NULL(host_port);
   host_port_ = host_port;
 
-  client_ = std::make_shared<NodeInspectorClient>(parent_env_, is_main);
+  client_ = std::make_shared<NodeInspectorClient>(
+      parent_env_, is_main, SplitString(options.domain_whitelist, ','));
   if (parent_env_->owns_inspector()) {
     CHECK_EQ(start_io_thread_async_initialized.exchange(true), false);
     CHECK_EQ(0, uv_async_init(parent_env_->event_loop(),
@@ -971,8 +1019,5 @@ void SameThreadInspectorSession::Dispatch(
   if (client)
     client->dispatchMessageFromFrontend(session_id_, message);
 }
-
-
-
 }  // namespace inspector
 }  // namespace node

src/inspector_agent.h

@@ -38,6 +38,7 @@ class InspectorSessionDelegate {
   virtual ~InspectorSessionDelegate() = default;
   virtual void SendMessageToFrontend(const v8_inspector::StringView& message)
                                      = 0;
+  virtual bool IsSessionTrusted() = 0;
 };
 
 class Agent {

src/inspector_io.cc

@@ -203,6 +203,10 @@ class IoSessionDelegate : public InspectorSessionDelegate {
                          StringBuffer::create(message));
   }
 
+  bool IsSessionTrusted() override {
+    return false;
+  }
+
  private:
   std::shared_ptr<RequestQueue> request_queue_;
   int id_;

src/inspector_js_api.cc

@@ -58,6 +58,10 @@ class JSBindingsConnection : public AsyncWrap {
       connection_->OnMessage(argument);
     }
 
+    bool IsSessionTrusted() override {
+      return true;
+    }
+
    private:
     Environment* env_;
     JSBindingsConnection* connection_;

src/inspector_profiler.h

@@ -26,6 +26,10 @@ class V8ProfilerConnection {
     void SendMessageToFrontend(
         const v8_inspector::StringView& message) override;
 
+    bool IsSessionTrusted() override {
+      return true;
+    }
+
    private:
     V8ProfilerConnection* connection_;
   };

src/node_options.cc

@@ -263,6 +263,10 @@ DebugOptionsParser::DebugOptionsParser() {
   AddOption("--debug-brk", "", &DebugOptions::break_first_line);
   Implies("--debug-brk", "--debug");
   AddAlias("--debug-brk=", { "--inspect-port", "--debug-brk" });
+
+  AddOption("--inspector-domain-whitelist",
+            "Comma-separated list of enabled Inspector protocol domains",
+            &DebugOptions::domain_whitelist);
 }
 
 EnvironmentOptionsParser::EnvironmentOptionsParser() {

src/node_options.h

@@ -70,6 +70,8 @@ class DebugOptions : public Options {
   bool break_first_line = false;
   // --inspect-brk-node
   bool break_node_first_line = false;
+  // --inspector-domain-whitelist
+  std::string domain_whitelist;
 
   enum { kDefaultInspectorPort = 9229 };
 

test/parallel/test-inspector-domain-whitelist.js

@@ -0,0 +1,40 @@
+// Flags: --expose-internals
+'use strict';
+const common = require('../common');
+
+common.skipIfInspectorDisabled();
+
+const assert = require('assert');
+const { NodeInstance } = require('../common/inspector-helper.js');
+const script = `
+const session = new (require('inspector').Session)();
+session.connect();
+session.post('Debugger.enable', (error, res) => {
+  if (!error) {
+    process.exit(42);
+  }
+});`;
+
+/**
+ * This test verifies that it is possible to disable built-in V8 inpsector
+ * domains by providing a domains whitelist from the command line.
+ * Disabled domains are still available for "trusted" sessions (ones that are
+ * not accepting arbitrary commands from external clients)
+ */
+async function runTest() {
+  const child = new NodeInstance([
+    '--inspect-brk=0', '--inspector-domain-whitelist=Runtime', '-e', script
+  ]);
+  const session = await child.connectInspectorSession();
+  await session.send({ method: 'Runtime.runIfWaitingForDebugger' });
+  try {
+    const response = await session.send({ method: 'Debugger.enable' });
+    assert.fail(JSON.stringify(response));
+  } catch (e) {
+    console.log(`Expected error ${JSON.stringify(e)} was recieved`);
+  }
+  await session.disconnect();
+  assert.strictEqual((await child.expectShutdown()).exitCode, 42);
+}
+
+runTest();