Skip to content

Commit

Permalink
src,permission: add multiple allow-fs-* flags
Browse files Browse the repository at this point in the history
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
PR-URL: #49047
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
  • Loading branch information
Ceres6 authored and UlisesGascon committed Sep 10, 2023
1 parent ceaa549 commit ec51e25
Show file tree
Hide file tree
Showing 24 changed files with 161 additions and 34 deletions.
22 changes: 18 additions & 4 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,10 @@ Error: Access to this API has been restricted

<!-- YAML
added: v20.0.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/49047
description: Paths delimited by comma (`,`) are no longer allowed.
-->

> Stability: 1 - Experimental
Expand All @@ -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.

Expand Down Expand Up @@ -192,6 +199,10 @@ node --experimental-permission --allow-fs-read=/path/to/index.js index.js

<!-- YAML
added: v20.0.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/49047
description: Paths delimited by comma (`,`) are no longer allowed.
-->

> Stability: 1 - Experimental
Expand All @@ -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.

Expand Down
2 changes: 1 addition & 1 deletion doc/api/permissions.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
19 changes: 18 additions & 1 deletion lib/internal/process/pre_execution.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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');
}
});
Expand Down
6 changes: 3 additions & 3 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down
4 changes: 2 additions & 2 deletions src/node_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/permission/child_process_permission.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
3 changes: 2 additions & 1 deletion src/permission/child_process_permission.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
6 changes: 4 additions & 2 deletions src/permission/fs_permission.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<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;
Expand Down
3 changes: 2 additions & 1 deletion src/permission/fs_permission.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion src/permission/inspector_permission.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
3 changes: 2 additions & 1 deletion src/permission/inspector_permission.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
3 changes: 2 additions & 1 deletion src/permission/permission.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/permission/permission.h
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
3 changes: 2 additions & 1 deletion src/permission/permission_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Expand Down
3 changes: 2 additions & 1 deletion src/permission/worker_permission.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
3 changes: 2 additions & 1 deletion src/permission/worker_permission.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
8 changes: 6 additions & 2 deletions test/es-module/test-cjs-legacyMainResolve-permission.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down
83 changes: 83 additions & 0 deletions test/parallel/test-cli-permission-multiple-allow.js
Original file line number Diff line number Diff line change
@@ -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.'));
}
2 changes: 1 addition & 1 deletion test/parallel/test-permission-fs-read.js
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-permission-fs-symlink-target-write.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
],
{
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-permission-fs-symlink.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
],
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-permission-fs-traversal-path.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
],
Expand Down
Loading

0 comments on commit ec51e25

Please sign in to comment.