src,process: initial permission model implementation

[RafaelGSS]

Reference issue: nodejs/security-wg#791

The tagged issue contains the initial proposal for this MVP. This Pull Request includes the foundation of the Permission Model.

---

Constraints

This Permission Model is not bulletproof, which means, there are constraints we agree on before landing this system:

• It’s not a sandbox, we assume the user trusts in the running code.
• No break-changes are ideal.
• It must add a low/no overhead when disabled and low overhead when enabled.

API Design

The Node.js Permission Model is a mechanism to restrict access to specific resources during the program execution. The API exists behind a flag --experimental-permission which when enabled, will restrict access to all available permissions.

Currently, the available permissions are:

• File System - manageable through --allow-fs-read and --allow-fs-write flags
• Child Process - manageable through --allow-child-process flag
• Worker Threads - manageable through --allow-worker flag

Therefore, when starting a Node.js process with --experimental-permission,
the ability to access the filesystem, spawn process and,
create worker_threads will be restricted.

The CLI Arguments

To allow access to the filesystem, use the --allow-fs-read and
--allow-fs-write flags:

The valid arguments for both flags are:

• * - To allow all operations to given scope (read/write).
• Paths delimited by comma (,) to manage reading/writing operations.

Example:

• --allow-fs-read=/tmp/ - It will allow FileSystemRead access to the /tmp/ folder
• --allow-fs-read=/tmp/,/home/.gitignore - It allows FileSystemRead access to the /tmp/ folder and the /home/.gitignore file — Relative paths are NOT supported.

Due to the PrefixRadixTree (fs_permission) lookup, relative paths are not supported. For this reason, the possiblyTransformPath was needed. I do have plans to create a pretty similar path.resolve on the C++ side so the possiblyTransformPath won't be needed, but I'll do it in a second iteration.

You can also mix both arguments:

• --allow-fs-write=* --allow-fs-read=/tmp/ - It will allow FileSystemRead access to the /tmp/ folder and allow all the FileSystemWrite operations.
  Note: It accepts wildcard parameters as well. For instance: --allow-fs-write=/home/test* will allow everything that matches the wildcard. e.g: /home/test/file1 / /home/test2

Note: I rather prefer reading those arguments from a file (policy-deny.json), instead passing them in the command line. However, to reduce the PR scope, I've decided to do it in a separate PR.

The API Arguments

A new property permission was added to the process module. The property contains two functions:

• deny(scope [,parameters])

API Call to deny permissions at runtime. e.g (REMOVED)

process.permission.deny('fs') // deny permissions to ALL fs operations

process.permission.deny('fs.out') // deny permissions to ALL FileSystemWrite operations
process.permission.deny('fs.out', '/home/rafaelgss/protected-folder') // deny FileSystemWrite permissions to the protected-folder
process.permission.deny('fs.in') // deny permissions to ALL FileSystemRead operations
process.permission.deny('fs.in', '/home/rafaelgss/protected-folder') // deny FileSystemRead permissions to the protected-folder

• has(scope [,parameters])

API Call to check permissions at runtime. e.g:

process.permission.has('fs.out') // true
process.permission.has('fs.out', '/home/rafaelgss/protected-folder') // true

process.permission.deny('fs.out', '/home/rafaelgss/protected-folder')

process.permission.has('fs.out') // true
process.permission.has('fs.out', '/home/rafaelgss/protected-folder') // false

Future implementations

The implementation of the next features like “net” or “env” will be easily possible just by creating a new net_permission.h and implementing the PermissionBase methods.

FAQ

The user should be able to grant permissions in runtime?

No. Much like with other well-known and well-used permissions systems, code ought to be able to decide it can drop privileges, but never be able to grant itself any expanded privileges.

Can I deny permissions to just a specific module?

No. The permission system is process-scoped. You can use the [policy](https://nodejs.org/api/policy.html) to restrict module access.

What if I spawn a process, it will inherit the root permissions?

A process that has --experimental-permission will not be able to spawn a child process by default. If the user explicitly allows it to spawn a child process, then it will be the user's responsibility to pass along the correct arguments.

Benchmarks

This feature adds a very low overhead (if any), either enabled or disabled). Please, note I'm measuring only the feature usage without restricted files/resources (a better benchmark suite will be created in subsequent PRs). The principal behavior is that it doesn't add overhead to the main fs usage. For example, using the benchmark/fs/readfile.js

comparisson between main and this PR (permission model disabled)

➜ node-benchmark-compare compare_fs_permission.csv
                                                                      confidence improvement accuracy (*)   (**)  (***)
fs/readfile.js concurrent=1 len=1024 encoding='' duration=5                          -0.58 %       ±0.78% ±1.05% ±1.38%
fs/readfile.js concurrent=1 len=1024 encoding='utf-8' duration=5                     -0.24 %       ±0.44% ±0.58% ±0.76%
fs/readfile.js concurrent=1 len=16777216 encoding='' duration=5                       0.15 %       ±0.96% ±1.28% ±1.67%
fs/readfile.js concurrent=1 len=16777216 encoding='utf-8' duration=5                  0.76 %       ±1.16% ±1.56% ±2.04%
fs/readfile.js concurrent=10 len=1024 encoding='' duration=5                          0.32 %       ±0.39% ±0.52% ±0.68%
fs/readfile.js concurrent=10 len=1024 encoding='utf-8' duration=5              *     -0.73 %       ±0.57% ±0.76% ±0.99%
fs/readfile.js concurrent=10 len=16777216 encoding='' duration=5                     -0.71 %       ±1.54% ±2.06% ±2.69%
fs/readfile.js concurrent=10 len=16777216 encoding='utf-8' duration=5                 0.09 %       ±0.69% ±0.91% ±1.19%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 8 comparisons, you can thus expect the following amount of false-positive results:
  0.40 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.08 false positives, when considering a   1% risk acceptance (**, ***),
  0.01 false positives, when considering a 0.1% risk acceptance (***)

comparisson enabling permission model (this branch)

node on  feat/permission-system [⇕$] took 40.4s
➜ ./node benchmark/fs/readfile.js
fs/readfile.js concurrent=1 len=1024 encoding="" duration=5: 24,192.15827315814
fs/readfile.js concurrent=10 len=1024 encoding="" duration=5: 95,917.90322639987
fs/readfile.js concurrent=1 len=16777216 encoding="" duration=5: 612.9672164291768
fs/readfile.js concurrent=10 len=16777216 encoding="" duration=5: 1,107.9227500862503
fs/readfile.js concurrent=1 len=1024 encoding="utf-8" duration=5: 24,314.915724686496
fs/readfile.js concurrent=10 len=1024 encoding="utf-8" duration=5: 92,569.37571752333
fs/readfile.js concurrent=1 len=16777216 encoding="utf-8" duration=5: 278.3922768971328
fs/readfile.js concurrent=10 len=16777216 encoding="utf-8" duration=5: 338.3870755499357

node on  feat/permission-system [⇕$!] took 40.4s
➜ ./node benchmark/fs/readfile-permission-enabled.js
(node:62815) ExperimentalWarning: Permission is an experimental feature
(Use `node --trace-warnings ...` to show where the warning was created)
^C

node on  feat/permission-system [⇕$!]
➜ ./node --no-warnings benchmark/fs/readfile-permission-enabled.js
fs/readfile-permission-enabled.js concurrent=1 len=1024 encoding="" duration=5: 24,185.367751785307
fs/readfile-permission-enabled.js concurrent=10 len=1024 encoding="" duration=5: 96,253.07607327335
fs/readfile-permission-enabled.js concurrent=1 len=16777216 encoding="" duration=5: 612.8715221066299
fs/readfile-permission-enabled.js concurrent=10 len=16777216 encoding="" duration=5: 1,106.1275272124165
fs/readfile-permission-enabled.js concurrent=1 len=1024 encoding="utf-8" duration=5: 24,375.648655236808
fs/readfile-permission-enabled.js concurrent=10 len=1024 encoding="utf-8" duration=5: 91,882.5214931248
fs/readfile-permission-enabled.js concurrent=1 len=16777216 encoding="utf-8" duration=5: 277.1205079822853
fs/readfile-permission-enabled.js concurrent=10 len=16777216 encoding="utf-8" duration=5: 337.30003270280775

Additional Considerations

• To make the THROW_IF_INSUFFICIENT_PERMISSIONS work, I had to make req_wrap weak, reverting the behaviour documented in fs: do not throw exception after creating FSReqCallback #35487. I was talking to @addaleax and it shouldn’t break anything. (See: src: make ReqWrap weak #44074)
• All of this work is being regularly discussed in the Security WG Meeting. Feel free to join if you want to raise questions/ideas.

cc: @nodejs/security-wg

[nodejs-github-bot]

Review requested:

• @nodejs/gyp
• @nodejs/startup

[mcollina]

Awesome work!

[targos]

If fs is completely denied, does it block the module loader? For example require('./package.json')

[RafaelGSS]

If fs is completely denied, does it block the module loader? For example require('./package.json')

Yes

rafaelgss@rafaelgss-desktop:~/repos/os/node$ ./node --policy-deny-fs=fs example.js
node:internal/modules/cjs/loader:157
  const result = internalModuleStat(filename);
                 ^

Error: Access to this API has been restricted
    at stat (node:internal/modules/cjs/loader:157:18)
    at Module._findPath (node:internal/modules/cjs/loader:531:16)
    at resolveMainPath (node:internal/modules/run_main:19:25)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:71:24)
    at node:internal/main/run_main_module:17:47 {
  code: 'ERR_ACCESS_DENIED',
  permission: 'FileSystemIn'
}

Node.js v19.0.0-pre

[targos]

It must be able to restrict access to specific folders and files

Don't we actually want the opposite (in common use cases)?
It seems way more usual to me to need to deny access to most of the file system, with a few exceptions (allow list instead of deny list)

[RafaelGSS]

It seems way more usual to me to need to deny access to most of the file system, with a few exceptions (allow list instead of deny list)

The problem is that it would block the Node.js internal functions. Node.js can't bypass those permissions since it runs the same code as the userland. If you deny access to the most of the file system, you would need to grant access to all internally needed FS operations manually.

I've talked to @jasnell and @addaleax previously and I don't think we're able to bypass Node.js needs without the complexity and compromising the security.

[addaleax]

I don't think we're able to bypass Node.js needs without the complexity and compromising the security.

To be fair, it’s probably quite a bit of work, but that doesn’t mean it isn’t worth doing. (This is something I’d not consider part of the initial implementation here, though.)

[tniessen]

It must be able to restrict access to specific folders and files

How is that going to work? For example, what prevents user code from creating a symlink from /tmp/xyz to $HOME/.ssh/id_rsa and then reading the protected file through said symlink?

[rluvaton]

Maybe we can allow changing the process policy only in the user land code and not in packages

[GeoffreyBooth]

I assume you know about https://nodejs.org/api/policy.html ? Shouldn’t the configuration for this be part of that?

[jasnell]

Given that @addaleax has already left a ton of comments, I'll wait a bit until those are resolved before going through and reviewing myself. Overall, however, on first read through things are looking pretty solid here. Happy to see this moving forward

[RafaelGSS]

How is that going to work? For example, what prevents user code from creating a symlink from /tmp/xyz to $HOME/.ssh/id_rsa and then reading the protected file through said symlink?

As mentioned in the description, this behavior wasn't addressed yet. I suggest discussing it in the next iteration (when all the code-review are fully resolved)

I assume you know about https://nodejs.org/api/policy.html ? Shouldn’t the configuration for this be part of that?

Yes. Honestly, this is quite different from the current policy behavior. We can discuss the nomenclature for sure, however, I suggest having this discussion later in this PR.

[addaleax]

How is that going to work? For example, what prevents user code from creating a symlink from /tmp/xyz to $HOME/.ssh/id_rsa and then reading the protected file through said symlink?

As mentioned in the description, this behavior wasn't addressed yet. I suggest discussing it in the next iteration (when all the code-review are fully resolved)

I think this is something that needs to be answered before this PR (or any PR with a path-based fs permissions model) can be merged, though.

[RafaelGSS]

I think this is something that needs to be answered before this PR (or any PR with a path-based fs permissions model) can be merged, though.

Sure, I will. What I meant by "next iteration" is the next round of review after addressing the list mentioned in the PR description 😄

[GeoffreyBooth]

I assume you know about nodejs.org/api/policy.html ? Shouldn’t the configuration for this be part of that?

Yes. Honestly, this is quite different from the current policy behavior. We can discuss the nomenclature for sure, however, I suggest having this discussion later in this PR.

I don’t think we can land a PR that adds a --policy-deny-* flag when we already have an existing --experimental-policy flag. Either this PR’s functionality needs to be configured through the policy.json file described in https://nodejs.org/api/policy.html or the existing policies functionality needs to be refactored to be defined via flags like --policy-deny-fs. We shouldn’t have two competing systems for security policies.

[RafaelGSS]

I don’t think we can land a PR that adds a --policy-deny-* flag when we already have an existing --experimental-policy flag. Either this PR’s functionality needs to be configured through the policy.json file described in https://nodejs.org/api/policy.html or the existing policies functionality needs to be refactored to be defined via flags like --policy-deny-fs. We shouldn’t have two competing systems for security policies.

That's indeed a good point. I'll mention it in the next Security WG. If you have any nomenclature suggestions, please raise them here 😄

[GeoffreyBooth]

That’s indeed a good point. I’ll mention it in the next Security WG. If you any nomenclature suggestion, please raise them here 😄

Looking in https://nodejs.org/api/policy.html, that feature is file/specifier specific, like “disable access to child_process“ or “ensure that app.js has this hash.” That’s too much data to try to convey via CLI flags, so I think we need to keep the separate file for defining all of that.

So I think maybe you could add a top-level section to that policy.json file for what you’re using flags for? Something like "permissions": { "deny": ["fs"] } or whatever you think is appropriate. And if what’s already in policies.json needs to move down a level under some new top-level field, that could happen too.

I agree there’s value in being able to define broad-brush permissions via a flag without needing a config file all the time (like if all the user wants to do is disable FS access or child processes or whatever, without getting into per-specifier definitions), so it’s probably worthwhile to keep the flag that this PR adds in addition to supporting the same via policies.json. We should probably come up with naming that makes sense when both exist, like --policy-file and --policy-permissions or something. I’ll leave the specifics to you and the Security WG; I just ask that you design the UX for this such that it makes sense alongside the policies feature that we already have. We should also probably create new flags as experimental at first, like --experimental-policy.

[RafaelGSS]

Adding dont-land-on-v19.x. After discussing it with TSC we've agreed to release it straight in Node.js v20.

Note to Node.js users: we've decided to drop the process.permission.deny for now. See: #47335