Skip to content
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: handle case-sensitive file systems #47269

Conversation

RafaelGSS
Copy link
Member

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.

@RafaelGSS RafaelGSS added semver-minor PRs that contain new features and should be released in the next minor version. security Issues and PRs related to security. labels Mar 27, 2023
@RafaelGSS RafaelGSS requested a review from tniessen March 27, 2023 14:13
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 27, 2023
@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 27, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 27, 2023
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

path.begin(),
path.end(),
path.begin(),
[](unsigned char c) -> unsigned char { return std::tolower(c); });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

I don't think we can do much more.

@RafaelGSS
Copy link
Member Author

RafaelGSS commented Mar 27, 2023

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

Closing it in favor of #47335

@RafaelGSS RafaelGSS closed this Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. security Issues and PRs related to security. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
5 participants