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

Add an option to prevent following symlinks #147

Open
nlf opened this issue Jul 23, 2020 · 6 comments
Open

Add an option to prevent following symlinks #147

nlf opened this issue Jul 23, 2020 · 6 comments
Labels
feature New functionality or improvement

Comments

@nlf
Copy link
Member

nlf commented Jul 23, 2020

Currently, if a symlink is placed in the directory that inert shares files from and the user requests the symlink, the contents of the targeted file will be retrieved.

It is common practice to have an option to disable following symlinks and instead respond with a 404. We should add one.

@kanongil
Copy link
Contributor

kanongil commented Aug 12, 2020

Example from nginx: http://nginx.org/en/docs/http/ngx_http_core_module.html#disable_symlinks

It is quite expensive to check, as each path component will need a stat lookup. Additionally, a safe implementation of such an option will require openat() and fstatat() support from the host. This means that it will never work on Windows, and will require native code (or node implementation of nodejs/node#31110) to interface with the syscalls.

Given this, I vote that this feature is dropped.

@Nargonath
Copy link
Member

Given what you stated I agree, also the initial problem stems in user land as it is his responsability not to create a symlink in the served directory to a private file AFAICT.

@Marsup
Copy link
Contributor

Marsup commented Aug 13, 2020

If I understand the problem well enough, can't you use fs.realpath and compare the result with the provided path to make sure they're equal ? There's no mention in the docs about cross OS compatibility so I'm not sure about that part.

@kanongil
Copy link
Contributor

fs.realpath can be used to implement the feature, but not in a safe way. Since it operates on the path, there will always be a window of opportunity where it can have returned a "wrong" value.

  1. Get realpath.
  2. Open file at resolved path.

Between 1 and 2, the filesystem can have been manipulated to include a symlink in the resolved path, causing the open to point to eg. /etc/passwd.

@Marsup
Copy link
Contributor

Marsup commented Aug 13, 2020

The timing is highly unlikely though, I would consider it secure enough.

@nlf
Copy link
Member Author

nlf commented Aug 13, 2020

It being expensive is not a reason to not implement it. It's a reason to default it to off, but it doesn't mean we shouldn't have the option.

As for cross platform concerns, we support what node core supports. If node doesn't provide us an interface to support windows properly, then we note in the documentation that windows may not support the feature correctly.

I agree with @Marsup that the timing situation between comparing the result of fs.realpath to the provided path immediately before reading the file is probably acceptable. You'd have to tie up the event loop for quite a few cycles for that to introduce a gap big enough for it to be meaningful, and if someone can do that you've got bigger problems than them potentially following a symlink that you placed there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality or improvement
Projects
None yet
Development

No branches or pull requests

4 participants