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

Possible arbitrary file access though Path Traversal vulnerability in some situations #25

Open
erikgeiser opened this issue Jun 28, 2020 · 3 comments · May be fixed by #28
Open

Possible arbitrary file access though Path Traversal vulnerability in some situations #25

erikgeiser opened this issue Jun 28, 2020 · 3 comments · May be fixed by #28
Assignees
Labels
bug Something isn't working

Comments

@erikgeiser
Copy link

A week ago I stumbled across this project on Hackernews and had a brief look at the code. I particular I noticed the spartan API which provides direct config file access:

@api.route('/config/<name>',  methods=['GET'])
def get_config(name: str):
    nginx_path = flask.current_app.config['NGINX_PATH']

    with io.open(os.path.join(nginx_path, name), 'r') as f:
        _file = f.read()

    return flask.render_template('config.html', name=name, file=_file), 200

The Problem

This code basically relies on the Flask router to filter out possibly malicious name values. The Flask router is obviously not designed for that, but it incidentally works fine for name values such as ../../../../etc/passwd as the route fails to match in this case. However, this is not always the case and more importantly it is dangerous to rely on this. I don't know if you officially support Windows as a platform but I want to use Windows paths as an example for this:

Imagine the NGINX_PATH is set to C:\\path\to\config\ and someone could do the following request:

curl 'http://localhost/api/config/D:\\some\unrelated\file'

Then the os.path.join call would work as follows:

os.path.join("C:\\\\path\\to\\config\\", "D:\\\\some\\unrelated\\file")
# 'D:\\\\some\\unrelated\\file'

This is a Path Traversal vulnerability which means your API would allow users to read and write arbitrary files. Even if you do not support Windows, the only protection in place on Linux is the router which is meant to be a protection. If someone clever would be able to get the name ../../../../etc/passwd passed through the router to this API endpoint you end in the same situation.

Possible Solution

As a solution I would suggest using os.path.basename on name first or if you want to support subdirectories of NGINX_PATH you could use os.path.join, then normalize the path with os.path.normpath or os.path.realpath and then check if the resulting path still starts with NGINX_PATH. For more information about path traversal vulnerabilities, see https://owasp.org/www-community/attacks/Path_Traversal.

@schenkd schenkd self-assigned this Jun 28, 2020
@schenkd schenkd pinned this issue Jun 28, 2020
@schenkd schenkd unpinned this issue Jun 28, 2020
@schenkd schenkd linked a pull request Jul 1, 2020 that will close this issue
@schenkd
Copy link
Owner

schenkd commented Jul 1, 2020

Hey @erikgeiser, thank you for taking the time to examine the code for possible points of attack. Although I believe the possibility of this attack to be unlikely, we will of course take care to fix this vulnerability. I would be happy if you keep an eye on the project and continue to support us.

Best David

@schenkd schenkd added the bug Something isn't working label Jul 1, 2020
@erikgeiser
Copy link
Author

You are right to assume it is unlikely, especially if the config directory is mounted into a docker container, which seems to be your primary usecase. If this was a serious vulnerability I would not have posted it as a public issue.

However, given the large amount of stars this project has, I assume that many user run it in vastly different circumstances and environments which may not be as forgiving as docker containers. Also I think it's always good to practice defense in depth and to make others aware of such issues.

I've also left some comments at the pull request ;-)

@schenkd
Copy link
Owner

schenkd commented Jul 1, 2020

@erikgeiser You're absolutely right. Thanks for your comments in PR. I haven't gotten around to testing the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants