0
\$\begingroup\$

I made a simple web-based file browser with Flask. It also shows some metadata of a file. I have a few questions regarding security though. Is it possible for an attacker to escape out of the preset root directory? Are there any ways to make my code more readable and/or more efficient? Would love to hear some feedback to improve my program! :)

app.py:

from flask import Flask, render_template
import os
import stat

app = Flask(__name__)

# Make the WSGI interface available at the top level so wfastcgi can get it.
wsgi_app = app.wsgi_app

FILE_SYSTEM_ROOT = "D:\Test123"

@app.route('/')
def index():
    return render_template('index.html')

@app.route('/browser')
def browse():
    itemList = os.listdir(FILE_SYSTEM_ROOT)
    return render_template('browse.html', itemList=itemList)

@app.route('/browser/<path:urlFilePath>')
def browser(urlFilePath):
    nestedFilePath = os.path.join(FILE_SYSTEM_ROOT, urlFilePath)
    if os.path.isdir(nestedFilePath):
        itemList = os.listdir(nestedFilePath)
        fileProperties = {"filepath": nestedFilePath}
        if not urlFilePath.startswith("/"):
            urlFilePath = "/" + urlFilePath
        return render_template('browse.html', urlFilePath=urlFilePath, itemList=itemList)
    if os.path.isfile(nestedFilePath):
        fileProperties = {"filepath": nestedFilePath}
        sbuf = os.fstat(os.open(nestedFilePath, os.O_RDONLY)) #Opening the file and getting metadata
        fileProperties['type'] = stat.S_IFMT(sbuf.st_mode) 
        fileProperties['mode'] = stat.S_IMODE(sbuf.st_mode) 
        fileProperties['mtime'] = sbuf.st_mtime 
        fileProperties['size'] = sbuf.st_size 
        if not urlFilePath.startswith("/"):
            urlFilePath = "/" + urlFilePath
        return render_template('file.html', currentFile=nestedFilePath, fileProperties=fileProperties)
    return 'something bad happened'

if __name__ == '__main__':
    app.run(host='0.0.0.0', port=80)

browse.html:

<!DOCTYPE html>

<html lang="en" xmlns="http://www.w3.org/1999/xhtml">
<head>
    <meta charset="utf-8" />
    <title></title>
</head>
<body>
    <ul>
        {% for item in itemList %}
        <li><a href="/browser{{ urlFilePath }}/{{ item }}">{{ item }}</a></li>
        {% endfor %}
    </ul>
</body>
</html>

file.html:

<!DOCTYPE html>

<html lang="en" xmlns="http://www.w3.org/1999/xhtml">
<head>
    <meta charset="utf-8" />
    <title></title>
</head>
<body>
    <p>Current file: {{ currentFile }}</p>
    <p>
        <table>
            {% for key, value in fileProperties.items() %}
            <tr>
                <td>{{ key }}</td>
                <td>{{ value }}</td>
            </tr>
            {% endfor %}
        </table>
    </p>
</body>
</html>

The output when browsing looks something like this: browse.html example

The output with the metadata of a file looks like this: file.html example

Improve this question
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Food for thoughts: what would happen if I browsed to htrp://localhost/browser/../an/other/path.txt? \$\endgroup\$
    – 301_Moved_Permanently
    Feb 27, 2019 at 22:38
  • 2
    \$\begingroup\$ I have rolled back the last edit. Please see What to do when someone answers. \$\endgroup\$
    – Zeta
    Mar 2, 2019 at 10:55

2 Answers 2

Reset to default
1
\$\begingroup\$

@Matthias has already mentioned this in the comments, and he is correct.

The code suffers from a Directory Traversal vulnerability

You can filter this in 2 ways:

  1. Use os.path.realpath and check if the resolved path is different

    def check_lfi(file_path):
    return os.path.realpath(file_path) != file_path

  2. Use regex to filter out bad (../) Paths

    import re
DIR_TRAVERSAL = re.compile(r"\.\.\/")
def check_lfi(file_path):
    return bool(DIR_TRAVERSAL.search(file_path))

My advice would be to use the first example, since blacklisting can be error prone (Unicode for instance)

Improve this answer
\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the hint, I updated my original post with the fix. I tested it out with curl and directory traversals aren't possible anymore. \$\endgroup\$
    – Woask
    Mar 2, 2019 at 10:47
0
\$\begingroup\$

I changed my code with the feedback from Ludiposed. This is what it looks like:

@app.route('/browser/<path:urlFilePath>')
def browser(urlFilePath):
    nestedFilePath = os.path.join(FILE_SYSTEM_ROOT, urlFilePath)
    nestedFilePath = nestedFilePath.replace("/", "\\")
    if os.path.realpath(nestedFilePath) != nestedFilePath:
        return "no directory traversal please."
    if os.path.isdir(nestedFilePath):
        itemList = os.listdir(nestedFilePath)
        fileProperties = {"filepath": nestedFilePath}
        if not urlFilePath.startswith("/"):
            urlFilePath = "/" + urlFilePath
        return render_template('browse.html', urlFilePath=urlFilePath, itemList=itemList)
    if os.path.isfile(nestedFilePath):
        fileProperties = {"filepath": nestedFilePath}
        sbuf = os.fstat(os.open(nestedFilePath, os.O_RDONLY)) #Opening the file and getting metadata
        fileProperties['type'] = stat.S_IFMT(sbuf.st_mode) 
        fileProperties['mode'] = stat.S_IMODE(sbuf.st_mode) 
        fileProperties['mtime'] = sbuf.st_mtime 
        fileProperties['size'] = sbuf.st_size 
        if not urlFilePath.startswith("/"):
            urlFilePath = "/" + urlFilePath
        return render_template('file.html', currentFile=nestedFilePath, fileProperties=fileProperties)
    return 'something bad happened'

I changed some things:

  • The forward slashes get replaced by backward slahes, since i'm developing in a Windows enviroment.
  • The canonical filepath should be the same as the requested filepath, this filters out ../ and other directory traversal methods from the url.
  • The new code has been tested with curl, and is safe.
Improve this answer
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.