security bug: static file serving arbitrary file/source

stretchr / goweb

A lightweight RESTful web framework for Go

632 stars 61 forks source link

security bug: static file serving arbitrary file/source #18

Closed freehaha closed 11 years ago

freehaha commented 11 years ago

Hi, when a server comes up with a static serving directory. from client side it is able to access arbitrary file using relative path.

for example: my server is serving /static for static files, i can get /etc/passwd by requesting /static/../../../../../../etc/passwd

$ telnet localhost 9123
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
GET /static/../../../../../../etc/passwd HTTP/1.0

HTTP/1.0 200 OK
Accept-Ranges: bytes
Content-Length: 1234
Content-Type: text/plain; charset=utf-8
Last-Modified: Sat, 01 Aug 2012 05:13:28 GMT
X-Custom-Header: Goweb
Connection: close
Date: Wed, 05 Jun 2013 04:30:46 GMT

root:x:0:0:root:/root:/bin/bash
........

matryer commented 11 years ago

What a great find. Well spotted.

@tylerb and I will address this tomorrow, unless you can think of a fix and submit a pull request?

tylerstillwater commented 11 years ago

@freehaha what OS are you using?

freehaha commented 11 years ago

@matryer I currently don't have a fix. i'll try to come up with one after reading the source. @tylerb I'm using Linux, Debian, x86_64, go1.1

tylerstillwater commented 11 years ago

@freehaha thanks for the info. The fix should be pretty simple, but I can't seem to duplicate the bug on my Mac.

I'll see if I can get access to a debian box and try to reproduce it there.

freehaha commented 11 years ago

hi, I've attach the patch that fixes this problem. However, I'm not sure that it affects other parts of the package so please take a look before merging.

matryer commented 11 years ago

What if we just throw an error if the path contains some illegal things, like "../" etc?

Would that work?

On 5 Jun 2013, at 09:27, Tyler notifications@github.com wrote:

@freehaha thanks for the info. The fix should be pretty simple, but I can't seem to duplicate the bug on my Mac.

I'll see if I can get access to a debian box and try to reproduce it there.

— Reply to this email directly or view it on GitHub.

matryer commented 11 years ago

@freehaha - it's fully TDD, so if you do go test ./... (or better still use https://github.com/stretchrcom/gort) and if all tests pass, then you're golden. Also, before writing any code, you should try and 'prove' the bug by seeing a test fail first. In that case, we know we will be protected from regression.

freehaha commented 11 years ago

I have some tests added. Please verify them, thanks!

freehaha commented 11 years ago

yeah, path.Clean probably fits better here
updated