Block path traversal characters

[ehtec]

Only allows files to be fetched when referenced by their real path (no ../ or ./ magic).

[ehtec]

I do not worry about this, people should not use some super special characters in filenames and I do not see this in the responsibility of Tornado.

Maybe people shouldn't use "super special" characters in filenames, but they should definitely be able to use non-English characters in their filenames. This is important and needs to be tested.

First, if you access a file through /static/../static_root_folder_name/file.txt. This will redirect to /static_root_folder_name/file.txt, which will not work as the /static prefix is missing now in the URL.

Oh, I think the fact that this ever worked was a bug (and I suppose is theoretically a leak of configuration parameters although since it works path-segment-by-path-segment instead of char-by-char there's not much risk).

Second, if you supply StaticFileHandler with a single file to serve (is this even possible?) on Windows, and the regex does not allow the lowercase filename (for example it allows File.txt but not file.txt). os.path.normcase will use lowercase characters on Windows as far as I know, so the file cannot be accessed anymore.

Yes, this is possible. It's most often seen (and is documented) for favicon.ico. I expect in general these kinds of cases use lowercase so it might be OK with normcase, although if you replace normcase with something that knows the actual case of the filename on disk (which I think would be better in most cases), it could potentially break these single-filename cases (if the file were stored on disk as e.g. Favicon.ICO)

There is no "real" way around this, as the whole vulnerability is about preventing the same file to be accessed in different ways.

Yes, if this were a vulnerability then the only solution would be to limit each file to a single URL even if that breaks some (hopefully few) applications. We've had some security fixes like this in the past. However, because I do not view this behavior as a vulnerability, I'm going to have to hold it to a much higher backwards-compatibility standard.

I had originally thought that forcing a single canonical URL per file would be a nice change to have, but now that we're digging into the details and implications I'm having a lot of doubts. (One area that we haven't even touched on yet is caching - a key feature of StaticFileHandler is that it supports version tags in URLs and long-term caching headers. This change strips off those tags on redirect and I'm not sure how the redirects would get cached). So while I thank you for your work on this PR and the original issue report, I think it may be time to chalk this up to "working as intended". Maybe we should just add a note to the URL routing docs that the "first match wins" behavior is not intended to serve as security access control vs later rules in the list and that there may be ways to construct URLs that miss a narrow rule and find their way to the wider one instead.

People can use non English characters in the filenames, it doesn't break anything. It's just that there might be bypasses so these could still be accessed even after this PR is merged.

The caching problem should be easy to solve, why not just pass the GET parameters to the redirect.

When single files are served, there should be some way to detect that in StaticFileHandler and not block anything.

Whether you see that as a vulnerability or not, there are applications out there severely affected by this, and saying this is secure is like building a 30cm wide bridge without a fence 100m above ground and saying it works as intended, just don't make an inaccurate step and you won't fall down. There is a reason why Flask, Apache and Nginx handle this differently...

[bdarnell]

There is a reason why Flask, Apache and Nginx handle this differently...

Just tried this with flask (https://replit.com/@bdarnell/NauticalNimbleLead#run.sh), and while it blocks all use of .., it allows . just like Tornado does (and it doesn't seem to do anything about case, just relying on the case-sensitivity of the underlying filesystem).

I still don't consider this a vulnerability, so given the backwards-compatibility implications and the subtleties noted above, I don't want to make any functionality changes based on this. I also don't think it's a particularly likely mistake to make, but I'd be willing to make some docs changes if you can suggest where people might be getting the wrong idea about how things work.