GeezerVison

Review requested:

• @nodejs/gyp
• @nodejs/startup

Awesome work!

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

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

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)

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.

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.)

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?

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

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

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

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.

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.

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 😄

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.

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 😄

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.

So, all the errors are gone.

FWIW The CI was failing for two reasons:

• On node-test-commit-arm-debug: because args.As<Array> behave slightly differently on ARM64 using Debug mode.
• On node-test-commit-custom-freestyle: because it uses the tools/test.py --worker mode, which runs parallel tests inside a worker context, and when permission is enabled, workers aren't allowed unless --allow-worker is passed. To fix it, I had to skip the tests whenever permission is enabled and workers aren't allowed (See: tools/run-worker.js).

I'm planning to merge it soon (once test-asan finishes).

The CI is already green. I've pushed a performance improvement, once it's gets green again, I'll merge it

CI: https://ci.nodejs.org/job/node-test-pull-request/49924/

CI: https://ci.nodejs.org/job/node-test-pull-request/49925/

CI: https://ci.nodejs.org/job/node-test-pull-request/49930/

CI: https://ci.nodejs.org/job/node-test-pull-request/49932/

Landed in 00c2225

Big time lurker on this PR, now that it will no longer be noisy, I want to share that I'm so excited about this!! This is the thing that excites me the most about Deno vs Node.js, so I'm very thrilled to see it being worked on for Node.js directly. Thank you for the amazing work!

Would you mind rebasing against v18.x?

Given the following PRs/issues

• permission: fix spawnSync permission check #46975
• permission: fix some vulnerabilities in fs #47091
• Permission model restrictions imposed through process.permission.deny can be bypassed through case-insensitive paths #47105

we're going to have a baking-time of 1 release for this feature (after landing all the patches).

@RafaelGSS The blocked label isn't used by releasers to filter commits. I replaced it with backport-blocked

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