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

Windows: User can escape from root directory #167

Open
yetanothernickname opened this issue Jul 17, 2019 · 20 comments · Fixed by #224
Open

Windows: User can escape from root directory #167

yetanothernickname opened this issue Jul 17, 2019 · 20 comments · Fixed by #224
Labels
bug A defect or bug that affects the original indended use of the application security windows

Comments

@yetanothernickname
Copy link

yetanothernickname commented Jul 17, 2019

Windows, default File System, root directory set in login event callback. User can browse parent directory using /../../ in URL.
Example:
root: 'X:\\Project\\Storage\\User'

URL ftp://127.0.0.1/../../ becomes a command CWD \/../../

At line 30 in fs.js we have nodePath.resolve('X:\Project\Storage\User', '.\\\..\..\')

So _resolvePath() returns

{
  clientPath: '\\..\..\',
  fsPath: 'X:\Project'
}
@trs
Copy link
Contributor

trs commented Jul 19, 2019

This must be a bug on windows. I’ll have to investigate.

There is a specific test to ensure users cannot escape the root directory: https://github.com/trs/ftp-srv/blob/master/test/fs.spec.js#L65

The clientPath is passed through normalize which would resolve any . and ...
For example, in the test case, attempting to change directory to ../../../../../../../../../../.. would result in a clientPath (the path the client is actually shown) as /. Whereas the fsPath (the actual path on the server) would resolve to the root.

@trs trs added bug A defect or bug that affects the original indended use of the application help wanted input needed labels Jul 19, 2019
@yetanothernickname
Copy link
Author

Notice the backslash in the CWD command. I can't explain it, but..

const path = require('path');
console.log(path.normalize('\\/../../../../'));

Linux:
../../../

Windows:
\\..\..\

@trs
Copy link
Contributor

trs commented Jul 20, 2019

Trying this on windows I can't recreate it using Filezilla, Firefox, or Edge. What client are you using?

But in the next PR I have removed the added separator when joining paths, this should solve your issue.

@yetanothernickname
Copy link
Author

Firefox. Demo
I have a suspicion that this module is not designed to work with URLs...

@trs
Copy link
Contributor

trs commented Aug 8, 2019

Thanks for the demo video. The attached PR (#168) should address this issue. I'll work on getting it released soon.

@yetanothernickname
Copy link
Author

You can see how #168 works in this video :)

@trs
Copy link
Contributor

trs commented Aug 12, 2019

@yetanothernickname You used #168 for that video?

@yetanothernickname
Copy link
Author

@trs Yes, with new fsPath()

@matt-forster
Copy link
Contributor

I'd like to see if we can confirm this is still the case with the changes we made because it shouldn't be.

@n-timofeev
Copy link

@forstermatth I was able to reproduce it on 4cd88b1

@matt-forster
Copy link
Contributor

@n-timofeev Thanks, I will take a look this week.

@matt-forster matt-forster pinned this issue Dec 9, 2020
matt-forster added a commit that referenced this issue Dec 15, 2020
This should prevent paths from being resolved above the root.

Should affect all commands that utilize the FS functions.

Fixes #167
matt-forster added a commit that referenced this issue Dec 15, 2020
This should prevent paths from being resolved above the root.

Should affect all commands that utilize the FS functions.

Fixes #167
@nfacha

This comment has been minimized.

matt-forster pushed a commit that referenced this issue Dec 16, 2020
* fix(fs): check resolved path against root

This should prevent paths from being resolved above the root.

Should affect all commands that utilize the FS functions.

Fixes #167

* test: use __dirname for relative certs

* fix: improve path resolution

* chore: remove unused package

* fix: normalize resolve path if absolute

Otherwise join will normalize

Co-authored-by: Tyler Stewart <tyler@autovance.com>
@matt-forster matt-forster unpinned this issue Dec 16, 2020
@botovance
Copy link
Contributor

🎉 This issue has been resolved in version 4.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@n-timofeev
Copy link

Still not fixed or i'am doing something wrong...
demo1, demo2

@matt-forster
Copy link
Contributor

matt-forster commented Dec 17, 2020

Hey @n-timofeev, could you confirm the version in the lock file you have in those videos?

I see you step through the new resolve in the second one, enough proof for me, apologies.

@matt-forster matt-forster reopened this Dec 17, 2020
matt-forster added a commit that referenced this issue Dec 17, 2020
In an attempt to reproduce the test case presented in #167
@matt-forster matt-forster changed the title User can escape from root directory Winddows: User can escape from root directory Dec 17, 2020
@matt-forster matt-forster changed the title Winddows: User can escape from root directory Windows: User can escape from root directory Dec 17, 2020
@matt-forster matt-forster pinned this issue Dec 17, 2020
@matt-forster
Copy link
Contributor

matt-forster commented Dec 17, 2020

I've got PR #232 setup to test windows in our CI environment - I've also added a test that uses the path as is in your demo videos.

There are a lot of tests failing in that branch right now, mostly due to tests expecting Unix output. This should get us on the right track to fixing this without a doubt.

@matt-forster
Copy link
Contributor

GHSA-pmw4-jgxx-pcq9

@n-timofeev
Copy link

n-timofeev commented Dec 18, 2020

Source of backslash
Server sends it in response to the PWD command and then Firefox prepend it in CWD

And i think, we need to block paths with sequential dots and slashes here as a temporary solution

@heartz66

This comment has been minimized.

@antonilol
Copy link

@heartz66 making symlinks in you ftp folder is you own choice and cannot be done on the client side

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A defect or bug that affects the original indended use of the application security windows
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants