Permission model restrictions imposed through process.permission.deny can be bypassed through case-insensitive paths

[tniessen]

process.permission.deny() does not respect whether the relevant directories use case-insensitive path processing. Thus, unless an exponential number of paths is given to process.permission.deny(), one can easily bypass such a restriction by changing capitalization:

C:\>node --experimental-permission --allow-fs-read=* --allow-fs-write=*
(node:44336) ExperimentalWarning: Permission is an experimental feature
(Use `node --trace-warnings ...` to show where the warning was created)
Welcome to Node.js v20.0.0-nightly2023031585d614090b.
Type ".help" for more information.
> process.permission.deny('fs.read', ['C:\\Windows\\System32\\*'])
true
> fs.readdirSync('C:\\Windows\\System32')
Uncaught Error: Access to this API has been restricted
    at Object.readdirSync (node:fs:1454:26) {
  code: 'ERR_ACCESS_DENIED',
  permission: 'FileSystemRead',
  resource: '\\\\?\\C:\\Windows\\System32'
}
> fs.readdirSync('C:\\wIndows\\sYstem32')
[
  ...
]

Note that some directories process paths in a case-sensitive manner even on Windows, so simply matching case-insensitively on Windows is not correct in general either. Conversely, as @richardlau pointed out below, macOS and Linux also support case-insensitive mounts, so this is not just a Windows issue.

---

I'm opening this as a public issue because the feature hasn't been released yet due to previous vulnerabilities (see #46975 (comment)).

This vulnerability is unrelated to the far more significant fs-related vulnerabilities discussed in #47090.

[richardlau]

Windows isn't the only system with case insensitive paths. e.g. See https://support.apple.com/en-gb/guide/disk-utility/dsku19ed921c/mac for the various file systems available on macOS.

[tniessen]

@richardlau Good point. IIRC, ext4 also supports case-insensitive lookup, so then it's virtually all operating systems.

[RafaelGSS]

Would a simple solution of storing and checking using std::tolower solve it? Or it's more complex than that?

[tniessen]

While it sounds like it might solve this particular issue, it would not be correct; if the paths are case-sensitive (e.g., base64 encoded checksums or so used to identify different resources), then it would block legitimate operations.

(Also, maybe different locales across fs/os/application might be an issue with capitalization? I have no idea.)

Really, the fundamental problem of the permission model is that it protects paths, not actual resources. This is also where the symlink issue etc. come from, which led to a sufficient but not necessarily correct solution (i.e., it might block legitimate operations).

[RafaelGSS]

Really, the fundamental problem of the permission model is that it protects paths, not actual resources. This is also where the #44004 (comment) etc. come from, which led to a sufficient but not necessarily correct solution (i.e., it might block legitimate operations).

Actually, the permission model is blocking paths because they might not exist at some point. For instance, see the following snippet:

C:\>node --experimental-permission --allow-fs-read=* --allow-fs-write=*
(node:44336) ExperimentalWarning: Permission is an experimental feature
(Use `node --trace-warnings ...` to show where the warning was created)
Welcome to Node.js v20.0.0-nightly2023031585d614090b.
Type ".help" for more information.
> process.permission.deny('fs.read', ['/home/rafaelgss/file-does-not-exist-yet.md'])
true
> fs.readFileSync('/home/rafaelgss/file-does-not-exist-yet.md')
Uncaught Error: Access to this API has been restricted
...

Regardless of the existence of file-does-not-exist-yet.md, the permission model will block access to this path.
As you stated, my suggestion is inaccurate, but IMO this would be the only safer choice unless we remove the process.permission.deny API.

For instance:

foo/
  fiLe.md
  file.md

process.permission.deny('fs.read', './foo/file.md') will block access to either fiLe.md or file.md which is... unfortunate. Do you have a better idea? cc: @nodejs/security-wg

---

Deno also blocks by paths, but they are case-sensitive. However, since they don't have an explicit API (that works in the same way we use) to revoke permissions in runtime, they should be fine. I've sent a message to Luca from Deno to ask a couple of questions.

[tniessen]

Really, the fundamental problem of the permission model is that it protects paths, not actual resources. This is also where the #44004 (comment) etc. come from, which led to a sufficient but not necessarily correct solution (i.e., it might block legitimate operations).

Actually, the permission model is blocking paths because they might not exist at some point.

That's a very specific goal and relying on paths entirely does indeed match that particular goal well. But it also introduces a bunch of problems.

For example, in your simple example, it might seem somewhat secure at first, but you could have just read /proc/self/root/home/rafaelgss/file-does-not-exist-yet.md instead and trivially bypass the restriction. Different path, so from your implementation's point of view, a different resource. And I expect that a lot of users will get a false sense of security from these design issues.

Of course, you can expect every application to block /proc (on relevant Linux systems) and many other directories depending on the operating system etc. But that's putting a lot of responsibility onto users, and it is also highly dependent on the environment. Maybe a future Linux kernel will have /proc128 or so. Maybe there is some software that creates /cool-app/actually/this/is/the/root as a symlink to /. So unless --allow-fs-(read|write) is always set to a list of trusted directories that are guaranteed to absolutely never contain symlinks to outside those same directories, security issues will just keep arising.

As you stated, my suggestion is inaccurate, but IMO this would be the only safer choice

It's similar to the symlink situation I mentioned above and in the original PR: creating a symlink to a read-only path should be okay as long as the symlink is only used for reading, but the implementation does not allow this because it cannot distinguish between paths and resources. The same is true here.

Do you have a better idea?

No.

[RafaelGSS]

For example, in your simple example, it might seem somewhat secure at first, but you could have just read /proc/self/root/home/rafaelgss/file-does-not-exist-yet.md instead and trivially bypass the restriction.

Sorry, I don't get it. /proc/self/root/home/rafaelgss/file-does-not-exist-yet.md is a different resource path from /home/rafaelgss/file-does-not-exist-yet.md, so regardless of the approach used in the permission model, it should be allowed. Am I missing something?

Of course, you can expect every application to block /proc (on relevant Linux systems) and many other directories depending on the operating system etc. But that's putting a lot of responsibility onto users, and it is also highly dependent on the environment. Maybe a future Linux kernel will have /proc128 or so. Maybe there is some software that creates /cool-app/actually/this/is/the/root as a symlink to /. So unless --allow-fs-(read|write) is always set to a list of trusted directories that are guaranteed to absolutely never contain symlinks to outside those same directories, security issues will just keep arising.

One constraint we defined when designing the permission model was that it's NOT a sandbox. It does not intend to solve all the security problems and the case you just mentioned is completely out of the scope of this feature. If a third-party software can symlink the root path, it can do whatever it wants. It also falls in the Node.js Threat Model that we trust in the file system. So for now, I believe we should limit the discussions to the actual problem, which is, bypassing the permission check when using a case-insensitive path in case-insensitive environments.

I will investigate a better way to do it because I'm not convinced my suggested approach is any good.

[tniessen]

For example, in your simple example, it might seem somewhat secure at first, but you could have just read /proc/self/root/home/rafaelgss/file-does-not-exist-yet.md instead and trivially bypass the restriction.

Sorry, I don't get it. /proc/self/root/home/rafaelgss/file-does-not-exist-yet.md is a different resource path from /home/rafaelgss/file-does-not-exist-yet.md, so regardless of the approach used in the permission model, it should be allowed. Am I missing something?

It's a different path but it points to the same resource (inode) on Linux. Two names for the same thing. For example, if the file exists, any file permissions and properties applied to /home/rafaelgss/file-does-not-exist-yet.md would also automatically apply to /proc/self/root/home/rafaelgss/file-does-not-exist-yet.md (because it is the same thing), whereas in this permission model they are entirely separate things.

---

Anyway, you are right, that is a larger issue and does not need to be discussed in the context of this issue.

[bnoordhuis]

Would a simple solution of storing and checking using std::tolower solve it?

I don't think so. HFS+ stores Unicode filenames as NFD, not NFC, making it quite trivial to circumvent the naive version of the check you suggest.

APFS complicates things even further in that it accepts both NFC and NFD; the former for new files, the latter for existing files.

You could check file paths against both forms but that's a) expensive, and b) risks false positives (although from a security perspective that's better than false negatives.)

[RafaelGSS]

So, I was looking at it yesterday and I came to the conclusion that a possible approach would be:

• On environments that are case-insensitive by default (such as Windows) we compare in a case-insensitive way (std::tolower for example)
• On environments that are case-sensitive by default we can:
  • Adjust our documentation according to this behavior
  • Allow the user to change it using a flag --permission-case-sensitive
  • Perform a check on startup when --experimental-permission is passed (basically a mkdir dira && mkdir dirA) and give a warning if the file system is case-sensitive

Wdyt?