From 83e6f8bf32b00f060d59e77fb5a876c08c6b3e30 Mon Sep 17 00:00:00 2001 From: Eugene Ostroukhov Date: Wed, 15 May 2019 17:01:38 -0700 Subject: [PATCH 1/3] inspector: allow whitelisting inspector domain This change introduces an "--inspector-domain-whitelist" that can be used to provide a comma-separated list of enabled Inspector domains. This will allow to limit exposure of the Node instances to applications that may connect to the inspector socket. E.g. this allows hiding unsafe methods like "Runtime.evaluate" that allow executing arbitrary code. 1. Runtime.runIfWaitingForDebugger will always be available. This is essential for tools that rely on --inspect-brk flag to connect before the code execution starts. 2. All domains are still available through the Inspector JS APIs. --- src/inspector/main_thread_interface.cc | 15 +++- src/inspector/worker_agent.cc | 26 ++++-- src/inspector/worker_agent.h | 4 +- src/inspector_agent.cc | 80 ++++++++++++++++--- src/inspector_agent.h | 1 + src/inspector_io.cc | 4 + src/inspector_js_api.cc | 4 + src/inspector_profiler.h | 4 + src/node_options.cc | 4 + src/node_options.h | 2 + .../test-inspector-domain-whitelist.js | 40 ++++++++++ 11 files changed, 164 insertions(+), 20 deletions(-) create mode 100644 test/parallel/test-inspector-domain-whitelist.js diff --git a/src/inspector/main_thread_interface.cc b/src/inspector/main_thread_interface.cc index ac4461baed4afd..ff92927201b256 100644 --- a/src/inspector/main_thread_interface.cc +++ b/src/inspector/main_thread_interface.cc @@ -194,8 +194,11 @@ class CrossThreadInspectorSession : public InspectorSession { class ThreadSafeDelegate : public InspectorSessionDelegate { public: - ThreadSafeDelegate(std::shared_ptr thread, int object_id) - : thread_(thread), delegate_(thread, object_id) {} + ThreadSafeDelegate(std::shared_ptr thread, + int object_id, + int is_trusted) + : thread_(thread), delegate_(thread, object_id), + is_trusted_(is_trusted) {} void SendMessageToFrontend(const v8_inspector::StringView& message) override { delegate_.Call( @@ -205,9 +208,14 @@ class ThreadSafeDelegate : public InspectorSessionDelegate { }); } + bool IsSessionTrusted() override { + return is_trusted_; + } + private: std::shared_ptr thread_; AnotherThreadObjectReference delegate_; + bool is_trusted_; }; } // namespace @@ -343,9 +351,10 @@ std::unique_ptr MainThreadHandle::MakeDelegateThreadSafe( std::unique_ptr delegate) { int id = newObjectId(); + bool is_trusted = delegate->IsSessionTrusted(); main_thread_->AddObject(id, WrapInDeletable(std::move(delegate))); return std::unique_ptr( - new ThreadSafeDelegate(shared_from_this(), id)); + new ThreadSafeDelegate(shared_from_this(), id, is_trusted)); } bool MainThreadHandle::Expired() { diff --git a/src/inspector/worker_agent.cc b/src/inspector/worker_agent.cc index d343de8494a36f..fd5eb1cb0d99a7 100644 --- a/src/inspector/worker_agent.cc +++ b/src/inspector/worker_agent.cc @@ -12,8 +12,11 @@ class NodeWorkers : public std::enable_shared_from_this { public: explicit NodeWorkers(std::weak_ptr frontend, - std::shared_ptr thread) - : frontend_(frontend), thread_(thread) {} + std::shared_ptr 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 frontend_; std::shared_ptr thread_; + bool is_session_trusted_; std::unordered_map> 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 workers_; @@ -77,17 +89,17 @@ std::unique_ptr WorkerInfo(const std::string& id, } } // namespace -WorkerAgent::WorkerAgent(std::weak_ptr manager) - : manager_(manager) {} - +WorkerAgent::WorkerAgent(std::weak_ptr 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(frontend_, manager->MainThread()); + workers_ = std::make_shared( + frontend_, manager->MainThread(), is_session_trusted_); } DispatchResponse WorkerAgent::sendMessageToWorker(const String& message, diff --git a/src/inspector/worker_agent.h b/src/inspector/worker_agent.h index 402c7194163967..89d5e514250c6c 100644 --- a/src/inspector/worker_agent.h +++ b/src/inspector/worker_agent.h @@ -15,7 +15,8 @@ class NodeWorkers; class WorkerAgent : public NodeWorker::Backend { public: - explicit WorkerAgent(std::weak_ptr manager); + explicit WorkerAgent(std::weak_ptr manager, + bool is_session_trusted); ~WorkerAgent() override = default; void Wire(UberDispatcher* dispatcher); @@ -31,6 +32,7 @@ class WorkerAgent : public NodeWorker::Backend { std::weak_ptr manager_; std::unique_ptr event_handle_; std::shared_ptr workers_; + bool is_session_trusted_; }; } // namespace protocol } // namespace inspector diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index d8b5d01a285834..d93d71160f3555 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -86,6 +86,37 @@ void StartIoInterrupt(Isolate* isolate, void* agent) { static_cast(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(); +} + +std::vector Split(const std::string& string, + const std::string& separator) { + std::vector result; + if (string.empty()) { + return result; + } + size_t pos = 0; + size_t len = string.length(); + while (pos < len) { + size_t comma = string.find(separator, pos); + if (comma == std::string::npos) { + comma = len; + } + result.push_back(string.substr(pos, comma - pos)); + pos = comma + 1; + } + return result; +} #ifdef __POSIX__ static void StartIoThreadWakeup(int signo) { @@ -221,15 +252,17 @@ class ChannelImpl final : public v8_inspector::V8Inspector::Channel, std::shared_ptr worker_manager, std::unique_ptr delegate, std::shared_ptr main_thread_, + std::unordered_set domain_whitelist, bool prevent_shutdown) - : delegate_(std::move(delegate)), prevent_shutdown_(prevent_shutdown), - retaining_context_(false) { + : 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(this); tracing_agent_ = std::make_unique(env, main_thread_); tracing_agent_->Wire(node_dispatcher_.get()); - worker_agent_ = std::make_unique(worker_manager); + worker_agent_ = std::make_unique( + worker_manager, delegate_->IsSessionTrusted()); worker_agent_->Wire(node_dispatcher_.get()); runtime_agent_ = std::make_unique(env); runtime_agent_->Wire(node_dispatcher_.get()); @@ -252,6 +285,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 +355,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 = Split(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 runtime_agent_; std::unique_ptr tracing_agent_; std::unique_ptr worker_agent_; std::unique_ptr delegate_; std::unique_ptr session_; std::unique_ptr node_dispatcher_; + std::unordered_set domain_whitelist_; bool prevent_shutdown_; bool retaining_context_; }; @@ -448,8 +506,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& 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 +585,7 @@ class NodeInspectorClient : public V8InspectorClient { getWorkerManager(), std::move(delegate), getThreadHandle(), + domains_whitelist_, prevent_shutdown); return session_id; } @@ -719,6 +782,7 @@ class NodeInspectorClient : public V8InspectorClient { // Allows accessing Inspector from non-main threads std::unique_ptr interface_; std::shared_ptr worker_manager_; + std::unordered_set domains_whitelist_; }; Agent::Agent(Environment* env) @@ -744,7 +808,8 @@ bool Agent::Start(const std::string& path, CHECK_NOT_NULL(host_port); host_port_ = host_port; - client_ = std::make_shared(parent_env_, is_main); + client_ = std::make_shared( + parent_env_, is_main, Split(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 +1036,5 @@ void SameThreadInspectorSession::Dispatch( if (client) client->dispatchMessageFromFrontend(session_id_, message); } - - - } // namespace inspector } // namespace node diff --git a/src/inspector_agent.h b/src/inspector_agent.h index 1c2bde0da73e84..7fe663504444f8 100644 --- a/src/inspector_agent.h +++ b/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 { diff --git a/src/inspector_io.cc b/src/inspector_io.cc index 7ba19087d01b0d..13f3de60fd82b9 100644 --- a/src/inspector_io.cc +++ b/src/inspector_io.cc @@ -203,6 +203,10 @@ class IoSessionDelegate : public InspectorSessionDelegate { StringBuffer::create(message)); } + bool IsSessionTrusted() override { + return false; + } + private: std::shared_ptr request_queue_; int id_; diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc index 5caf3fa09a4d10..539944ef967f5c 100644 --- a/src/inspector_js_api.cc +++ b/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_; diff --git a/src/inspector_profiler.h b/src/inspector_profiler.h index 219405b8c7f3f4..8ec60946511cbb 100644 --- a/src/inspector_profiler.h +++ b/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_; }; diff --git a/src/node_options.cc b/src/node_options.cc index a36666c3e0f452..721e440c681417 100644 --- a/src/node_options.cc +++ b/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() { diff --git a/src/node_options.h b/src/node_options.h index db564ddb3d3e6d..23b62e93352642 100644 --- a/src/node_options.h +++ b/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 }; diff --git a/test/parallel/test-inspector-domain-whitelist.js b/test/parallel/test-inspector-domain-whitelist.js new file mode 100644 index 00000000000000..54ff4c9fd92e97 --- /dev/null +++ b/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(); From 805f54e783113c6e4558c43daa0f8258180bed47 Mon Sep 17 00:00:00 2001 From: Eugene Ostroukhov Date: Thu, 16 May 2019 15:26:38 -0700 Subject: [PATCH 2/3] squash: addressing the code review comments --- src/inspector/main_thread_interface.cc | 5 +++-- src/inspector_agent.cc | 29 ++++++-------------------- 2 files changed, 9 insertions(+), 25 deletions(-) diff --git a/src/inspector/main_thread_interface.cc b/src/inspector/main_thread_interface.cc index ff92927201b256..eff1d488a1991f 100644 --- a/src/inspector/main_thread_interface.cc +++ b/src/inspector/main_thread_interface.cc @@ -197,8 +197,9 @@ class ThreadSafeDelegate : public InspectorSessionDelegate { ThreadSafeDelegate(std::shared_ptr thread, int object_id, int is_trusted) - : thread_(thread), delegate_(thread, object_id), - is_trusted_(is_trusted) {} + : thread_(thread), + delegate_(thread, object_id), + is_trusted_(is_trusted) {} void SendMessageToFrontend(const v8_inspector::StringView& message) override { delegate_.Call( diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index d93d71160f3555..baefe7cbdc33ed 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -99,25 +99,6 @@ std::string BuildMethodNotFoundErrorJSON( return response->toJSONString(); } -std::vector Split(const std::string& string, - const std::string& separator) { - std::vector result; - if (string.empty()) { - return result; - } - size_t pos = 0; - size_t len = string.length(); - while (pos < len) { - size_t comma = string.find(separator, pos); - if (comma == std::string::npos) { - comma = len; - } - result.push_back(string.substr(pos, comma - pos)); - pos = comma + 1; - } - return result; -} - #ifdef __POSIX__ static void StartIoThreadWakeup(int signo) { uv_sem_post(&start_io_thread_semaphore); @@ -254,8 +235,10 @@ class ChannelImpl final : public v8_inspector::V8Inspector::Channel, std::shared_ptr main_thread_, std::unordered_set domain_whitelist, bool prevent_shutdown) - : delegate_(std::move(delegate)), domain_whitelist_(domain_whitelist), - prevent_shutdown_(prevent_shutdown), retaining_context_(false) { + : 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(this); tracing_agent_ = @@ -367,7 +350,7 @@ class ChannelImpl final : public v8_inspector::V8Inspector::Channel, if (method == "Runtime.runIfWaitingForDebugger") { return true; } - const auto& domainAndMethod = Split(method, "."); + const auto& domainAndMethod = SplitString(method, '.'); if (domainAndMethod.size() < 2) { return true; // Dispatcher will send a proper error } @@ -809,7 +792,7 @@ bool Agent::Start(const std::string& path, host_port_ = host_port; client_ = std::make_shared( - parent_env_, is_main, Split(options.domain_whitelist, ",")); + 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(), From fe0dbcd876620e85c340f118ca97826bb134eaa3 Mon Sep 17 00:00:00 2001 From: Eugene Ostroukhov Date: Thu, 16 May 2019 15:33:46 -0700 Subject: [PATCH 3/3] squash: address code review comment --- src/inspector/main_thread_interface.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/inspector/main_thread_interface.cc b/src/inspector/main_thread_interface.cc index eff1d488a1991f..94cbf224a9fc2a 100644 --- a/src/inspector/main_thread_interface.cc +++ b/src/inspector/main_thread_interface.cc @@ -196,7 +196,7 @@ class ThreadSafeDelegate : public InspectorSessionDelegate { public: ThreadSafeDelegate(std::shared_ptr thread, int object_id, - int is_trusted) + bool is_trusted) : thread_(thread), delegate_(thread, object_id), is_trusted_(is_trusted) {}