GeezerVison is showing github.com — src,permission: add --permission-audit · nodejs/node@cd1eb3e
File treeExpand file treeCollapse file tree Open diff view settings Expand file treeCollapse file tree Open diff view settings Collapse file Copy file name to clipboardExpand all lines: doc/api/cli.md +11Lines changed: 11 additions & 0 deletions - Display the source diff
- Display the rich diff
| Original file line number | Diff line number | Diff line change |
|---|
@@ -2137,6 +2137,16 @@ following permissions are restricted: | 2137 | 2137 | * WASI - manageable through [`--allow-wasi`][] flag | 2138 | 2138 | * Addons - manageable through [`--allow-addons`][] flag | 2139 | 2139 |
| | 2140 | +### `--permission-audit` | | 2141 | + | | 2142 | +<!-- YAML | | 2143 | +added: REPLACEME | | 2144 | +--> | | 2145 | + | | 2146 | +Enable audit only for the permission model. When enabled, permission checks | | 2147 | +are performed but access is not denied. Instead, a warning is emitted for | | 2148 | +each permission violation via diagnostics channel. | | 2149 | + | 2140 | 2150 | ### `--preserve-symlinks` | 2141 | 2151 |
| 2142 | 2152 | <!-- YAML | @@ -3661,6 +3671,7 @@ one is included in the list below. | 3661 | 3671 | * `--openssl-legacy-provider` | 3662 | 3672 | * `--openssl-shared-config` | 3663 | 3673 | * `--pending-deprecation` | | 3674 | +* `--permission-audit` | 3664 | 3675 | * `--permission` | 3665 | 3676 | * `--preserve-symlinks-main` | 3666 | 3677 | * `--preserve-symlinks` | |
Collapse file Copy file name to clipboardExpand all lines: doc/node.1 +5Lines changed: 5 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -468,6 +468,11 @@ Among other uses, this can be used to enable FIPS-compliant crypto if Node.js is | 468 | 468 | .It Fl -pending-deprecation | 469 | 469 | Emit pending deprecation warnings. | 470 | 470 | . | | 471 | +.It Fl -permission-audit | | 472 | +Enable audit only for the permission model. When enabled, permission checks | | 473 | +are performed but access is not denied. Instead, a warning is emitted for | | 474 | +each permission violation via diagnostics channel. | | 475 | +. | 471 | 476 | .It Fl -preserve-symlinks | 472 | 477 | Instructs the module loader to preserve symbolic links when resolving and caching modules other than the main module. | 473 | 478 | . | |
Collapse file Copy file name to clipboardExpand all lines: lib/internal/process/pre_execution.js +1-1Lines changed: 1 addition & 1 deletion | Original file line number | Diff line number | Diff line change |
|---|
@@ -634,7 +634,7 @@ function setupDiagnosticsChannel() { | 634 | 634 | } | 635 | 635 |
| 636 | 636 | function initializePermission() { | 637 | | - const permission = getOptionValue('--permission'); | | 637 | + const permission = getOptionValue('--permission') || getOptionValue('--permission-audit'); | 638 | 638 | if (permission) { | 639 | 639 | process.binding = function binding(_module) { | 640 | 640 | throw new ERR_ACCESS_DENIED('process.binding'); | |
Collapse file Copy file name to clipboardExpand all lines: src/env.cc +4-1Lines changed: 4 additions & 1 deletion | Original file line number | Diff line number | Diff line change |
|---|
@@ -900,8 +900,11 @@ Environment::Environment(IsolateData* isolate_data, | 900 | 900 | tracing::CastTracedValue(traced_value)); | 901 | 901 | } | 902 | 902 |
| 903 | | - if (options_->permission) { | | 903 | + if (options_->permission || options_->permission_audit) { | 904 | 904 | permission()->EnablePermissions(); | | 905 | + if (options_->permission_audit) { | | 906 | + permission()->EnableWarningOnly(); | | 907 | + } | 905 | 908 | // The process shouldn't be able to neither | 906 | 909 | // spawn/worker nor use addons or enable inspector | 907 | 910 | // unless explicitly allowed by the user | |
Collapse file Copy file name to clipboardExpand all lines: src/node_options.cc +5Lines changed: 5 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -639,6 +639,11 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() { | 639 | 639 | &EnvironmentOptions::permission, | 640 | 640 | kAllowedInEnvvar, | 641 | 641 | false); | | 642 | + AddOption("--permission-audit", | | 643 | + "enable audit only for the permission system", | | 644 | + &EnvironmentOptions::permission_audit, | | 645 | + kAllowedInEnvvar, | | 646 | + false); | 642 | 647 | AddOption("--allow-fs-read", | 643 | 648 | "allow permissions to read the filesystem", | 644 | 649 | &EnvironmentOptions::allow_fs_read, | |
Collapse file Copy file name to clipboardExpand all lines: src/node_options.h +1Lines changed: 1 addition & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -143,6 +143,7 @@ class EnvironmentOptions : public Options { | 143 | 143 | std::string input_type; // Value of --input-type | 144 | 144 | bool entry_is_url = false; | 145 | 145 | bool permission = false; | | 146 | + bool permission_audit = false; | 146 | 147 | std::vector<std::string> allow_fs_read; | 147 | 148 | std::vector<std::string> allow_fs_write; | 148 | 149 | bool allow_addons = false; | |
Collapse file Copy file name to clipboardExpand all lines: src/permission/permission.cc +91-1Lines changed: 91 additions & 1 deletion | Original file line number | Diff line number | Diff line change |
|---|
| 3 | 3 | #include "env-inl.h" | 4 | 4 | #include "memory_tracker-inl.h" | 5 | 5 | #include "node.h" | | 6 | +#include "node_diagnostics_channel.h" | 6 | 7 | #include "node_errors.h" | 7 | 8 | #include "node_external_reference.h" | 8 | 9 | #include "node_file.h" | @@ -27,6 +28,27 @@ namespace permission { | 27 | 28 |
| 28 | 29 | namespace { | 29 | 30 |
| | 31 | +constexpr std::string_view GetDiagnosticsChannelName(PermissionScope scope) { | | 32 | + switch (scope) { | | 33 | + case PermissionScope::kFileSystem: | | 34 | + case PermissionScope::kFileSystemRead: | | 35 | + case PermissionScope::kFileSystemWrite: | | 36 | + return "node:permission-model:fs"; | | 37 | + case PermissionScope::kChildProcess: | | 38 | + return "node:permission-model:child"; | | 39 | + case PermissionScope::kWorkerThreads: | | 40 | + return "node:permission-model:worker"; | | 41 | + case PermissionScope::kInspector: | | 42 | + return "node:permission-model:inspector"; | | 43 | + case PermissionScope::kWASI: | | 44 | + return "node:permission-model:wasi"; | | 45 | + case PermissionScope::kAddon: | | 46 | + return "node:permission-model:addon"; | | 47 | + default: | | 48 | + return {}; | | 49 | + } | | 50 | +} | | 51 | + | 30 | 52 | // permission.has('fs.in', '/tmp/') | 31 | 53 | // permission.has('fs.in') | 32 | 54 | static void Has(const FunctionCallbackInfo<Value>& args) { | @@ -70,7 +92,7 @@ PermissionScope Permission::StringToPermission(const std::string& perm) { | 70 | 92 | } | 71 | 93 | #undef V | 72 | 94 |
| 73 | | -Permission::Permission() : enabled_(false) { | | 95 | +Permission::Permission() : enabled_(false), warning_only_(false) { | 74 | 96 | std::shared_ptr<PermissionBase> fs = std::make_shared<FSPermission>(); | 75 | 97 | std::shared_ptr<PermissionBase> child_p = | 76 | 98 | std::make_shared<ChildProcessPermission>(); | @@ -170,6 +192,74 @@ void Permission::EnablePermissions() { | 170 | 192 | } | 171 | 193 | } | 172 | 194 |
| | 195 | +void Permission::EnableWarningOnly() { | | 196 | + if (!warning_only_) { | | 197 | + warning_only_ = true; | | 198 | + } | | 199 | +} | | 200 | + | | 201 | +bool Permission::is_scope_granted(Environment* env, | | 202 | + const PermissionScope permission, | | 203 | + const std::string_view& res) const { | | 204 | + auto perm_node = nodes_.find(permission); | | 205 | + bool result = false; | | 206 | + if (perm_node != nodes_.end()) { | | 207 | + result = perm_node->second->is_granted(env, permission, res); | | 208 | + } | | 209 | + | | 210 | + if (!result && !publishing_) { | | 211 | + auto channel_name = GetDiagnosticsChannelName(permission); | | 212 | + if (!channel_name.empty()) { | | 213 | + auto ch = GetOrCreateChannel(env, permission); | | 214 | + if (ch && ch->HasSubscribers()) { | | 215 | + publishing_ = true; | | 216 | + v8::Isolate* isolate = env->isolate(); | | 217 | + v8::HandleScope handle_scope(isolate); | | 218 | + v8::Local<v8::Context> context = env->context(); | | 219 | + v8::Local<v8::Object> msg = | | 220 | + v8::Object::New(isolate, v8::Null(isolate), nullptr, nullptr, 0); | | 221 | + const char* perm_str = PermissionToString(permission); | | 222 | + msg->Set(context, | | 223 | + FIXED_ONE_BYTE_STRING(isolate, "permission"), | | 224 | + v8::String::NewFromUtf8(isolate, perm_str).ToLocalChecked()) | | 225 | + .Check(); | | 226 | + msg->Set(context, | | 227 | + FIXED_ONE_BYTE_STRING(isolate, "resource"), | | 228 | + v8::String::NewFromUtf8(isolate, | | 229 | + res.data(), | | 230 | + v8::NewStringType::kNormal, | | 231 | + static_cast<int>(res.size())) | | 232 | + .ToLocalChecked()) | | 233 | + .Check(); | | 234 | + ch->Publish(env, msg); | | 235 | + publishing_ = false; | | 236 | + } | | 237 | + } | | 238 | + } | | 239 | + | | 240 | + return result; | | 241 | +} | | 242 | + | | 243 | +BaseObjectPtr<diagnostics_channel::Channel> Permission::GetOrCreateChannel( | | 244 | + Environment* env, PermissionScope scope) const { | | 245 | + auto it = channels_.find(scope); | | 246 | + if (it != channels_.end()) { | | 247 | + // Promote weak ref to strong for the duration of this call. | | 248 | + BaseObjectPtr<diagnostics_channel::Channel> ptr(it->second.get()); | | 249 | + if (ptr) return ptr; | | 250 | + channels_.erase(it); | | 251 | + } | | 252 | + auto channel_name = GetDiagnosticsChannelName(scope); | | 253 | + diagnostics_channel::Channel* ch = | | 254 | + diagnostics_channel::Channel::Get(env, channel_name.data()); | | 255 | + if (ch != nullptr) { | | 256 | + channels_.emplace(scope, | | 257 | + BaseObjectWeakPtr<diagnostics_channel::Channel>(ch)); | | 258 | + return BaseObjectPtr<diagnostics_channel::Channel>(ch); | | 259 | + } | | 260 | + return {}; | | 261 | +} | | 262 | + | 173 | 263 | void Permission::Apply(Environment* env, | 174 | 264 | const std::vector<std::string>& allow, | 175 | 265 | PermissionScope scope) { | |
Collapse file Copy file name to clipboardExpand all lines: src/permission/permission.h +17-9Lines changed: 17 additions & 9 deletions | Original file line number | Diff line number | Diff line change |
|---|
| 4 | 4 | #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS | 5 | 5 |
| 6 | 6 | #include "debug_utils.h" | | 7 | +#include "node_diagnostics_channel.h" | 7 | 8 | #include "node_options.h" | 8 | 9 | #include "permission/addon_permission.h" | 9 | 10 | #include "permission/child_process_permission.h" | @@ -36,7 +37,7 @@ namespace permission { | 36 | 37 | [[unlikely]] { \ | 37 | 38 | node::permission::Permission::ThrowAccessDenied( \ | 38 | 39 | env__, perm__, resource__); \ | 39 | | - return __VA_ARGS__; \ | | 40 | + if (!env__->permission()->warning_only()) return __VA_ARGS__; \ | 40 | 41 | } \ | 41 | 42 | } while (0) | 42 | 43 |
| @@ -50,7 +51,7 @@ namespace permission { | 50 | 51 | [[unlikely]] { \ | 51 | 52 | node::permission::Permission::AsyncThrowAccessDenied( \ | 52 | 53 | env__, (wrap), perm__, resource__); \ | 53 | | - return __VA_ARGS__; \ | | 54 | + if (!env__->permission()->warning_only()) return __VA_ARGS__; \ | 54 | 55 | } \ | 55 | 56 | } while (0) | 56 | 57 |
| @@ -99,6 +100,8 @@ class Permission { | 99 | 100 |
| 100 | 101 | FORCE_INLINE bool enabled() const { return enabled_; } | 101 | 102 |
| | 103 | + FORCE_INLINE bool warning_only() const { return warning_only_; } | | 104 | + | 102 | 105 | static PermissionScope StringToPermission(const std::string& perm); | 103 | 106 | static const char* PermissionToString(PermissionScope perm); | 104 | 107 | static void ThrowAccessDenied(Environment* env, | @@ -114,20 +117,25 @@ class Permission { | 114 | 117 | const std::vector<std::string>& allow, | 115 | 118 | PermissionScope scope); | 116 | 119 | void EnablePermissions(); | | 120 | + void EnableWarningOnly(); | 117 | 121 |
| 118 | 122 | private: | 119 | 123 | COLD_NOINLINE bool is_scope_granted(Environment* env, | 120 | 124 | const PermissionScope permission, | 121 | | - const std::string_view& res = "") const { | 122 | | - auto perm_node = nodes_.find(permission); | 123 | | - if (perm_node != nodes_.end()) { | 124 | | - return perm_node->second->is_granted(env, permission, res); | 125 | | - } | 126 | | - return false; | 127 | | - } | | 125 | + const std::string_view& res = "") const; | | 126 | + | | 127 | + BaseObjectPtr<diagnostics_channel::Channel> GetOrCreateChannel( | | 128 | + Environment* env, PermissionScope scope) const; | 128 | 129 |
| 129 | 130 | std::unordered_map<PermissionScope, std::shared_ptr<PermissionBase>> nodes_; | 130 | 131 | bool enabled_; | | 132 | + bool warning_only_; | | 133 | + mutable bool publishing_ = false; | | 134 | + // Weak refs: BindingData (via BaseObjectPtr) is the sole owner of Channels. | | 135 | + // Using weak refs here avoids keeping Channels alive past Realm teardown. | | 136 | + mutable std::unordered_map<PermissionScope, | | 137 | + BaseObjectWeakPtr<diagnostics_channel::Channel>> | | 138 | + channels_; | 131 | 139 | }; | 132 | 140 |
| 133 | 141 | } // namespace permission | |
Collapse file Copy file name to clipboard +29Lines changed: 29 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
| | 1 | +// Flags: --permission --allow-fs-read=* | | 2 | +'use strict'; | | 3 | + | | 4 | +const common = require('../common'); | | 5 | +const { isMainThread } = require('worker_threads'); | | 6 | + | | 7 | +if (!isMainThread) { | | 8 | + common.skip('This test only works on a main thread'); | | 9 | +} | | 10 | + | | 11 | +const assert = require('node:assert'); | | 12 | +const dc = require('node:diagnostics_channel'); | | 13 | +const fs = require('node:fs'); | | 14 | + | | 15 | +const messages = []; | | 16 | +dc.subscribe('node:permission-model:fs', (msg) => { | | 17 | + messages.push(msg); | | 18 | +}); | | 19 | + | | 20 | +// Granted permission should not publish | | 21 | +fs.readFileSync(__filename); | | 22 | +assert.strictEqual(messages.length, 0); | | 23 | + | | 24 | +// Denied permission should publish | | 25 | +const hasWrite = process.permission.has('fs.write', '/tmp/test'); | | 26 | +assert.strictEqual(hasWrite, false); | | 27 | + | | 28 | +assert.ok(messages.length > 0, 'Expected at least one denied message'); | | 29 | +assert.strictEqual(messages[0].permission, 'FileSystemWrite'); |
|