src,permission: restrict inspector when pm enabled

PR-URL: nodejs-private/node-private#410 Refs: https://hackerone.com/bugs?subject=nodejs&report_id=1962701 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> CVE-ID: CVE-2023-30587
nodejs · Jun 20, 2023 · 34d92ed · 34d92ed
1 parent 1b16ffa
commit 34d92ed
Show file tree

Hide file tree

Showing 11 changed files with 125 additions and 6 deletions.
diff --git a/doc/api/permissions.md b/doc/api/permissions.md
@@ -466,7 +466,8 @@ flag.
 
 When starting Node.js with `--experimental-permission`,
 the ability to access the file system through the `fs` module, spawn processes,
-and use `node:worker_threads` will be restricted.
+use `node:worker_threads` and enable the runtime inspector
+will be restricted.
 
 ```console
 $ node --experimental-permission index.js

diff --git a/node.gyp b/node.gyp
@@ -139,6 +139,7 @@
       'src/node_zlib.cc',
       'src/permission/child_process_permission.cc',
       'src/permission/fs_permission.cc',
+      'src/permission/inspector_permission.cc',
       'src/permission/permission.cc',
       'src/permission/worker_permission.cc',
       'src/pipe_wrap.cc',
@@ -258,6 +259,7 @@
       'src/node_worker.h',
       'src/permission/child_process_permission.h',
       'src/permission/fs_permission.h',
+      'src/permission/inspector_permission.h',
       'src/permission/permission.h',
       'src/permission/worker_permission.h',
       'src/pipe_wrap.h',

diff --git a/src/env.cc b/src/env.cc
@@ -806,9 +806,12 @@ Environment::Environment(IsolateData* isolate_data,
   if (options_->experimental_permission) {
     permission()->EnablePermissions();
     // If any permission is set the process shouldn't be able to neither
-    // spawn/worker nor use addons unless explicitly allowed by the user
+    // spawn/worker nor use addons or enable inspector
+    // unless explicitly allowed by the user
     if (!options_->allow_fs_read.empty() || !options_->allow_fs_write.empty()) {
       options_->allow_native_addons = false;
+      flags_ = flags_ | EnvironmentFlags::kNoCreateInspector;
+      permission()->Apply("*", permission::PermissionScope::kInspector);
       if (!options_->allow_child_process) {
         permission()->Apply("*", permission::PermissionScope::kChildProcess);
       }

diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc
@@ -14,8 +14,9 @@
 #include "node_options-inl.h"
 #include "node_process-inl.h"
 #include "node_url.h"
-#include "util-inl.h"
+#include "permission/permission.h"
 #include "timer_wrap-inl.h"
+#include "util-inl.h"
 #include "v8-inspector.h"
 #include "v8-platform.h"
 
@@ -755,6 +756,10 @@ bool Agent::StartIoThread() {
   if (io_ != nullptr)
     return true;
 
+  THROW_IF_INSUFFICIENT_PERMISSIONS(parent_env_,
+                                    permission::PermissionScope::kInspector,
+                                    "StartIoThread",
+                                    false);
   if (!parent_env_->should_create_inspector() && !client_) {
     ThrowUninitializedInspectorError(parent_env_);
     return false;
@@ -780,6 +785,10 @@ void Agent::Stop() {
 std::unique_ptr<InspectorSession> Agent::Connect(
     std::unique_ptr<InspectorSessionDelegate> delegate,
     bool prevent_shutdown) {
+  THROW_IF_INSUFFICIENT_PERMISSIONS(parent_env_,
+                                    permission::PermissionScope::kInspector,
+                                    "Connect",
+                                    std::unique_ptr<InspectorSession>{});
   if (!parent_env_->should_create_inspector() && !client_) {
     ThrowUninitializedInspectorError(parent_env_);
     return std::unique_ptr<InspectorSession>{};
@@ -796,6 +805,10 @@ std::unique_ptr<InspectorSession> Agent::Connect(
 std::unique_ptr<InspectorSession> Agent::ConnectToMainThread(
     std::unique_ptr<InspectorSessionDelegate> delegate,
     bool prevent_shutdown) {
+  THROW_IF_INSUFFICIENT_PERMISSIONS(parent_env_,
+                                    permission::PermissionScope::kInspector,
+                                    "ConnectToMainThread",
+                                    std::unique_ptr<InspectorSession>{});
   if (!parent_env_->should_create_inspector() && !client_) {
     ThrowUninitializedInspectorError(parent_env_);
     return std::unique_ptr<InspectorSession>{};
@@ -810,6 +823,9 @@ std::unique_ptr<InspectorSession> Agent::ConnectToMainThread(
 }
 
 void Agent::WaitForDisconnect() {
+  THROW_IF_INSUFFICIENT_PERMISSIONS(parent_env_,
+                                    permission::PermissionScope::kInspector,
+                                    "WaitForDisconnect");
   if (!parent_env_->should_create_inspector() && !client_) {
     ThrowUninitializedInspectorError(parent_env_);
     return;
@@ -963,6 +979,10 @@ void Agent::SetParentHandle(
 
 std::unique_ptr<ParentInspectorHandle> Agent::GetParentHandle(
     uint64_t thread_id, const std::string& url, const std::string& name) {
+  THROW_IF_INSUFFICIENT_PERMISSIONS(parent_env_,
+                                    permission::PermissionScope::kInspector,
+                                    "GetParentHandle",
+                                    std::unique_ptr<ParentInspectorHandle>{});
   if (!parent_env_->should_create_inspector() && !client_) {
     ThrowUninitializedInspectorError(parent_env_);
     return std::unique_ptr<ParentInspectorHandle>{};
@@ -977,6 +997,8 @@ std::unique_ptr<ParentInspectorHandle> Agent::GetParentHandle(
 }
 
 void Agent::WaitForConnect() {
+  THROW_IF_INSUFFICIENT_PERMISSIONS(
+      parent_env_, permission::PermissionScope::kInspector, "WaitForConnect");
   if (!parent_env_->should_create_inspector() && !client_) {
     ThrowUninitializedInspectorError(parent_env_);
     return;
@@ -987,6 +1009,10 @@ void Agent::WaitForConnect() {
 }
 
 std::shared_ptr<WorkerManager> Agent::GetWorkerManager() {
+  THROW_IF_INSUFFICIENT_PERMISSIONS(parent_env_,
+                                    permission::PermissionScope::kInspector,
+                                    "GetWorkerManager",
+                                    std::unique_ptr<WorkerManager>{});
   if (!parent_env_->should_create_inspector() && !client_) {
     ThrowUninitializedInspectorError(parent_env_);
     return std::unique_ptr<WorkerManager>{};

diff --git a/src/node_worker.cc b/src/node_worker.cc
@@ -85,8 +85,13 @@ Worker::Worker(Environment* env,
                 Number::New(env->isolate(), static_cast<double>(thread_id_.id)))
       .Check();
 
-  inspector_parent_handle_ =
-      GetInspectorParentHandle(env, thread_id_, url.c_str(), name.c_str());
+  // Without this check, to use the permission model with
+  // workers (--allow-worker) one would need to pass --allow-inspector as well
+  if (env->permission()->is_granted(
+          node::permission::PermissionScope::kInspector)) {
+    inspector_parent_handle_ =
+        GetInspectorParentHandle(env, thread_id_, url.c_str(), name.c_str());
+  }
 
   argv_ = std::vector<std::string>{env->argv()[0]};
   // Mark this Worker object as weak until we actually start the thread.

diff --git a/src/permission/inspector_permission.cc b/src/permission/inspector_permission.cc
@@ -0,0 +1,22 @@
+#include "inspector_permission.h"
+
+#include <string>
+
+namespace node {
+
+namespace permission {
+
+// Currently, Inspector manage a single state
+// Once denied, it's always denied
+void InspectorPermission::Apply(const std::string& allow,
+                                PermissionScope scope) {
+  deny_all_ = true;
+}
+
+bool InspectorPermission::is_granted(PermissionScope perm,
+                                     const std::string_view& param) {
+  return deny_all_ == false;
+}
+
+}  // namespace permission
+}  // namespace node
diff --git a/src/permission/inspector_permission.h b/src/permission/inspector_permission.h
@@ -0,0 +1,28 @@
+#ifndef SRC_PERMISSION_INSPECTOR_PERMISSION_H_
+#define SRC_PERMISSION_INSPECTOR_PERMISSION_H_
+
+#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
+
+#include <string>
+#include "permission/permission_base.h"
+
+namespace node {
+
+namespace permission {
+
+class InspectorPermission final : public PermissionBase {
+ public:
+  void Apply(const std::string& allow, PermissionScope scope) override;
+  bool is_granted(PermissionScope perm,
+                  const std::string_view& param = "") override;
+
+ private:
+  bool deny_all_;
+};
+
+}  // namespace permission
+
+}  // namespace node
+
+#endif  // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
+#endif  // SRC_PERMISSION_INSPECTOR_PERMISSION_H_
diff --git a/src/permission/permission.cc b/src/permission/permission.cc
@@ -79,6 +79,8 @@ Permission::Permission() : enabled_(false) {
       std::make_shared<ChildProcessPermission>();
   std::shared_ptr<PermissionBase> worker_t =
       std::make_shared<WorkerPermission>();
+  std::shared_ptr<PermissionBase> inspector =
+      std::make_shared<InspectorPermission>();
 #define V(Name, _, __)                                                         \
   nodes_.insert(std::make_pair(PermissionScope::k##Name, fs));
   FILESYSTEM_PERMISSIONS(V)
@@ -91,6 +93,10 @@ Permission::Permission() : enabled_(false) {
   nodes_.insert(std::make_pair(PermissionScope::k##Name, worker_t));
   WORKER_THREADS_PERMISSIONS(V)
 #undef V
+#define V(Name, _, __)                                                         \
+  nodes_.insert(std::make_pair(PermissionScope::k##Name, inspector));
+  INSPECTOR_PERMISSIONS(V)
+#undef V
 }
 
 void Permission::ThrowAccessDenied(Environment* env,

diff --git a/src/permission/permission.h b/src/permission/permission.h
@@ -7,6 +7,7 @@
 #include "node_options.h"
 #include "permission/child_process_permission.h"
 #include "permission/fs_permission.h"
+#include "permission/inspector_permission.h"
 #include "permission/permission_base.h"
 #include "permission/worker_permission.h"
 #include "v8.h"

diff --git a/src/permission/permission_base.h b/src/permission/permission_base.h
@@ -22,10 +22,13 @@ namespace permission {
 #define WORKER_THREADS_PERMISSIONS(V)                                          \
   V(WorkerThreads, "worker", PermissionsRoot)
 
+#define INSPECTOR_PERMISSIONS(V) V(Inspector, "inspector", PermissionsRoot)
+
 #define PERMISSIONS(V)                                                         \
   FILESYSTEM_PERMISSIONS(V)                                                    \
   CHILD_PROCESS_PERMISSIONS(V)                                                 \
-  WORKER_THREADS_PERMISSIONS(V)
+  WORKER_THREADS_PERMISSIONS(V)                                                \
+  INSPECTOR_PERMISSIONS(V)
 
 #define V(name, _, __) k##name,
 enum class PermissionScope {

diff --git a/test/parallel/test-permission-inspector.js b/test/parallel/test-permission-inspector.js
@@ -0,0 +1,22 @@
+// Flags: --experimental-permission --allow-fs-read=*
+'use strict';
+
+const common = require('../common');
+common.skipIfWorker();
+common.skipIfInspectorDisabled();
+
+const { Session } = require('inspector');
+const assert = require('assert');
+
+if (!common.hasCrypto)
+  common.skip('no crypto');
+
+{
+  assert.throws(() => {
+    const session = new Session();
+    session.connect();
+  }, common.expectsError({
+    code: 'ERR_ACCESS_DENIED',
+    permission: 'Inspector',
+  }));
+}