Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src,process: initial permission model implementation #44004

Merged
merged 1 commit into from
Feb 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 72 additions & 0 deletions benchmark/fs/readfile-permission-enabled.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
// Call fs.readFile with permission system enabled
// over and over again really fast.
// Then see how many times it got called.
'use strict';

const path = require('path');
const common = require('../common.js');
const fs = require('fs');
const assert = require('assert');

const tmpdir = require('../../test/common/tmpdir');
tmpdir.refresh();
const filename = path.resolve(tmpdir.path,
`.removeme-benchmark-garbage-${process.pid}`);

const bench = common.createBenchmark(main, {
duration: [5],
encoding: ['', 'utf-8'],
len: [1024, 16 * 1024 * 1024],
concurrent: [1, 10],
}, {
flags: ['--experimental-permission', '--allow-fs-read=*', '--allow-fs-write=*'],
});

function main({ len, duration, concurrent, encoding }) {
try {
fs.unlinkSync(filename);
} catch {
// Continue regardless of error.
}
let data = Buffer.alloc(len, 'x');
fs.writeFileSync(filename, data);
data = null;

let reads = 0;
let benchEnded = false;
bench.start();
setTimeout(() => {
benchEnded = true;
bench.end(reads);
try {
fs.unlinkSync(filename);
} catch {
// Continue regardless of error.
}
process.exit(0);
}, duration * 1000);

function read() {
fs.readFile(filename, encoding, afterRead);
}

function afterRead(er, data) {
if (er) {
if (er.code === 'ENOENT') {
// Only OK if unlinked by the timer from main.
assert.ok(benchEnded);
return;
}
throw er;
}

if (data.length !== len)
throw new Error('wrong number of bytes returned');

reads++;
if (!benchEnded)
read();
}

while (concurrent--) read();
}
19 changes: 19 additions & 0 deletions benchmark/permission/permission-fs-deny.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
'use strict';
const common = require('../common.js');

const configs = {
n: [1e5],
concurrent: [1, 10],
};

const options = { flags: ['--experimental-permission'] };

const bench = common.createBenchmark(main, configs, options);

async function main(conf) {
bench.start();
for (let i = 0; i < conf.n; i++) {
process.permission.deny('fs.read', ['/home/example-file-' + i]);
}
bench.end(conf.n);
}
50 changes: 50 additions & 0 deletions benchmark/permission/permission-fs-is-granted.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
'use strict';
const common = require('../common.js');
const fs = require('fs/promises');
const path = require('path');

const configs = {
n: [1e5],
concurrent: [1, 10],
};

const rootPath = path.resolve(__dirname, '../../..');

const options = {
flags: [
'--experimental-permission',
`--allow-fs-read=${rootPath}`,
],
};

const bench = common.createBenchmark(main, configs, options);

const recursivelyDenyFiles = async (dir) => {
const files = await fs.readdir(dir, { withFileTypes: true });
for (const file of files) {
if (file.isDirectory()) {
await recursivelyDenyFiles(path.join(dir, file.name));
} else if (file.isFile()) {
process.permission.deny('fs.read', [path.join(dir, file.name)]);
}
}
};

async function main(conf) {
const benchmarkDir = path.join(__dirname, '../..');
// Get all the benchmark files and deny access to it
await recursivelyDenyFiles(benchmarkDir);

bench.start();

for (let i = 0; i < conf.n; i++) {
// Valid file in a sequence of denied files
process.permission.has('fs.read', benchmarkDir + '/valid-file');
// Denied file
process.permission.has('fs.read', __filename);
// Valid file a granted directory
process.permission.has('fs.read', '/tmp/example');
}

bench.end(conf.n);
}
169 changes: 169 additions & 0 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,154 @@ If this flag is passed, the behavior can still be set to not abort through
[`process.setUncaughtExceptionCaptureCallback()`][] (and through usage of the
`node:domain` module that uses it).

### `--allow-child-process`

<!-- YAML
added: REPLACEME
-->

> Stability: 1 - Experimental

When using the [Permission Model][], the process will not be able to spawn any
child process by default.
Attempts to do so will throw an `ERR_ACCESS_DENIED` unless the
user explicitly passes the `--allow-child-process` flag when starting Node.js.
RafaelGSS marked this conversation as resolved.
Show resolved Hide resolved

Example:

```js
const childProcess = require('node:child_process');
// Attempt to bypass the permission
childProcess.spawn('node', ['-e', 'require("fs").writeFileSync("/new-file", "example")']);
```

```console
$ node --experimental-permission --allow-fs-read=* index.js
node:internal/child_process:388
const err = this._handle.spawn(options);
^
Error: Access to this API has been restricted
at ChildProcess.spawn (node:internal/child_process:388:28)
at Object.spawn (node:child_process:723:9)
at Object.<anonymous> (/home/index.js:3:14)
at Module._compile (node:internal/modules/cjs/loader:1120:14)
at Module._extensions..js (node:internal/modules/cjs/loader:1174:10)
at Module.load (node:internal/modules/cjs/loader:998:32)
at Module._load (node:internal/modules/cjs/loader:839:12)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
at node:internal/main/run_main_module:17:47 {
Comment on lines +130 to +138
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should remove the stack traces from the errors in the docs. It is low value noise that is not likely to age well IMO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking to reduce it to:

Error: Access to this API has been restricted
    at ChildProcess.spawn (node:internal/child_process:388:28)
    at Object.spawn (node:child_process:723:9)
    at node:internal/main/run_main_module:17:47 {
  code: 'ERR_ACCESS_DENIED',
  permission: 'ChildProcess'
}

I think they are useful for SEO. People copy and paste part of the stack trace on google and so on.

code: 'ERR_ACCESS_DENIED',
permission: 'ChildProcess'
}
```

### `--allow-fs-read`

<!-- YAML
added: REPLACEME
-->

> Stability: 1 - Experimental

This flag configures file system read permissions using
the [Permission Model][].

The valid arguments for the `--allow-fs-read` flag are:

* `*` - To allow the `FileSystemRead` operations.
* Paths delimited by comma (,) to manage `FileSystemRead` (reading) operations.

Examples can be found in the [File System Permissions][] documentation.

Relative paths are NOT yet supported by the CLI flag.

The initializer module also needs to be allowed. Consider the following example:

```console
$ node --experimental-permission t.js
node:internal/modules/cjs/loader:162
const result = internalModuleStat(filename);
^

Error: Access to this API has been restricted
at stat (node:internal/modules/cjs/loader:162:18)
at Module._findPath (node:internal/modules/cjs/loader:640:16)
at resolveMainPath (node:internal/modules/run_main:15:25)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:53:24)
at node:internal/main/run_main_module:23:47 {
code: 'ERR_ACCESS_DENIED',
permission: 'FileSystemRead',
resource: '/Users/rafaelgss/repos/os/node/t.js'
}
```

The process needs to have access to the `index.js` module:

```console
$ node --experimental-permission --allow-fs-read=/path/to/index.js index.js
```

### `--allow-fs-write`

<!-- YAML
added: REPLACEME
-->

> Stability: 1 - Experimental

This flag configures file system write permissions using
the [Permission Model][].

The valid arguments for the `--allow-fs-write` flag are:

* `*` - To allow the `FileSystemWrite` operations.
* Paths delimited by comma (,) to manage `FileSystemWrite` (writing) operations.

Examples can be found in the [File System Permissions][] documentation.

Relative paths are NOT supported through the CLI flag.

### `--allow-worker`

<!-- YAML
added: REPLACEME
-->

> Stability: 1 - Experimental

When using the [Permission Model][], the process will not be able to create any
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comments here as in the child process section above.

worker threads by default.
For security reasons, the call will throw an `ERR_ACCESS_DENIED` unless the
user explicitly pass the flag `--allow-worker` in the main Node.js process.

Example:

```js
const { Worker } = require('node:worker_threads');
// Attempt to bypass the permission
new Worker(__filename);
```

```console
$ node --experimental-permission --allow-fs-read=* index.js
RafaelGSS marked this conversation as resolved.
Show resolved Hide resolved
node:internal/worker:188
this[kHandle] = new WorkerImpl(url,
^

Error: Access to this API has been restricted
at new Worker (node:internal/worker:188:21)
at Object.<anonymous> (/home/index.js.js:3:1)
at Module._compile (node:internal/modules/cjs/loader:1120:14)
at Module._extensions..js (node:internal/modules/cjs/loader:1174:10)
at Module.load (node:internal/modules/cjs/loader:998:32)
at Module._load (node:internal/modules/cjs/loader:839:12)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
at node:internal/main/run_main_module:17:47 {
code: 'ERR_ACCESS_DENIED',
permission: 'WorkerThreads'
}
```

### `--build-snapshot`

<!-- YAML
Expand Down Expand Up @@ -386,6 +534,20 @@ added:

Enable experimental support for the `https:` protocol in `import` specifiers.

### `--experimental-permission`

<!-- YAML
added: REPLACEME
-->

Enable the Permission Model for current process. When enabled, the
following permissions are restricted:

* File System - manageable through
\[`--allow-fs-read`]\[],\[`allow-fs-write`]\[] flags
* Child Process - manageable through \[`--allow-child-process`]\[] flag
* Worker Threads - manageable through \[`--allow-worker`]\[] flag
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about native add-ons? Those should also be disabled by default

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I raised my concerns about a flag --allow-addons here.

Also, as previously mentioned by @richardlau, we do have a flag --no-addons (see #44004 (comment)). Perhaps we can just mention it in permissions.md?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also enable this flag (--no-addons) when using the permission model unless a --allow-addons is passed. But, I think it might be confusing for the users.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it would be confusing as long as we document it accordingly, but it doesn't have to happen in this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really believe we need to disable native addons by default when permissions are enabled, even if there currently is no way to re-enable them right away. It is too easy to bypass any permissions model using a native addon and allowing them opens a major hole in the model.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was considering doing it in a separate PR. Anyway, I think I'll follow the approach: --no-addons by default unless --allow-addons is passed. Do you believe it should land in this PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I do. At least the setting --no-addons by default bit. The --allow-addons can be added separately


### `--experimental-policy`

<!-- YAML
Expand Down Expand Up @@ -1883,6 +2045,10 @@ Node.js options that are allowed are:

<!-- node-options-node start -->

* `--allow-child-process`
* `--allow-fs-read`
* `--allow-fs-write`
* `--allow-worker`
* `--conditions`, `-C`
* `--diagnostic-dir`
* `--disable-proto`
Expand All @@ -1896,6 +2062,7 @@ Node.js options that are allowed are:
* `--experimental-loader`
* `--experimental-modules`
* `--experimental-network-imports`
* `--experimental-permission`
* `--experimental-policy`
* `--experimental-shadow-realm`
* `--experimental-specifier-resolution`
Expand Down Expand Up @@ -2331,9 +2498,11 @@ done
[ECMAScript module]: esm.md#modules-ecmascript-modules
[ECMAScript module loader]: esm.md#loaders
[Fetch API]: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API
[File System Permissions]: permissions.md#file-system-permissions
[Modules loaders]: packages.md#modules-loaders
[Node.js issue tracker]: https://github.com/nodejs/node/issues
[OSSL_PROVIDER-legacy]: https://www.openssl.org/docs/man3.0/man7/OSSL_PROVIDER-legacy.html
[Permission Model]: permissions.md#permission-model
[REPL]: repl.md
[ScriptCoverage]: https://chromedevtools.github.io/devtools-protocol/tot/Profiler#type-ScriptCoverage
[ShadowRealm]: https://github.com/tc39/proposal-shadowrealm
Expand Down
8 changes: 8 additions & 0 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -679,6 +679,13 @@ APIs _not_ using `AbortSignal`s typically do not raise an error with this code.
This code does not use the regular `ERR_*` convention Node.js errors use in
order to be compatible with the web platform's `AbortError`.

<a id="ERR_ACCESS_DENIED"></a>

### `ERR_ACCESS_DENIED`
RafaelGSS marked this conversation as resolved.
Show resolved Hide resolved

A special type of error that is triggered whenever Node.js tries to get access
to a resource restricted by the [Permission Model][].

<a id="ERR_AMBIGUOUS_ARGUMENT"></a>

### `ERR_AMBIGUOUS_ARGUMENT`
Expand Down Expand Up @@ -3542,6 +3549,7 @@ The native call from `process.cpuUsage` could not be processed.
[JSON Web Key Elliptic Curve Registry]: https://www.iana.org/assignments/jose/jose.xhtml#web-key-elliptic-curve
[JSON Web Key Types Registry]: https://www.iana.org/assignments/jose/jose.xhtml#web-key-types
[Node.js error codes]: #nodejs-error-codes
[Permission Model]: permissions.md#permission-model
[RFC 7230 Section 3]: https://tools.ietf.org/html/rfc7230#section-3
[Subresource Integrity specification]: https://www.w3.org/TR/SRI/#the-integrity-attribute
[V8's stack trace API]: https://v8.dev/docs/stack-trace-api
Expand Down
Loading