Repository navigation
Permission model restrictions imposed through process.permission.deny can be bypassed through case-insensitive paths #47105
Description
Activity
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.
@richardlau Good point. IIRC, ext4 also supports case-insensitive lookup, so then it's virtually all operating systems.
Reacted by Richard LauWould a simple solution of storing and checking using
std::tolowersolve it? Or it's more complex than that?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).
Reacted by JavierReally, 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 theprocess.permission.denyAPI.For instance:
foo/ fiLe.md file.mdprocess.permission.deny('fs.read', './foo/file.md')will block access to eitherfiLe.mdorfile.mdwhich 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.
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.mdinstead 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/proc128or so. Maybe there is some software that creates/cool-app/actually/this/is/the/rootas 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.
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.mdis 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.
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.mdis 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.mdwould 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.
Reacted by Rafael GonzagaWould 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.)
Reacted by Tobias NießenSo, 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::tolowerfor 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-permissionis passed (basically amkdir dira && mkdir dirA) and give a warning if the file system is case-sensitive
Wdyt?
- On environments that are case-insensitive by default (such as Windows) we compare in a case-insensitive way (
Perform a check on startup when
--experimental-permissionis passed (basically amkdir dira && mkdir dirA) and give a warning if the file system is case-sensitiveWhere would we do that? File systems can be mounted just about anywhere on Linux and Windows (probably also on macOS), so
/tmpmight be case-sensitive while/home/tniessen/.sshmight not be. On Windows NTFS, I think this is even a directory attribute, so directories might behave differently within the same file system.Reacted by Richard LauTherefore, a realistic approach, for now, can be:
- Allow the user to specific globally if the system is case-insensitive through cli flags
- Document/alert about this behavior
Then, we can include to the Permission Model - Rodmap an configuration to allow users to manage these file system settings by themselves.
Unless we consider dropping the runtime block API (
process.permission.deny) I don't think we have a better option than that.Allow the user to specific globally if the system is case-insensitive through cli flags
That's rather sub-optimal though. As Tobias points out, it doesn't have to be a binary yes/no.
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.
Reacted by Tobias NießenThat's rather sub-optimal though. As Tobias points out, it doesn't have to be a binary yes/no.
Yes, I agree. That's why a granular configuration is through a settings file. Something like:
{ "caseInsensitivePaths": [ "/home/foo": true, "/home/bar": { "dir": true, "files": false } ] }But yeah, I can concur with this might be a design flaw. I will investigate other options.
Reacted by JavierSo, after sleeping on this and talking with some colleagues I couldn't find a better solution than having a
--permission-case-sensitive(which is enabled by default on Linux) and clearly mentioning in the Permission Model documentation this restriction. Please, review #47269.Also, would be interested in your thoughts on this @mcollina @bmeck @addaleax @jasnell
Here's an alternative solution to consider: switch to openat(2) and friends, and change the permission model so only files in the blessed set of directories can be accessed.
(There are details to be worked out but they're much less patchy than the current approach.)
Reacted by Tobias NießenUnfortunately I think changing the model as @bnoordhuis suggests is likely the only way to address this. Deny everything except whatever is in an explicit allow list. Only paths that match byte for byte in the allow list would succeed. If someone uses a capitalization that is unexpected, it automatically defaults to denied even if it otherwise would work because the fs is case insensitive.
Using directory file descriptors certainly seems like a much cleaner approach than the current path-matching logic, but looking at uvwasi's implementation of preopens, for example, it also seems non-trivial using libuv and potentially a breaking change.
I'm fine with the remotion of
process.permission.denyif that fixes it and reduces a vector attack.That would, of course, fix this particular issue, or any potential issue related to
deny(). But I think it's worth nothing that it would not solve the underlying problem. The fact that the permission model does not respect how the OS interprets paths means that it might still break applications that rely on OS-compatible path processing (e.g., when accessing paths obtained throughreadlink()or from environment variables, which is a somewhat common issue on Windows).That being said, while perhaps not functionally correct, I don't see an immediate security issue with that approach right now.
I don't know how removing
deny()would affect the planned use cases of the permission model since I didn't get involved in the design phase and also haven't seen a detailed roadmap for adoption of this feature.I mean, if removing the
deny()would let this feature to land in the next v19 release, I'm all for it. The soonest we get users feedback the better. We need to see how will be the ecosystem adoption to this feature.Reacted by Marco IppolitoAlso, the problem with respecting how OS interprets path was the overhead it adds on top of the feature. One of the constraints we agreed on was: "no overhead when disabled/low overhead when enabled". But, I'm happy to discuss it too (in another issue).
Another thing we should consider when evaluating new approaches is the fact the permission model works for non-existing file paths. e.g:
--allow-fs-read=/home/rafaelgss/possible-new-path/possible-new-file.js. Therefore, I feel either theopenat(2)or storing and matching file descriptors wouldn't be that easy.
process.permission.deny()does not respect whether the relevant directories use case-insensitive path processing. Thus, unless an exponential number of paths is given toprocess.permission.deny(), one can easily bypass such a restriction by changing capitalization: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.