permission: handle case-sensitive file systems

[RafaelGSS]

Fixes #47105

This PR introduces a new cli flag that allows the user to explicitly set how FSPermission will handle the paths (case-sensitive or not).

As previously discussed, this also shows a feature limitation since you can mount a file system using a custom configuration (case-sensitive on /home/user1 and case-insensitive on /home/user2). However, this will be categorized as a known limitation and is likely to be discussed at nodejs/security-wg#898 when enabling per-file configuration.

[nodejs-github-bot]

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

[tniessen]

Does this affect both --allow-fs* and process.permission.deny()?

If so, assuming a system uses a mix of case-sensitive and case-insensitive file systems and/or directories, making process.permission.deny() work somewhat reasonably requires case-insensitivity (in order to over-approximate the set of paths that access is being denied to), but then suddenly the paths specified in --allow-fs* are also matched case-insensitively (thus also over-approximated)?

Without forcing users to go through the trouble of managing all of this manually, would it not be safer to over-approximate the set of denied paths and to under-approximate the set of allowed paths?

[tniessen]

cc @bnoordhuis, I am not sure if this is related to the concern in #47105 (comment).

[mcollina]

lgtm

I don't think we can do much more.

[RafaelGSS]

If so, assuming a system uses a mix of case-sensitive and case-insensitive file systems and/or directories, making process.permission.deny() work somewhat reasonably requires case-insensitivity (in order to over-approximate the set of paths that access is being denied to), but then suddenly the paths specified in --allow-fs* are also matched case-insensitively (thus also over-approximated)?

@tniessen I'm not sure if I understand, but, the flag you're referring to is designed to work with File System permissions (also known as FSPermission) throughout the entire system -- either .deny or --allow-fs*. Once set, this flag remains constant for security reasons and to reduce complexity in the code. While this flag may be a known limitation for now, it may be addressed in the future as part of our roadmap. However, at the moment, I believe it's better to keep things simple, of course, addressing all the security flaws.

[tniessen]

I'm not sure if I understand, but, the flag you're referring to is designed to work with File System permissions (also known as FSPermission) throughout the entire system

What I mean is that, based on my understanding of this flag, if I allow access to /home/tniessen and then want to deny access to the directory foo/bar (ignoring capitalization) in /home/tniessen, then I have to grant access to /home/tNiEsSen. That's what I mean when I say that this over-approximates the set of allowed paths.

However, at the moment, I believe it's better to keep things simple

A simple option might be to remove process.permission.deny(). As Ben put it in #47105 (comment),

It's better take a step back and think about the soundness of the design. If the filter is so easily defeated, and fixing it requires punting so much policy to the user, then the design is flawed.

[RafaelGSS]

What I mean is that, based on my understanding of this flag, if I allow access to /home/tniessen and then want to deny access to the directory foo/bar (ignoring capitalization) in /home/tniessen, then I have to grant access to /home/tNiEsSen. That's what I mean when I say that this over-approximates the set of allowed paths.

See the following code:

$ ls ~/foo/
bar
$ cat ~/foo/bar
hello world
$ ./node --experimental-permission --allow-fs-read=/Users/rafaelgss/ --no-permission-case-sensitive
(node:51672) ExperimentalWarning: Permission is an experimental feature
(Use `node --trace-warnings ...` to show where the warning was created)
Welcome to Node.js v20.0.0-pre.
Type ".help" for more information.
>
Access to FileSystemOut is restricted.
REPL session history will not be persisted.
> const fs = require('fs')
undefined
> fs.readFileSync('/Users/rafaelgss/foo/bar').toString()
'hello world\n'
> process.permission.deny('fs.read', ['/Users/rafaelgss/foo/bar'])
true
> fs.readFileSync('/Users/rafaelgss/foo/bar').toString()
Uncaught Error: Access to this API has been restricted
    at Object.openSync (node:fs:587:26)
    at Object.readFileSync (node:fs:458:35) {
  code: 'ERR_ACCESS_DENIED',
  permission: 'FileSystemRead',
  resource: '/Users/rafaelgss/foo/bar'
}
> fs.readFileSync('/Users/rafaelgss/index.cjs').toString()
"console.log('oi')\n"

We keep 2 trees in the FSPermission, one for granted resources and one for denied (in runtime) resources, therefore if you want to deny a resource inside a granted wildcard, you can. Also, note that --no-permission-case-sensitive isn't needed here, it will be false by default since I'm using macOS.

Is this behavior you are expecting? If yes, we should be fine.

[tniessen]

That's not what I meant. Let me rephrase my question again:

Also, note that --no-permission-case-sensitive isn't needed here, it will be false by default since I'm using macOS.

If that means that all path comparisons are case-insensitive by default, doesn't that mean that you also (implicitly) granted access to /uSeRs/rAfAeLgSs/ in your example, which might be an entirely different directory?

[RafaelGSS]

If that means that all path comparisons are case-insensitive by default, doesn't that mean that you also (implicitly) granted access to /uSeRs/rAfAeLgSs/ in your example, which might be an entirely different directory?

That's correct. But, at this point, I was considering it as a known limitation.

In any case, I think we'll reach a consensus in #47105 to remove the process.permission.deny and reduce the attack vector, so this pr will probably be closed

[RafaelGSS]

Closing it in favor of #47335