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: nodejs/security-wg#1039
Ceres6 · Aug 6, 2023 · c569a23 · c569a23
1 parent a973446
commit c569a23
Show file tree

Hide file tree

Showing 22 changed files with 149 additions and 30 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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<std::string> allow_fs_read;
+  std::vector<std::string> 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
@@ -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<std::string>& allow,
                                    PermissionScope scope) {
   deny_all_ = true;
 }

diff --git 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<std::string>& 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
@@ -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<std::string>& 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
@@ -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<std::string>& 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
@@ -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<std::string>& allow,
                                 PermissionScope scope) {
   deny_all_ = true;
 }

diff --git 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<std::string>& 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
@@ -130,7 +130,8 @@ void Permission::EnablePermissions() {
   }
 }
 
-void Permission::Apply(const std::string& allow, PermissionScope scope) {
+void Permission::Apply(const std::vector<std::string>& 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
@@ -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<std::string>& allow, PermissionScope scope);
   void EnablePermissions();
 
  private:

diff --git 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<std::string>& 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
@@ -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<std::string>& allow,
+                             PermissionScope scope) {
   deny_all_ = true;
 }
 

diff --git 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<std::string>& 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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,
     ],
   );