StaticRoute is vulnerable to directory traversal attacks

[magv]

Hi. There is a problem in the way StaticRoute class tries to prevent directory traversal attacks.

Currently it obtains the full file path by gluing the base directory with the requested path via os.path.join, and then checks if ".." is in the resulting path -- if there is, the path is rejected, otherwise, it is accepted. Here's the relevant code (taken from here):

    @asyncio.coroutine
    def handle(self, request):
        resp = StreamResponse()
        filename = request.match_info['filename']
        filepath = os.path.join(self._directory, filename)
        if '..' in filename:
            raise HTTPNotFound()
        if not os.path.exists(filepath) or not os.path.isfile(filepath):
            raise HTTPNotFound()
        ...

Unfortunately, os.path.join has a quirk: if it determines that it's second argument is an absolute path, it will ignore the first argument completely, and only return the second one, so if filename above is an absolute path, filepath will just be this path. Therefore, the code above does not protect against directory traversal attacks.

---

Here's a simple proof of concept:

import asyncio
from aiohttp import web

@asyncio.coroutine
def init(loop, address, service):
    app = web.Application(loop=loop)
    app.router.add_static("/data/", "data")
    server = yield from loop.create_server(app.make_handler(), address, service)
    return server

loop = asyncio.get_event_loop()
loop.run_until_complete(init(loop, "127.0.0.1", 8000))
loop.run_forever()

Run this program and direct your browser to one of these addresses (depending on which OS you're using):

http://127.0.0.1:8000/data//etc/passwd
http://127.0.0.1:8000/data/c:/config.sys

... and you'll obtain data which you should not be able to see.

---

OK, so there doesn't seem to be a single widely accepted way to sanitize relative paths; for example Django does this whole dance, while Bottle only does this. Both approaches seem to fix the current problem, so I ask you to adopt something of this sort.

[asvetlov]

Good point!
@magv would you propose a pull request?

[asvetlov]

Fixed by #383

[ludovic-gasc]

Hi @asvetlov, I don't know if it's in your plans, however, I suggest to publish a new release: even if it's a better practice for now to serve static files via Nginx, I'm pretty sure that somebody somewhere use that on production.
During my coaching session at work, you can't imagine the number of devs at the clients that used the manage.py Django server on production because it was simpler to deploy and they didn't have efficiency issues because it's often used for internal usage only.

[asvetlov]

NP. Published 0.16.3 release.

[ludovic-gasc]

Thank you ;-)

[lock]

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.