From 413c16e49069974ac6fcd6a47b05ec97e1218540 Mon Sep 17 00:00:00 2001 From: Carlos Espa <43477095+Ceres6@users.noreply.github.com> Date: Thu, 17 Aug 2023 20:39:04 +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 PR-URL: https://github.com/nodejs/node/pull/49047 Reviewed-By: Rafael Gonzaga Reviewed-By: Marco Ippolito --- doc/api/cli.md | 22 ++++- doc/api/permissions.md | 2 +- lib/internal/process/pre_execution.js | 19 ++++- 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 | 6 +- 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-cjs-legacyMainResolve-permission.js | 8 +- .../test-cli-permission-multiple-allow.js | 83 +++++++++++++++++++ test/parallel/test-permission-fs-read.js | 2 +- ...test-permission-fs-symlink-target-write.js | 4 +- test/parallel/test-permission-fs-symlink.js | 2 +- .../test-permission-fs-traversal-path.js | 2 +- test/parallel/test-permission-fs-wildcard.js | 6 +- test/parallel/test-permission-fs-write.js | 2 +- 24 files changed, 161 insertions(+), 34 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 6cd0a795c216e7..b5346f949d14fa 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -145,6 +145,10 @@ Error: Access to this API has been restricted > Stability: 1 - Experimental @@ -155,8 +159,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/` + +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. @@ -192,6 +199,10 @@ node --experimental-permission --allow-fs-read=/path/to/index.js index.js > Stability: 1 - Experimental @@ -202,8 +213,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/` + +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 d79aa41c53e7b6..89b916a6ee66ae 100644 --- a/lib/internal/process/pre_execution.js +++ b/lib/internal/process/pre_execution.js @@ -554,6 +554,22 @@ 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 changed. ` + + 'Passing a comma-separated list of paths is no longer valid. ' + + 'Documentation can be found at ' + + 'https://nodejs.org/api/permissions.html#file-system-permissions', + 'Warning', + ); + } + } ObjectDefineProperty(process, 'permission', { __proto__: null, @@ -572,7 +588,8 @@ function initializePermission() { '--allow-worker', ]; ArrayPrototypeForEach(availablePermissionFlags, (flag) => { - if (getOptionValue(flag)) { + const value = getOptionValue(flag); + if (value.length) { throw new ERR_MISSING_OPTION('--experimental-permission'); } }); diff --git a/src/env.cc b/src/env.cc index 20b404996ccd0f..1d8df40c3446ac 100644 --- a/src/env.cc +++ b/src/env.cc @@ -875,12 +875,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 1cc575bb9c7e3c..bc18a45e681a3c 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..fadf75968c779d 100644 --- a/src/permission/fs_permission.cc +++ b/src/permission/fs_permission.cc @@ -116,9 +116,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/es-module/test-cjs-legacyMainResolve-permission.js b/test/es-module/test-cjs-legacyMainResolve-permission.js index 9f78883eebb7cd..b45e3dee3bbfcd 100644 --- a/test/es-module/test-cjs-legacyMainResolve-permission.js +++ b/test/es-module/test-cjs-legacyMainResolve-permission.js @@ -31,7 +31,9 @@ describe('legacyMainResolve', () => { for (const [mainOrFolder, allowReads] of paths) { const allowReadFilePaths = allowReads.map((filepath) => path.resolve(fixtextureFolder, filepath)); - const allowReadFiles = allowReads?.length > 0 ? ['--allow-fs-read', allowReadFilePaths.join(',')] : []; + const allowReadFiles = allowReads?.length > 0 ? + allowReadFilePaths.flatMap((path) => ['--allow-fs-read', path]) : + []; const fixtextureFolderEscaped = escapeWhenSepIsBackSlash(fixtextureFolder); const { status, stderr } = spawnSync( @@ -85,7 +87,9 @@ describe('legacyMainResolve', () => { for (const [folder, expectedFile, allowReads] of paths) { const allowReadFilePaths = allowReads.map((filepath) => path.resolve(fixtextureFolder, folder, filepath)); - const allowReadFiles = allowReads?.length > 0 ? ['--allow-fs-read', allowReadFilePaths.join(',')] : []; + const allowReadFiles = allowReads?.length > 0 ? + allowReadFilePaths.flatMap((path) => ['--allow-fs-read', path]) : + []; const fixtextureFolderEscaped = escapeWhenSepIsBackSlash(fixtextureFolder); const { status, stderr } = spawnSync( 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..68e9029b1ddb45 --- /dev/null +++ b/test/parallel/test-cli-permission-multiple-allow.js @@ -0,0 +1,83 @@ +'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 [fs, fsIn, fsOut, fsOutAllowed] = stdout.toString().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 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 55251cb163b126..e2b4aa2a657442 100644 --- a/test/parallel/test-permission-fs-symlink-target-write.js +++ b/test/parallel/test-permission-fs-symlink-target-write.js @@ -36,8 +36,8 @@ fs.writeFileSync(path.join(readWriteFolder, 'file'), 'NO evil file contents'); process.execPath, [ '--experimental-permission', - `--allow-fs-read=${file},${commonPathWildcard},${readOnlyFolder},${readWriteFolder}`, - `--allow-fs-write=${readWriteFolder},${writeOnlyFolder}`, + `--allow-fs-read=${file}`, `--allow-fs-read=${commonPathWildcard}`, `--allow-fs-read=${readOnlyFolder}`, `--allow-fs-read=${readWriteFolder}`, + `--allow-fs-write=${readWriteFolder}`, `--allow-fs-write=${writeOnlyFolder}`, file, ], { diff --git a/test/parallel/test-permission-fs-symlink.js b/test/parallel/test-permission-fs-symlink.js index 808965001accd5..c7d753c267c1e7 100644 --- a/test/parallel/test-permission-fs-symlink.js +++ b/test/parallel/test-permission-fs-symlink.js @@ -37,7 +37,7 @@ const symlinkFromBlockedFile = tmpdir.resolve('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 b84bed65db3b0a..547cd81c77cf18 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..0c81ff5da51b87 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.flatMap((path) => ['--allow-fs-read', path]), '-e', ` const path = require('path'); @@ -67,7 +67,7 @@ if (common.isWindows) { process.execPath, [ '--experimental-permission', - `--allow-fs-read=${allowList.join(',')}`, + ...allowList.flatMap((path) => ['--allow-fs-read', path]), '-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}`, ...allowList.flatMap((path) => ['--allow-fs-read', path]), file, ], ); diff --git a/test/parallel/test-permission-fs-write.js b/test/parallel/test-permission-fs-write.js index 9f257df86f8672..626c00e5c007a2 100644 --- a/test/parallel/test-permission-fs-write.js +++ b/test/parallel/test-permission-fs-write.js @@ -26,7 +26,7 @@ const file = fixtures.path('permission', 'fs-write.js'); [ '--experimental-permission', '--allow-fs-read=*', - `--allow-fs-write=${regularFile},${commonPath}`, + `--allow-fs-write=${regularFile}`, `--allow-fs-write=${commonPath}`, file, ], {