4 ms·

Hmm, startswith('/', '../') in is_secure_path doesn't check for paths like this: './../' Although it would prohibit some legitimate uses, you could just prohib

Hmm, startswith('/', '../') in is_secure_path doesn't check for paths like this: './../' Although it would prohibit some legitimate uses, you could just prohibit any usage of '../' anywhere in the path.

marcinw 16y ago
What about "..%2F..%2F" or "..\..\" or "..%5C..%5C"? In Java, you also need watch out for NULL (%00) in the path as well since java.io.File disregards anything after the null (most just test to see if filename ends with "ext" where ext is considered "safe", which is a huge error). Don't ever allow the client to specify a filename; use an indirect reference such as a uuid. If you must, always perform path normalization and/or only allow alphanumeric characters.
thwarted 16y ago
IIRC, the client should be resolving relative paths (using the provided or calculated base URI), and should be removing current directory (./) and parent directory (../) components before the request is even sent to the server. Server code should only ever see path-canonical requests. So it should be safe to just issue a 400 or 404 if you see ./ or ../ in the request. Now obviously doesn't apply if you figure out which page to serve based on a submitted GET variable, like the OP seems to suggest doing. I'd see about using PATH_INFO instead (it would also make the Urls easier to read, obscure the implementation a little and potentially be more portable) But it's reasonable, and safer, to just say "the page variable has a specific subdir/subdir/file.html structure and immediately 404 if any of those components don't match ([\w–]), rather than trying to use filesystem-path semantics to resolve to a file and trusting that'll be secure and does what you want.
the_mitsuhiko 16y ago
posixpath.normpath can never leave ./ in front: >>> import posixpath >>> posixpath.normpath('./../test') '../test'
tav 16y ago
You've overlooked the critical call in the previous line: def is_secure_path(path): path = posixpath.normpath(path) return not path.startswith(('/', '../')) The call to `normpath` normalises the path, e.g. >>> normpath('./../foo') '../foo'
- jimmybot 16y ago
  Ah, you're right, thanks.

marcinw 16y ago

What about "..%2F..%2F" or "..\..\" or "..%5C..%5C"? In Java, you also need watch out for NULL (%00) in the path as well since java.io.File disregards anything after the null (most just test to see if filename ends with "ext" where ext is considered "safe", which is a huge error). Don't ever allow the client to specify a filename; use an indirect reference such as a uuid. If you must, always perform path normalization and/or only allow alphanumeric characters.

thwarted 16y ago

IIRC, the client should be resolving relative paths (using the provided or calculated base URI), and should be removing current directory (./) and parent directory (../) components before the request is even sent to the server. Server code should only ever see path-canonical requests. So it should be safe to just issue a 400 or 404 if you see ./ or ../ in the request. Now obviously doesn't apply if you figure out which page to serve based on a submitted GET variable, like the OP seems to suggest doing. I'd see about using PATH_INFO instead (it would also make the Urls easier to read, obscure the implementation a little and potentially be more portable) But it's reasonable, and safer, to just say "the page variable has a specific subdir/subdir/file.html structure and immediately 404 if any of those components don't match ([\w–]), rather than trying to use filesystem-path semantics to resolve to a file and trusting that'll be secure and does what you want.

the_mitsuhiko 16y ago

posixpath.normpath can never leave ./ in front: >>> import posixpath >>> posixpath.normpath('./../test') '../test'

tav 16y ago

You've overlooked the critical call in the previous line: def is_secure_path(path): path = posixpath.normpath(path) return not path.startswith(('/', '../')) The call to `normpath` normalises the path, e.g. >>> normpath('./../foo') '../foo'

jimmybot 16y ago
Ah, you're right, thanks.