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

Fix directory traversal bug #4341

Merged
merged 5 commits into from Jun 11, 2019
Merged

Fix directory traversal bug #4341

merged 5 commits into from Jun 11, 2019

Conversation

wbrbr
Copy link
Contributor

@wbrbr wbrbr commented Jun 3, 2019

I found a neat little directory traversal bug in the webadmin code.
What the server does is that if the requested URL isn't "/", "/webadmin" or "/~webadmin" it will just try to fetch the corresponding file and send it back. The URL is sanitized before that: since we only want to serve files that are in the webadmin/files/ directory, we search for ../ in the URL and remove it.

The problem here is that this sanitization doesn't work: if an attacker sends ....//file.foo the sanitizer will transform it into ../file.foo and they will have successfully bypassed the filter. For example if I go to localhost:8080/....//....//webadmin.ini I get the webadmin.ini file containing all the user passwords in plaintext.

I see three ways of fixing this:

  • transform the relative path to an absolute path and check that it begins with the full path to webadmin/files
  • apply the sanitization filter in a loop while there is something to sanitize
  • just remove .. instead of ../. I think this fixes the bug but it might be less robust than the other two solutions (the first being probably the most robust but also the most complicated since we need to support Windows and Unix paths)

@peterbell10
Copy link
Member

Nice catch!

I'm not a fan of just replacing .., since it could potentially be part of an innocent file name. The other solutions all sound good. Another alternative would be to just fail as soon as any instances of ../ are found.

@wbrbr
Copy link
Contributor Author

wbrbr commented Jun 3, 2019

Here you go

@madmaxoft madmaxoft merged commit 85006d1 into cuberite:master Jun 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants