Skip to content

Commit

Permalink
src,permission: restrict inspector when pm enabled
Browse files Browse the repository at this point in the history
  • Loading branch information
RafaelGSS committed Jun 20, 2023
1 parent 1b16ffa commit 34d92ed
Show file tree
Hide file tree
Showing 11 changed files with 125 additions and 6 deletions.
3 changes: 2 additions & 1 deletion doc/api/permissions.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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',
Expand Down
5 changes: 4 additions & 1 deletion src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
28 changes: 27 additions & 1 deletion src/inspector_agent.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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;
Expand All @@ -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>{};
Expand All @@ -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>{};
Expand All @@ -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;
Expand Down Expand Up @@ -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>{};
Expand All @@ -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;
Expand All @@ -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>{};
Expand Down
9 changes: 7 additions & 2 deletions src/node_worker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
22 changes: 22 additions & 0 deletions src/permission/inspector_permission.cc
Original file line number Diff line number Diff line change
@@ -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
28 changes: 28 additions & 0 deletions src/permission/inspector_permission.h
Original file line number Diff line number Diff line change
@@ -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_
6 changes: 6 additions & 0 deletions src/permission/permission.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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,
Expand Down
1 change: 1 addition & 0 deletions src/permission/permission.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
5 changes: 4 additions & 1 deletion src/permission/permission_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
22 changes: 22 additions & 0 deletions test/parallel/test-permission-inspector.js
Original file line number Diff line number Diff line change
@@ -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',
}));
}

0 comments on commit 34d92ed

Please sign in to comment.