diff --git a/doc/api/cli.md b/doc/api/cli.md index e979ec95c4259d..a44d4989ed37e8 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -191,6 +191,45 @@ This behavior also applies to `child_process.spawn()`, but in that case, the flags are propagated via the `NODE_OPTIONS` environment variable rather than directly through the process arguments. +### `--allow-env` + +> Stability: 1.1 - Active development + +When using the [Permission Model][], access to environment variables is +restricted by default unless the user explicitly passes the `--allow-env` +flag when starting Node.js. + +* Reading a restricted variable (`process.env.HOME`) silently returns + `undefined`. +* Writing (`process.env.FOO = 'bar'`) or deleting (`delete process.env.FOO`) + a restricted variable throws `ERR_ACCESS_DENIED`. + +The valid arguments for the `--allow-env` flag are: + +* `*` - To allow access to all environment variables. +* Specific environment variable names can be allowed using a comma-separated + list. Example: `--allow-env=HOME,PATH,NODE_ENV` + +Example: + +```js +console.log(process.env.HOME); // undefined (silently denied) +process.env.FOO = 'bar'; // ERR_ACCESS_DENIED (throws) +``` + +```console +$ node --permission index.js +node:internal/process/per_thread:12 + throw new ERR_ACCESS_DENIED('EnvVar', name); + ^ + +Error: Access to this API has been restricted + at node:internal/main/run_main_module:17:47 { + code: 'ERR_ACCESS_DENIED', + permission: 'EnvVar' +} +``` + ### `--allow-ffi` @@ -4595,6 +4601,7 @@ cases: [`'exit'`]: #event-exit [`'message'`]: child_process.md#event-message [`'uncaughtException'`]: #event-uncaughtexception +[`--allow-env`]: cli.md#--allow-env [`--no-deprecation`]: cli.md#--no-deprecation [`--permission`]: cli.md#--permission [`--unhandled-rejections`]: cli.md#--unhandled-rejectionsmode diff --git a/lib/internal/process/permission.js b/lib/internal/process/permission.js index 97ea0265fa15d9..494b41bcd770b7 100644 --- a/lib/internal/process/permission.js +++ b/lib/internal/process/permission.js @@ -46,6 +46,7 @@ module.exports = ObjectFreeze({ '--allow-fs-write', '--allow-addons', '--allow-child-process', + '--allow-env', '--allow-net', '--allow-inspector', '--allow-wasi', diff --git a/node.gyp b/node.gyp index 6a21e715c89d22..5d75d715b2b2ce 100644 --- a/node.gyp +++ b/node.gyp @@ -173,6 +173,7 @@ 'src/path.cc', 'src/permission/child_process_permission.cc', 'src/permission/ffi_permission.cc', + 'src/permission/env_var_permission.cc', 'src/permission/fs_permission.cc', 'src/permission/inspector_permission.cc', 'src/permission/permission.cc', @@ -309,6 +310,7 @@ 'src/path.h', 'src/permission/child_process_permission.h', 'src/permission/ffi_permission.h', + 'src/permission/env_var_permission.h', 'src/permission/fs_permission.h', 'src/permission/inspector_permission.h', 'src/permission/permission.h', diff --git a/src/env.cc b/src/env.cc index f618fca2e2d157..263a247f0c93e6 100644 --- a/src/env.cc +++ b/src/env.cc @@ -949,6 +949,13 @@ Environment::Environment(IsolateData* isolate_data, permission()->Apply(this, {"*"}, permission::PermissionScope::kWASI); } + if (!options_->allow_env.empty()) { + permission()->Apply( + this, options_->allow_env, permission::PermissionScope::kEnvVar); + } else { + permission()->Apply(this, {}, permission::PermissionScope::kEnvVar); + } + // Implicit allow entrypoint to kFileSystemRead if (!options_->has_eval_string && !options_->force_repl) { std::string first_argv; diff --git a/src/node_env_var.cc b/src/node_env_var.cc index e94180cd659d35..e4a542abcdb542 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -4,6 +4,7 @@ #include "node_external_reference.h" #include "node_i18n.h" #include "node_process-inl.h" +#include "permission/permission.h" #include "util.h" #include // tzset(), _tzset() @@ -435,6 +436,14 @@ static Intercepted EnvGetter(Local property, return Intercepted::kYes; } CHECK(property->IsString()); + + Utf8Value key(env->isolate(), property); + if (env->permission()->enabled() && + !env->permission()->is_granted( + env, permission::PermissionScope::kEnvVar, key.ToStringView())) { + return Intercepted::kNo; + } + MaybeLocal value_string = env->env_vars()->Get(env->isolate(), property.As()); @@ -453,6 +462,15 @@ static Intercepted EnvSetter(Local property, const PropertyCallbackInfo& info) { Environment* env = Environment::GetCurrent(info); CHECK(env->has_run_bootstrapping_code()); + + if (property->IsString()) { + Utf8Value key(env->isolate(), property); + THROW_IF_INSUFFICIENT_PERMISSIONS(env, + permission::PermissionScope::kEnvVar, + key.ToStringView(), + Intercepted::kYes); + } + // calling env->EmitProcessEnvWarning() sets a variable indicating that // warnings have been emitted. It should be called last after other // conditions leading to a warning have been met. @@ -489,6 +507,13 @@ static Intercepted EnvQuery(Local property, Environment* env = Environment::GetCurrent(info); CHECK(env->has_run_bootstrapping_code()); if (property->IsString()) { + Utf8Value key(env->isolate(), property); + if (env->permission()->enabled() && + !env->permission()->is_granted( + env, permission::PermissionScope::kEnvVar, key.ToStringView())) { + return Intercepted::kNo; + } + int32_t rc = env->env_vars()->Query(env->isolate(), property.As()); bool has_env = (rc != -1); TraceEnvVar(env, "query", property.As()); @@ -506,6 +531,12 @@ static Intercepted EnvDeleter(Local property, Environment* env = Environment::GetCurrent(info); CHECK(env->has_run_bootstrapping_code()); if (property->IsString()) { + Utf8Value key(env->isolate(), property); + THROW_IF_INSUFFICIENT_PERMISSIONS(env, + permission::PermissionScope::kEnvVar, + key.ToStringView(), + Intercepted::kYes); + env->env_vars()->Delete(env->isolate(), property.As()); TraceEnvVar(env, "delete", property.As()); @@ -525,7 +556,23 @@ static void EnvEnumerator(const PropertyCallbackInfo& info) { Local ret; if (env->env_vars()->Enumerate(env->isolate()).ToLocal(&ret)) { - info.GetReturnValue().Set(ret); + if (env->permission()->enabled()) { + LocalVector filtered(env->isolate()); + for (uint32_t i = 0; i < ret->Length(); i++) { + Local elem; + if (!ret->Get(env->context(), i).ToLocal(&elem)) continue; + Utf8Value key(env->isolate(), elem); + if (env->permission()->is_granted(env, + permission::PermissionScope::kEnvVar, + key.ToStringView())) { + filtered.push_back(elem); + } + } + info.GetReturnValue().Set( + Array::New(env->isolate(), filtered.data(), filtered.size())); + } else { + info.GetReturnValue().Set(ret); + } } } diff --git a/src/node_options.cc b/src/node_options.cc index bbb72d2ba1bcf4..bdde46b1f73385 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -712,6 +712,12 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() { kAllowedInEnvvar, false, OptionNamespaces::kPermissionNamespace); + AddOption("--allow-env", + "allow access to environment variables when any permissions are " + "set", + &EnvironmentOptions::allow_env, + kAllowedInEnvvar, + OptionNamespaces::kPermissionNamespace); AddOption("--experimental-repl-await", "experimental await keyword support in REPL", &EnvironmentOptions::experimental_repl_await, diff --git a/src/node_options.h b/src/node_options.h index 8ee0f55e65e8a6..6bd07ac80e6a06 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -147,6 +147,7 @@ class EnvironmentOptions : public Options { bool allow_wasi = false; bool allow_ffi = false; bool allow_worker_threads = false; + std::vector allow_env; bool experimental_repl_await = true; bool experimental_vm_modules = false; bool async_context_frame = true; diff --git a/src/permission/env_var_permission.cc b/src/permission/env_var_permission.cc new file mode 100644 index 00000000000000..a5698032610dc4 --- /dev/null +++ b/src/permission/env_var_permission.cc @@ -0,0 +1,42 @@ +#include "env_var_permission.h" + +#include +#include + +namespace node { + +namespace permission { + +void EnvVarPermission::Apply(Environment* env, + const std::vector& allow, + PermissionScope scope) { + deny_all_ = true; + if (!allow.empty()) { + if (allow.size() == 1 && allow[0] == "*") { + allow_all_ = true; + return; + } + for (const std::string& arg : allow) { + allowed_env_vars_.insert(arg); + } + } +} + +bool EnvVarPermission::is_granted(Environment* env, + PermissionScope perm, + const std::string_view& param) const { + if (deny_all_) { + if (allow_all_) { + return true; + } + if (param.empty()) { + return false; + } + return allowed_env_vars_.find(std::string(param)) != + allowed_env_vars_.end(); + } + return true; +} + +} // namespace permission +} // namespace node diff --git a/src/permission/env_var_permission.h b/src/permission/env_var_permission.h new file mode 100644 index 00000000000000..71d64ba087e5b6 --- /dev/null +++ b/src/permission/env_var_permission.h @@ -0,0 +1,35 @@ +#ifndef SRC_PERMISSION_ENV_VAR_PERMISSION_H_ +#define SRC_PERMISSION_ENV_VAR_PERMISSION_H_ + +#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#include +#include +#include +#include "permission/permission_base.h" + +namespace node { + +namespace permission { + +class EnvVarPermission final : public PermissionBase { + public: + void Apply(Environment* env, + const std::vector& allow, + PermissionScope scope) override; + bool is_granted(Environment* env, + PermissionScope perm, + const std::string_view& param = "") const override; + + private: + bool deny_all_ = false; + bool allow_all_ = false; + std::set allowed_env_vars_; +}; + +} // namespace permission + +} // namespace node + +#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS +#endif // SRC_PERMISSION_ENV_VAR_PERMISSION_H_ diff --git a/src/permission/permission.cc b/src/permission/permission.cc index 0da8fdcd0b9c1e..af03d3584c1356 100644 --- a/src/permission/permission.cc +++ b/src/permission/permission.cc @@ -48,6 +48,8 @@ constexpr std::string_view GetDiagnosticsChannelName(PermissionScope scope) { return "node:permission-model:addon"; case PermissionScope::kFFI: return "node:permission-model:ffi"; + case PermissionScope::kEnvVar: + return "node:permission-model:env"; default: return {}; } @@ -108,6 +110,8 @@ Permission::Permission() : enabled_(false), warning_only_(false) { std::shared_ptr net = std::make_shared(); std::shared_ptr addon = std::make_shared(); std::shared_ptr ffi = std::make_shared(); + std::shared_ptr env_var = + std::make_shared(); #define V(Name, _, __, ___) \ nodes_.insert(std::make_pair(PermissionScope::k##Name, fs)); FILESYSTEM_PERMISSIONS(V) @@ -140,6 +144,10 @@ Permission::Permission() : enabled_(false), warning_only_(false) { nodes_.insert(std::make_pair(PermissionScope::k##Name, ffi)); FFI_PERMISSIONS(V) #undef V +#define V(Name, _, __, ___) \ + nodes_.insert(std::make_pair(PermissionScope::k##Name, env_var)); + ENV_VAR_PERMISSIONS(V) +#undef V } const char* GetErrorFlagSuggestion(node::permission::PermissionScope perm) { diff --git a/src/permission/permission.h b/src/permission/permission.h index b43c9648093b08..ed63d547421c4a 100644 --- a/src/permission/permission.h +++ b/src/permission/permission.h @@ -8,6 +8,7 @@ #include "node_options.h" #include "permission/addon_permission.h" #include "permission/child_process_permission.h" +#include "permission/env_var_permission.h" #include "permission/ffi_permission.h" #include "permission/fs_permission.h" #include "permission/inspector_permission.h" diff --git a/src/permission/permission_base.h b/src/permission/permission_base.h index 4c796c770dccf8..6ce18edd3ea078 100644 --- a/src/permission/permission_base.h +++ b/src/permission/permission_base.h @@ -36,6 +36,7 @@ namespace permission { V(Addon, "addon", PermissionsRoot, "--allow-addons") #define FFI_PERMISSIONS(V) V(FFI, "ffi", PermissionsRoot, "--allow-ffi") +#define ENV_VAR_PERMISSIONS(V) V(EnvVar, "env", PermissionsRoot, "--allow-env") #define PERMISSIONS(V) \ FILESYSTEM_PERMISSIONS(V) \ @@ -45,10 +46,11 @@ namespace permission { INSPECTOR_PERMISSIONS(V) \ NET_PERMISSIONS(V) \ ADDON_PERMISSIONS(V) \ - FFI_PERMISSIONS(V) + FFI_PERMISSIONS(V) \ + ENV_VAR_PERMISSIONS(V) -#define V(name, _, __, ___) k##name, enum class PermissionScope { +#define V(name, _, __, ___) k##name, kPermissionsRoot = -1, PERMISSIONS(V) kPermissionsCount }; diff --git a/test/parallel/test-permission-env-allow-all.js b/test/parallel/test-permission-env-allow-all.js new file mode 100644 index 00000000000000..5ca5d06a421f80 --- /dev/null +++ b/test/parallel/test-permission-env-allow-all.js @@ -0,0 +1,23 @@ +// Flags: --permission --allow-fs-read=* --allow-env=* +'use strict'; + +const common = require('../common'); +const { isMainThread } = require('worker_threads'); + +if (!isMainThread) { + common.skip('This test only works on a main thread'); +} + +const assert = require('assert'); + +// With --allow-env=* all env vars should be accessible +{ + assert.ok(process.permission.has('env')); + // Should not throw for any env var + assert.ok(process.env.HOME !== undefined); + assert.ok(process.env.PATH !== undefined); + process.env.TEST_PERMISSION_VAR = 'test'; + assert.strictEqual(process.env.TEST_PERMISSION_VAR, 'test'); + delete process.env.TEST_PERMISSION_VAR; + assert.strictEqual(process.env.TEST_PERMISSION_VAR, undefined); +} diff --git a/test/parallel/test-permission-env-cli.js b/test/parallel/test-permission-env-cli.js new file mode 100644 index 00000000000000..889a9370b87b58 --- /dev/null +++ b/test/parallel/test-permission-env-cli.js @@ -0,0 +1,95 @@ +// Flags: --permission --allow-fs-read=* --allow-env=HOME --allow-env=PATH +'use strict'; + +const common = require('../common'); +const { isMainThread } = require('worker_threads'); + +if (!isMainThread) { + common.skip('This test only works on a main thread'); +} + +const assert = require('assert'); + +{ + assert.ok(!process.permission.has('env')); +} + +{ + assert.ok(process.env.HOME !== undefined); + assert.ok(process.env.PATH !== undefined); + assert.ok(process.permission.has('env', 'HOME')); + assert.ok(process.permission.has('env', 'PATH')); +} + +{ + assert.strictEqual(process.env.SECRET_KEY, undefined); +} + +{ + assert.throws(() => { + process.env.NEW_VAR = 'value'; + }, common.expectsError({ + code: 'ERR_ACCESS_DENIED', + permission: 'EnvVar', + resource: 'NEW_VAR', + })); +} + +{ + assert.throws(() => { + delete process.env.SECRET_KEY; + }, common.expectsError({ + code: 'ERR_ACCESS_DENIED', + permission: 'EnvVar', + resource: 'SECRET_KEY', + })); +} + +{ + assert.strictEqual('SECRET_KEY' in process.env, false); +} + +{ + const keys = Object.keys(process.env); + for (const key of keys) { + assert.ok( + key === 'HOME' || key === 'PATH', + `Unexpected env var in enumeration: ${key}` + ); + } +} + +{ + const keys = []; + for (const key in process.env) { + keys.push(key); + } + for (const key of keys) { + assert.ok( + key === 'HOME' || key === 'PATH', + `Unexpected env var in for...in: ${key}` + ); + } +} + +{ + const copy = Object.assign({}, process.env); + const keys = Object.keys(copy); + for (const key of keys) { + assert.ok( + key === 'HOME' || key === 'PATH', + `Unexpected env var in Object.assign: ${key}` + ); + } +} + +{ + const parsed = JSON.parse(JSON.stringify(process.env)); + const keys = Object.keys(parsed); + for (const key of keys) { + assert.ok( + key === 'HOME' || key === 'PATH', + `Unexpected env var in JSON.stringify: ${key}` + ); + } +} diff --git a/test/parallel/test-permission-env-deny-all.js b/test/parallel/test-permission-env-deny-all.js new file mode 100644 index 00000000000000..3c93118f261990 --- /dev/null +++ b/test/parallel/test-permission-env-deny-all.js @@ -0,0 +1,34 @@ +// Flags: --permission --allow-fs-read=* +'use strict'; + +const common = require('../common'); +const { isMainThread } = require('worker_threads'); + +if (!isMainThread) { + common.skip('This test only works on a main thread'); +} + +const assert = require('assert'); + +{ + assert.ok(!process.permission.has('env')); +} + +{ + assert.strictEqual(process.env.HOME, undefined); + assert.strictEqual(process.env.PATH, undefined); +} + +{ + assert.throws(() => { + process.env.TEST_VAR = 'value'; + }, common.expectsError({ + code: 'ERR_ACCESS_DENIED', + permission: 'EnvVar', + })); +} + +{ + const keys = Object.keys(process.env); + assert.strictEqual(keys.length, 0); +}