From c569a233f4fbfc3781a985b23bc059475cfc479d Mon Sep 17 00:00:00 2001 From: Carlos Espa Date: Sun, 6 Aug 2023 23:11:54 +0200 Subject: [PATCH] src,permission: add multiple allow-fs-* flags Support for a single comma separates list for allow-fs-* flags is removed. Instead now multiple flags can be passed to allow multiple paths. Fixes: https://github.com/nodejs/security-wg/issues/1039 --- doc/api/cli.md | 14 +++- doc/api/permissions.md | 2 +- lib/internal/process/pre_execution.js | 21 ++++- src/env.cc | 6 +- src/node_options.h | 4 +- src/permission/child_process_permission.cc | 2 +- src/permission/child_process_permission.h | 3 +- src/permission/fs_permission.cc | 7 +- src/permission/fs_permission.h | 3 +- src/permission/inspector_permission.cc | 2 +- src/permission/inspector_permission.h | 3 +- src/permission/permission.cc | 3 +- src/permission/permission.h | 2 +- src/permission/permission_base.h | 3 +- src/permission/worker_permission.cc | 3 +- src/permission/worker_permission.h | 3 +- .../test-cli-permission-multiple-allow.js | 84 +++++++++++++++++++ test/parallel/test-permission-fs-read.js | 2 +- ...test-permission-fs-symlink-target-write.js | 2 +- test/parallel/test-permission-fs-symlink.js | 2 +- .../test-permission-fs-traversal-path.js | 2 +- test/parallel/test-permission-fs-wildcard.js | 6 +- 22 files changed, 149 insertions(+), 30 deletions(-) create mode 100644 test/parallel/test-cli-permission-multiple-allow.js diff --git a/doc/api/cli.md b/doc/api/cli.md index cf509503dc838a..85513ba8d65ede 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -155,8 +155,11 @@ the [Permission Model][]. The valid arguments for the `--allow-fs-read` flag are: * `*` - To allow all `FileSystemRead` operations. -* Paths delimited by comma (`,`) to allow only matching `FileSystemRead` - operations. +* Multiple paths can be allowed using multiple `--allow-fs-read` flags. + Example `--allow-fs-read=/folder1/ --allow-fs-read=/folder1/` + +NOTE: Paths delimited by comma (`,`) are no longer allowed. +When passing a single flag with a comma a warning will be diplayed Examples can be found in the [File System Permissions][] documentation. @@ -202,8 +205,11 @@ the [Permission Model][]. The valid arguments for the `--allow-fs-write` flag are: * `*` - To allow all `FileSystemWrite` operations. -* Paths delimited by comma (`,`) to allow only matching `FileSystemWrite` - operations. +* Multiple paths can be allowed using multiple `--allow-fs-read` flags. + Example `--allow-fs-read=/folder1/ --allow-fs-read=/folder1/` + +NOTE: Paths delimited by comma (`,`) are no longer allowed. +When passing a single flag with a comma a warning will be diplayed Examples can be found in the [File System Permissions][] documentation. diff --git a/doc/api/permissions.md b/doc/api/permissions.md index 2c50c4fbabfb55..0d1ec9e5d61a72 100644 --- a/doc/api/permissions.md +++ b/doc/api/permissions.md @@ -532,7 +532,7 @@ Example: * `--allow-fs-write=*` - It will allow all `FileSystemWrite` operations. * `--allow-fs-write=/tmp/` - It will allow `FileSystemWrite` access to the `/tmp/` folder. -* `--allow-fs-read=/tmp/,/home/.gitignore` - It allows `FileSystemRead` access +* `--allow-fs-read=/tmp/ --allow-fs-read=/home/.gitignore` - It allows `FileSystemRead` access to the `/tmp/` folder **and** the `/home/.gitignore` path. Wildcards are supported too: diff --git a/lib/internal/process/pre_execution.js b/lib/internal/process/pre_execution.js index 316dac27d37f7c..3f54585dd8eddf 100644 --- a/lib/internal/process/pre_execution.js +++ b/lib/internal/process/pre_execution.js @@ -1,6 +1,7 @@ 'use strict'; const { + ArrayIsArray, ArrayPrototypeForEach, NumberParseInt, ObjectDefineProperties, @@ -550,6 +551,23 @@ function initializePermission() { 'It could invalidate the permission model.', 'SecurityWarning'); } } + const warnCommaFlags = [ + '--allow-fs-read', + '--allow-fs-write', + ]; + for (const flag of warnCommaFlags) { + const value = getOptionValue(flag); + if (value.length === 1 && value[0].includes(',')) { + process.emitWarning( + `the ${flag} CLI flag has recently changed. ` + + 'Passing a comma separated list of files is no longer valid. ' + + `Each file/directory should have its own ${flag} call. ` + + `Example: ${flag}=/folder1/ ${flag}=/folder2/. ` + + 'If the provided value is a single path with commas' + + 'this warning can be safely ignored', 'Warning', + ); + } + } ObjectDefineProperty(process, 'permission', { __proto__: null, @@ -568,7 +586,8 @@ function initializePermission() { '--allow-worker', ]; ArrayPrototypeForEach(availablePermissionFlags, (flag) => { - if (getOptionValue(flag)) { + const value = getOptionValue(flag); + if (ArrayIsArray(value) ? value.length : value) { throw new ERR_MISSING_OPTION('--experimental-permission'); } }); diff --git a/src/env.cc b/src/env.cc index cc0c7b2642c106..4e91278d54f922 100644 --- a/src/env.cc +++ b/src/env.cc @@ -849,12 +849,12 @@ Environment::Environment(IsolateData* isolate_data, // unless explicitly allowed by the user options_->allow_native_addons = false; flags_ = flags_ | EnvironmentFlags::kNoCreateInspector; - permission()->Apply("*", permission::PermissionScope::kInspector); + permission()->Apply({"*"}, permission::PermissionScope::kInspector); if (!options_->allow_child_process) { - permission()->Apply("*", permission::PermissionScope::kChildProcess); + permission()->Apply({"*"}, permission::PermissionScope::kChildProcess); } if (!options_->allow_worker_threads) { - permission()->Apply("*", permission::PermissionScope::kWorkerThreads); + permission()->Apply({"*"}, permission::PermissionScope::kWorkerThreads); } if (!options_->allow_fs_read.empty()) { diff --git a/src/node_options.h b/src/node_options.h index bb8b68894b4430..0a10185bf10537 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -121,8 +121,8 @@ class EnvironmentOptions : public Options { std::string experimental_policy_integrity; bool has_policy_integrity_string = false; bool experimental_permission = false; - std::string allow_fs_read; - std::string allow_fs_write; + std::vector allow_fs_read; + std::vector allow_fs_write; bool allow_child_process = false; bool allow_worker_threads = false; bool experimental_repl_await = true; diff --git a/src/permission/child_process_permission.cc b/src/permission/child_process_permission.cc index 7151eb15f90da2..de078febf4bcd9 100644 --- a/src/permission/child_process_permission.cc +++ b/src/permission/child_process_permission.cc @@ -9,7 +9,7 @@ namespace permission { // Currently, ChildProcess manage a single state // Once denied, it's always denied -void ChildProcessPermission::Apply(const std::string& allow, +void ChildProcessPermission::Apply(const std::vector& allow, PermissionScope scope) { deny_all_ = true; } diff --git a/src/permission/child_process_permission.h b/src/permission/child_process_permission.h index b67169f1c4e180..cf0ec97d5021a3 100644 --- a/src/permission/child_process_permission.h +++ b/src/permission/child_process_permission.h @@ -12,7 +12,8 @@ namespace permission { class ChildProcessPermission final : public PermissionBase { public: - void Apply(const std::string& allow, PermissionScope scope) override; + void Apply(const std::vector& allow, + PermissionScope scope) override; bool is_granted(PermissionScope perm, const std::string_view& param = "") override; diff --git a/src/permission/fs_permission.cc b/src/permission/fs_permission.cc index 91c63dff6582a8..620b97b129d874 100644 --- a/src/permission/fs_permission.cc +++ b/src/permission/fs_permission.cc @@ -1,6 +1,7 @@ #include "fs_permission.h" #include "base_object-inl.h" #include "debug_utils-inl.h" +#include "node_process.h" #include "util.h" #include "v8.h" @@ -116,9 +117,11 @@ namespace permission { // allow = '*' // allow = '/tmp/,/home/example.js' -void FSPermission::Apply(const std::string& allow, PermissionScope scope) { +void FSPermission::Apply(const std::vector& allow, + PermissionScope scope) { using std::string_view_literals::operator""sv; - for (const std::string_view res : SplitString(allow, ","sv)) { + + for (const std::string_view res : allow) { if (res == "*"sv) { if (scope == PermissionScope::kFileSystemRead) { deny_all_in_ = false; diff --git a/src/permission/fs_permission.h b/src/permission/fs_permission.h index 217d0a92d6ce71..244e95727ad487 100644 --- a/src/permission/fs_permission.h +++ b/src/permission/fs_permission.h @@ -15,7 +15,8 @@ namespace permission { class FSPermission final : public PermissionBase { public: - void Apply(const std::string& allow, PermissionScope scope) override; + void Apply(const std::vector& allow, + PermissionScope scope) override; bool is_granted(PermissionScope perm, const std::string_view& param) override; struct RadixTree { diff --git a/src/permission/inspector_permission.cc b/src/permission/inspector_permission.cc index 3cff03433b4225..401d801ac0adb5 100644 --- a/src/permission/inspector_permission.cc +++ b/src/permission/inspector_permission.cc @@ -8,7 +8,7 @@ namespace permission { // Currently, Inspector manage a single state // Once denied, it's always denied -void InspectorPermission::Apply(const std::string& allow, +void InspectorPermission::Apply(const std::vector& allow, PermissionScope scope) { deny_all_ = true; } diff --git a/src/permission/inspector_permission.h b/src/permission/inspector_permission.h index 33eb25732c0d4d..e5c6d1b81677f5 100644 --- a/src/permission/inspector_permission.h +++ b/src/permission/inspector_permission.h @@ -12,7 +12,8 @@ namespace permission { class InspectorPermission final : public PermissionBase { public: - void Apply(const std::string& allow, PermissionScope scope) override; + void Apply(const std::vector& allow, + PermissionScope scope) override; bool is_granted(PermissionScope perm, const std::string_view& param = "") override; diff --git a/src/permission/permission.cc b/src/permission/permission.cc index 38767e46093f0b..4392f49b66e9b7 100644 --- a/src/permission/permission.cc +++ b/src/permission/permission.cc @@ -130,7 +130,8 @@ void Permission::EnablePermissions() { } } -void Permission::Apply(const std::string& allow, PermissionScope scope) { +void Permission::Apply(const std::vector& allow, + PermissionScope scope) { auto permission = nodes_.find(scope); if (permission != nodes_.end()) { permission->second->Apply(allow, scope); diff --git a/src/permission/permission.h b/src/permission/permission.h index 3252e8d540d306..942937a80cae28 100644 --- a/src/permission/permission.h +++ b/src/permission/permission.h @@ -49,7 +49,7 @@ class Permission { const std::string_view& res); // CLI Call - void Apply(const std::string& allow, PermissionScope scope); + void Apply(const std::vector& allow, PermissionScope scope); void EnablePermissions(); private: diff --git a/src/permission/permission_base.h b/src/permission/permission_base.h index c4728e40ce8f2c..c2f377424f6fc5 100644 --- a/src/permission/permission_base.h +++ b/src/permission/permission_base.h @@ -39,7 +39,8 @@ enum class PermissionScope { class PermissionBase { public: - virtual void Apply(const std::string& allow, PermissionScope scope) = 0; + virtual void Apply(const std::vector& allow, + PermissionScope scope) = 0; virtual bool is_granted(PermissionScope perm, const std::string_view& param = "") = 0; }; diff --git a/src/permission/worker_permission.cc b/src/permission/worker_permission.cc index 69c89a4a4fea87..a18938e5fe1efd 100644 --- a/src/permission/worker_permission.cc +++ b/src/permission/worker_permission.cc @@ -9,7 +9,8 @@ namespace permission { // Currently, PolicyDenyWorker manage a single state // Once denied, it's always denied -void WorkerPermission::Apply(const std::string& allow, PermissionScope scope) { +void WorkerPermission::Apply(const std::vector& allow, + PermissionScope scope) { deny_all_ = true; } diff --git a/src/permission/worker_permission.h b/src/permission/worker_permission.h index 71681a4485a82f..cdc224925c2291 100644 --- a/src/permission/worker_permission.h +++ b/src/permission/worker_permission.h @@ -12,7 +12,8 @@ namespace permission { class WorkerPermission final : public PermissionBase { public: - void Apply(const std::string& allow, PermissionScope scope) override; + void Apply(const std::vector& allow, + PermissionScope scope) override; bool is_granted(PermissionScope perm, const std::string_view& param = "") override; diff --git a/test/parallel/test-cli-permission-multiple-allow.js b/test/parallel/test-cli-permission-multiple-allow.js new file mode 100644 index 00000000000000..ee7d3f05bc75ec --- /dev/null +++ b/test/parallel/test-cli-permission-multiple-allow.js @@ -0,0 +1,84 @@ +'use strict'; + +require('../common'); + +const { spawnSync } = require('child_process'); +const assert = require('assert'); +const path = require('path'); + +{ + const tmpPath = path.resolve('/tmp/'); + const otherPath = path.resolve('/other-path/'); + const { status, stdout } = spawnSync( + process.execPath, + [ + '--experimental-permission', + '--allow-fs-write', tmpPath, '--allow-fs-write', otherPath, '-e', + `console.log(process.permission.has("fs")); + console.log(process.permission.has("fs.read")); + console.log(process.permission.has("fs.write")); + console.log(process.permission.has("fs.write", "/tmp/")); + console.log(process.permission.has("fs.write", "/other-path/"));`, + ] + ); + const [fs, fsIn, fsOut, fsOutAllowed1, fsOutAllowed2] = stdout.toString().split('\n'); + assert.strictEqual(fs, 'false'); + assert.strictEqual(fsIn, 'false'); + assert.strictEqual(fsOut, 'false'); + assert.strictEqual(fsOutAllowed1, 'true'); + assert.strictEqual(fsOutAllowed2, 'true'); + assert.strictEqual(status, 0); +} + +{ + const tmpPath = path.resolve('/tmp/'); + const pathWithComma = path.resolve('/other,path/'); + const { status, stdout } = spawnSync( + process.execPath, + [ + '--experimental-permission', + '--allow-fs-write', + tmpPath, + '--allow-fs-write', + pathWithComma, + '-e', + `console.log(process.permission.has("fs")); + console.log(process.permission.has("fs.read")); + console.log(process.permission.has("fs.write")); + console.log(process.permission.has("fs.write", "/tmp/")); + console.log(process.permission.has("fs.write", "/other,path/"));`, + ] + ); + const [fs, fsIn, fsOut, fsOutAllowed1, fsOutAllowed2] = stdout.toString().split('\n'); + assert.strictEqual(fs, 'false'); + assert.strictEqual(fsIn, 'false'); + assert.strictEqual(fsOut, 'false'); + assert.strictEqual(fsOutAllowed1, 'true'); + assert.strictEqual(fsOutAllowed2, 'true'); + assert.strictEqual(status, 0); +} + +{ + const filePath = path.resolve('/tmp/file,with,comma.txt'); + const { status, stdout, stderr } = spawnSync( + process.execPath, + [ + '--experimental-permission', + '--allow-fs-read=*', + `--allow-fs-write=${filePath}`, + '-e', + `console.log(process.permission.has("fs")); + console.log(process.permission.has("fs.read")); + console.log(process.permission.has("fs.write")); + console.log(process.permission.has("fs.write", "/tmp/file,with,comma.txt"));`, + ] + ); + const res = stdout.toString(); + const [fs, fsIn, fsOut, fsOutAllowed] = res.split('\n'); + assert.strictEqual(fs, 'false'); + assert.strictEqual(fsIn, 'true'); + assert.strictEqual(fsOut, 'false'); + assert.strictEqual(fsOutAllowed, 'true'); + assert.strictEqual(status, 0); + assert.ok(stderr.toString().includes('Warning: the --allow-fs-write CLI flag has recently changed')); +} diff --git a/test/parallel/test-permission-fs-read.js b/test/parallel/test-permission-fs-read.js index 010a5932c4eae1..5be993c9df6be5 100644 --- a/test/parallel/test-permission-fs-read.js +++ b/test/parallel/test-permission-fs-read.js @@ -28,7 +28,7 @@ const commonPath = path.join(__filename, '../../common'); const { status, stderr } = spawnSync( process.execPath, [ - '--experimental-permission', `--allow-fs-read=${file},${commonPathWildcard}`, file, + '--experimental-permission', `--allow-fs-read=${file}`, `--allow-fs-read=${commonPathWildcard}`, file, ], { env: { diff --git a/test/parallel/test-permission-fs-symlink-target-write.js b/test/parallel/test-permission-fs-symlink-target-write.js index f6a99fe06c9d1f..73075a319f47d6 100644 --- a/test/parallel/test-permission-fs-symlink-target-write.js +++ b/test/parallel/test-permission-fs-symlink-target-write.js @@ -36,7 +36,7 @@ fs.writeFileSync(path.join(readWriteFolder, 'file'), 'NO evil file contents'); process.execPath, [ '--experimental-permission', - `--allow-fs-read=${file},${commonPathWildcard},${readOnlyFolder},${readWriteFolder}`, + `--allow-fs-read=${file},${commonPathWildcard}`, `--allow-fs-read=${readOnlyFolder}`, `--allow-fs-read=${readWriteFolder}`, `--allow-fs-write=${readWriteFolder},${writeOnlyFolder}`, file, ], diff --git a/test/parallel/test-permission-fs-symlink.js b/test/parallel/test-permission-fs-symlink.js index 96d4fdad4d2d4b..0e29320044c698 100644 --- a/test/parallel/test-permission-fs-symlink.js +++ b/test/parallel/test-permission-fs-symlink.js @@ -37,7 +37,7 @@ const symlinkFromBlockedFile = path.join(tmpdir.path, 'example-symlink.md'); process.execPath, [ '--experimental-permission', - `--allow-fs-read=${file},${commonPathWildcard},${symlinkFromBlockedFile}`, + `--allow-fs-read=${file}`, `--allow-fs-read=${commonPathWildcard}`, `--allow-fs-read=${symlinkFromBlockedFile}`, `--allow-fs-write=${symlinkFromBlockedFile}`, file, ], diff --git a/test/parallel/test-permission-fs-traversal-path.js b/test/parallel/test-permission-fs-traversal-path.js index 2b294a4574e2f3..b12f8ce2ff1c02 100644 --- a/test/parallel/test-permission-fs-traversal-path.js +++ b/test/parallel/test-permission-fs-traversal-path.js @@ -31,7 +31,7 @@ const commonPathWildcard = path.join(__filename, '../../common*'); process.execPath, [ '--experimental-permission', - `--allow-fs-read=${file},${commonPathWildcard},${allowedFolder}`, + `--allow-fs-read=${file}`, `--allow-fs-read=${commonPathWildcard}`, `--allow-fs-read=${allowedFolder}`, `--allow-fs-write=${allowedFolder}`, file, ], diff --git a/test/parallel/test-permission-fs-wildcard.js b/test/parallel/test-permission-fs-wildcard.js index 5b0dc411666013..66f49932dec5ec 100644 --- a/test/parallel/test-permission-fs-wildcard.js +++ b/test/parallel/test-permission-fs-wildcard.js @@ -32,7 +32,7 @@ if (common.isWindows) { process.execPath, [ '--experimental-permission', - `--allow-fs-read=${allowList.join(',')}`, + allowList.map((path) => `--allow-fs-read=${path}`).join(' '), '-e', ` const path = require('path'); @@ -67,7 +67,7 @@ if (common.isWindows) { process.execPath, [ '--experimental-permission', - `--allow-fs-read=${allowList.join(',')}`, + allowList.map((path) => `--allow-fs-read=${path}`).join(' '), '-e', ` const assert = require('assert') @@ -92,7 +92,7 @@ if (common.isWindows) { process.execPath, [ '--experimental-permission', - `--allow-fs-read=${file},${commonPathWildcard},${allowList.join(',')}`, + `--allow-fs-read=${file}`, `--allow-fs-read=${commonPathWildcard}`, `--allow-fs-read=${allowList.join(',')}`, file, ], );