views:

72

answers:

3

I don't think I'm missing anything. Then again I'm kind of a newbie.

def GET(self, filename):
    name = urllib.unquote(filename)
    full = path.abspath(path.join(STATIC_PATH, filename))
    #Make sure request is not tricksy and tries to get out of
    #the directory, e.g. filename = "../.ssh/id_rsa". GET OUTTA HERE
    assert full[:len(STATIC_PATH)] == STATIC_PATH, "bad path"
    return open(full).read()

Edit: I realize this will return the wrong HTTP error code if the file doesn't exist (at least under web.py). I will fix this.

A: 

Make sure that either STATIC_PATH ends with a directory separator, or that the character following it in full is such.

Ignacio Vazquez-Abrams
+5  A: 

os.path.abspath, per se, is quite secure. Your assert check will be compiled away if run under python -O, which is one risk. If your STATIC_PATH does not end with the proper directory separator, you might accidentally allow a path which just happens to have it as a prefix -- e.g., if STATIC_PATH is /foo/bar, you'd erroneously accept a file living under /foo/barbie/ (so, unless STATIC_PATH does end with the separator character, you need a slightly stricter check for strong assurance).

Alex Martelli
Thanks. I didn't know about `python -O` but that does make sense. I suppose because of that I should not use assert in the future just in case I forget its use and end up running `python -O`I also didn't know `path.join` took that sort of liberty but that's definitely good to know.
mcmt
@mcmt, not sure what you mean by `os.path.join` "taking a liberty" -- it _supposed_ to ignore trailing `os.sep` characters in all but the last item, so the path's constructed and `abspath`ed correctly, so you need to check the first `len(staticpath.rstrip(os.sep))+1` of the result vs `staticpath.rstrip(os.sep)+os.sep` unless you otherwise guarantee that the `staticpath` _does_ end with `os.sep`.
Alex Martelli
Oh, I misunderstood what you were saying. Thanks for clarifying.
mcmt
+1  A: 

Generally speaking, dealing with these kinds of issues is a huge pain. There's always some tricky way of combining path names to create something a programmer didn't expect or plan for.

A better solution is usually to make sure that your webserver is running under a user that only has access to the files it needs to have access to. I think a simple solution to the ssh issue is not to allow this user SSH access. If you really need to log in under this user, I'd consider logging in under a different account and su-ing to it.

Jason Baker
I agree; I just used ssh as an example. At any rate, when I deploy I'm going to let nginx serve static files. I was curious as to the issue in general.
mcmt