src,process: initial permission model implementation

[RafaelGSS]

Reference issue: https://github.com/nodejs/security-wg/issues/791

The tagged issue contains the initial proposal for this MVP.

This Pull Request includes the foundation of the Permission Model. This PR is not ready to land. I need to address the following list first:

• [x] Improve the THROW_IF_INSUFFICIENT_PERMISSION call
  • Currently, it does a raw string check and it certainly can be improved by replacing it with a radix-tree/bloom filter algorithm.
• [ ] Handle spawn process
• [ ] Handle symlinks (Let's discuss it in the root issue)
  • e.g: app doesn’t have access to the bar/ folder
  • app symlinks foo/asdf  to bar/asdf
  • and then edits foo/asdf which is actually bar/asdf without ever touching bar/ from the Node.js app perspective
• [ ] Create documentation

Therefore, I suggest avoiding discussing it in the first PR iteration.

---

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

Currently, the CLI Nomenclature stands for --policy-deny-$RESOURCE . This PR includes the --policy-deny-fs which aims to manage permissions to the entire FileSystem APIs.

It must be able to restrict access to specific folders and files either in the Node.js bootstrap through the --policy-deny-fs CLI option or in runtime using the process.policy.deny('fs') API call.

The CLI Arguments

The valid arguments for the CLI option are:

• out - To manage FileSystemOut (Writing) operations.
• in - To manage FileSystemIn (Reading) operations.
• fs - To block all the FileSystem operations.

out and in can accept file/directory path as argument using : as a delimiter.

Example:

• --policy-deny-fs=in:/tmp/ - It will restrict FileSystemIn access to the /tmp/ folder
• --policy-deny-fs=in:/tmp/:./.gitignore - It will restrict FileSystemIn access to the /tmp/ folder and the ./.gitignore path — Relative paths are supported.

You can also mix both arguments:

• --policy-deny-fs=out,in:/tmp/ - It will restrict FileSystemIn access to the /tmp/ folder and restrict all the FileSystemOut operations.

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 policy was added to the process module. The property contains two functions:

• deny(scope [,parameters])

API Call to deny permissions at runtime. e.g

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

process.policy.deny('fs.out') // deny permissions to ALL FileSystemOut operations
process.policy.deny('fs.out', '/home/rafaelgss/protected-folder') // deny FileSystemOut permissions to the protected-folder
process.policy.deny('fs.in') // deny permissions to ALL FileSystemIn operations
process.policy.deny('fs.in', '/home/rafaelgss/protected-folder') // deny FileSystemIn permissions to the protected-folder

• check(scope [,parameters])

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

process.policy.check('fs.out') // true
process.policy.check('fs.out', '/home/rafaelgss/protected-folder') // true

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

process.policy.check('fs.out') // true
process.policy.check('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 policy_deny_net.h and implementing PolicyDeny 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.

Additional Considerations

• To make the THROW_IF_INSUFFICIENT_PERMISSIONS work, I had to make req_wrap weak, reverting the behavior documented in https://github.com/nodejs/node/pull/35487. I was talking to @addaleax and it shouldn’t break anything.
• All of this work is being regularly discussed in the Security WG Meeting. Feel free to join if you want to raise questions/ideas.
• The API Nomenclature is not written in stone. I believe that policy-deny-* is reasonable, but, If you have any objections, please, raise them here and suggest a better approach if you can.

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 :smile:

[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 :smile:

[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]

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. -- @GeoffreyBooth

We've discussed it in the last Security WG (https://github.com/nodejs/security-wg/issues/818) and TL;DR is:

policy definitely doesn't seem an appropriate namespace to land this permission system. Both features are quite different (even though, produce almost the same behaviour in some circumstances). The current policy mechanism is experimental, meaning that if the permission system uses its scope, it will create an unnecessary dependency. In other words, the permission system mechanism might never go stable if the current policy mechanism doesn't go stable. For this reason, we're evaluating a new namespace and the available options are:

• Access Control (--access-deny-fs=, --access-deny-net=)
• Permission System (--permission-deny-fs=, --permission-deny-net=)
• Permissions (--deny-fs=, --deny-net=)

I'd suggest voting :+1: for the Access Control, :heart: for the Permission System, and :rocket: for the Permissions. In case you don't find any of them attractive (which is totally reasonable), please, suggest a better namespace.

[GeoffreyBooth]

suggest a better namespace.

Perhaps just copy Deno's naming? https://deno.land/manual/getting_started/permissions

They call their feature Permissions but their flag is allow, as in --allow-read or --allow-net. This was one of the primary features of Deno back when it was announced so I think a lot of Node developers might be aware of it even if they don't use Deno.

Our version seems to be permissive by default so our flag would be --deny-*, or we could support both; maybe passing any allow flag implies denying everything that's unspecified.

We still need to reconcile this with the existing policy feature. It's not just naming; the two features have overlap, and we need to decide if we're keeping those areas of overlap (and if so, what to do when both are specified) or removing them, like by limiting the policy feature to just hashes and whatever else it does that this feature doesn't do.

I also think this should be experimental to start. We don't need to include experimental in the flag name but it should emit the experimental warning and be marked as such in the docs.

[RafaelGSS]

Our version seems to be permissive by default so our flag would be --deny-*, or we could support both; maybe passing any allow flag implies denying everything that's unspecified.

The structure defined in https://github.com/nodejs/security-wg/issues/791 is to be permissive by default (avoiding a global breakage). The nomenclature suggested (--deny-*) makes sense to me. I've included it as a possibility on the list.

Also, the recommendation to use --experimental-permissions is totally doable to me. I'll include that soon as we define the nomenclature.

We still need to reconcile this with the existing policy feature. It's not just naming; the two features have overlap, and we need to decide if we're keeping those areas of overlap (and if so, what to do when both are specified) or removing them, like by limiting the policy feature to just hashes and whatever else it does that this feature doesn't do.

Honestly, I don't see the existing feature overlapping with this one. As said above, they might have a similar behaviour, but the purpose is different. To avoid polluting this PR with this discussion, would you mind participating in the next Security WG Meeting and elaborating on your point of view? It's planned to be Thu, August 18.

[GeoffreyBooth]

The structure defined in nodejs/security-wg#791 is to be permissive by default (avoiding a global breakage). The nomenclature suggested (--deny-*) makes sense to me. I've included it as a possibility on the list.

Thanks. I would also take a look at https://deno.land/manual/getting_started/permissions, there are a lot of good ideas there. For example, they separate the permissions for filesystem read and filesystem write; that's probably something we should do too. For network access, if it's possible to create separate permissions for full network access versus only responding to incoming requests, that would be a good distinction too. Such a permission would let you spin up a webserver that wouldn't be capable of exfiltrating data, like if one of your dependencies stole your environment variables on startup and posted them somewhere. (It would still be vulnerable to data exfiltration as part of responding to a request, but at least the other attack vector would be denied.)

Also, the recommendation to use --experimental-permissions is totally doable to me. I’ll include that soon as we define the nomenclature.

Personally I don’t think you need to add --experimental to the flag name; just include the emitExperimentalWarning call somewhere in the code and note it in the docs. We did this recently with #43942, where we added a new flag --import that was experimental but not named --experimental-import.

Honestly, I don’t see the existing feature overlapping with this one. As said above, they might have a similar behaviour, but the purpose is different.

I think while they’re both experimental it’s not urgent to resolve the differences between the two; but I think a coherent user experience for users using both features together needs to be something that we prioritize as you’re developing this. It feels to me like something that should be worked out early on, in a design phase, not after you’ve already landed the first PR; but I won’t block on those grounds.

In general I think it’s bad UX to have two features that achieve the same result, and have similar-seeming intentions, but with completely different methods (a flag versus a config file). I understand that the two features don’t fully overlap, that each one does some things that the other doesn’t, but in a way that makes this even harder to resolve because you can’t just replace one with the other. I want to resolve the conflict of how to handle what overlap they do have as early as possible so that development can continue on both without significant breaking changes or refactoring needed by either team. I’m not sure I can attend the next security meeting; perhaps just open a discussion issue and we can hash it out async?

[Qard]

Is there a reason we can't follow the Deno model but have permissive by default unless there are --allow-* flags set? It should mean no ecosystem breakage. Also gives us a path to gradual adoption and then have the opportunity to flip to locked down by default in the future, if we so choose.

[RafaelGSS]

Is there a reason we can't follow the Deno model but have permissive by default unless there are --allow-* flags set? It should mean no ecosystem breakage. Also gives us a path to gradual adoption and then have the opportunity to flip to locked down by default in the future, if we so choose.

That's a reasonable approach too. I will probably need to modify a few things, but that's a good time for that. How can we get a consensus on that? I'll investigate this week the efforts to make it happen.

The first downside I see is that it will make the adoption significantly harder, but security-wise seems suitable.

[RafaelGSS]

@Qard Ok, I recall why I didn't this way first. The problem isn't just the breakage of the ecosystem, but that would limit the Node.js itself. For instance, let's say you execute the application with: --allow-write, which means that FileSystemIn (read) would be completely denied. However, since this PR doesn't distinguish Node.js code from userland code, it will block the Node.js execution since it can't just require a module (https://github.com/nodejs/node/pull/44004#issuecomment-1196678019). As mentioned before, making it work will require a big amount of work that's probably not worth it to do in this first iteration. I mean, I'm not saying that we won't have it, but likely in this MVP, it would significantly increase the complexity.

[Qard]

Well, you could just leave it on the user to give all the necessary permissions, including what Node.js itself needs. I would argue that's probably the safer approach anyway. It's easier to loosen restrictions without breaking compatibility than it is to tighten them.

[RafaelGSS]

Well, you could just leave it on the user to give all the necessary permissions, including what Node.js itself needs. I would argue that's probably the safer approach anyway. It's easier to loosen restrictions without breaking compatibility than it is to tighten them.

How the users would know which file permissions the node would need to run? For instance, to do a simple require, Node.js reads some folders which shouldn't be allowed by default. I've added a log every time the is_granted method is called for a quite simple application:

// t.js
require('fs')
require('./t2')
// t2.js
module.exports = {
  a: 1
}

rafaelgss@rafaelgss-desktop:~/repos/os/node$ ./node t.js
Looking to: /home/rafaelgss/repos/os/node/t.js
Looking to: /home
Looking to: /home/rafaelgss
Looking to: /home/rafaelgss/repos
Looking to: /home/rafaelgss/repos/os
Looking to: /home/rafaelgss/repos/os/node
Looking to: /home/rafaelgss/repos/os/node/t.js
Looking to: /home/rafaelgss/repos/os/node/package.json
Looking to: /home/rafaelgss/repos/os/package.json
Looking to: /home/rafaelgss/repos/package.json
Looking to: /home/rafaelgss/package.json
Looking to: /home/package.json
Looking to: /package.json
Looking to: /home/rafaelgss/repos/os/node/t.js
Looking to: /home/rafaelgss/repos/os/node
Looking to: /home/rafaelgss/repos/os/node/t2
Looking to: /home/rafaelgss/repos/os/node/t2.js
Looking to: /home/rafaelgss/repos/os/node/t2.js
Looking to: /home/rafaelgss/repos/os/node/t2.js

As you can see, due to our Module Resolution Algorithm all those directories are visited. It would require the user to grant access to all the root directories for instance.

[Qard]

Well, can we not alter our module resolution to halt when it reaches a level it doesn't have access to? It should be starting at the module path and walking upwards until it finds a match. We could just make it spit out the "module not found" error when it hits an access barrier the same way we already spit it out when reaching root without a match.

[RafaelGSS]

That's a good idea. I'm not sure about its impact, but I'll give it a shot. Thanks!

[RafaelGSS]

Hey folks!

Finally, I got some time to work on this. As discussed, I've implemented the non-permissive approach and fixed some bugs. The remaining piece is to deal with symlinks. Therefore, it's ready to review!

Since it's a such big PR, I tried to detail everything in the PR description + docs. If something is unclear to you, lmk.

cc: @nodejs/security-wg @nodejs/tsc

[ovflowd]

The documentation should mention that the permission system only protect the Node.js JavaScript APIs. The moment a user load a binary addon.. they can bypass all of this.

Yea, by spawning a child process that is not Node, or any other language, they can directly bypass this granted. This allows us to document how users should be careful when spawning child processes. (But I believe this is regardless of this implementation). We should (not sure if we already have) security guidelines for spawning child processes (aka exec).