-
Notifications
You must be signed in to change notification settings - Fork 29.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Permission model restrictions imposed through process.permission.deny can be bypassed through case-insensitive paths #47105
Comments
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. |
Would a simple solution of storing and checking using |
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). |
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 For instance:
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. |
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 Of course, you can expect every application to block
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.
No. |
Sorry, I don't get it.
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. |
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 Anyway, you are right, that is a larger issue and does not need to be discussed in the context of this issue. |
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.) |
So, I was looking at it yesterday and I came to the conclusion that a possible approach would be:
Wdyt? |
Where would we do that? File systems can be mounted just about anywhere on Linux and Windows (probably also on macOS), so |
Therefore, a realistic approach, for now, can be:
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 ( |
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. |
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. |
So, after sleeping on this and talking with some colleagues I couldn't find a better solution than having a 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.) |
Unfortunately 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 |
That would, of course, fix this particular issue, or any potential issue related to 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 |
I mean, if removing the |
Also, 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: |
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.The text was updated successfully, but these errors were encountered: