Quote Originally Posted by Dr Unne View Post
I'm probably not smart enough to find bugs or break your script, but I can see things that make me nervous.

I've heard (and I usually agree) that it's always better to define what you want to allow, rather than define what you want to disallow.
I agree completely.

You're disallowing periods and to some extent slashes, but you're allowing nearly anything else.
Why is this bad? Unix allows such characters (excluding the "/" and empty ASCII space to be part of the filename). Technically, although unlikely, such characters may be present in a valid filename.

In reality you'd want a filename to match a pattern like /^[A-Za-z0-9][A-Za-z0-9.]*$/. (Or modify it to allow slashes, but I wouldn't even allow them at all, personally.) Anything that doesn't match your regex should be rejected immediately. I passed your script a filename like #$%%*#!!%#^ and nothing bad happened, but still it accepted it as a valid filename all the way to the point where it was testing the filesystem for its existence. That's probably not good. I was looking for ways to URL-escape a "." character, and couldn't find any that survived your script, but if there are some, it would defeat your script also.
In Unix, "#$%%*#!!%#^" is a valid filename.
I don't think URL encoding would get past the check either. If (for some strange reason) it did, one could simply call rawurldecode() on the path before all the checking.

People can also start passing arbitrary binary data in via POST. I'm always scared someone's going to send me a 50MB URL and it's going to overflow and start doing horrible things.
People can send large amounts of post data, but the quantity is limited by PHP configuration settings.
post_max_size - Sets the maximum size of post data PHP will accept before bailing
max_input_time - Maximum time PHP will spend parsing post data
If 50MB would cause it to overflow, I would seriously lose all faith in those programming PHP. I don't think they would be foolish enough to provide open source code with any such overflow vulnerabilities.

You're also disclosing to the user the names of all your files. There's no reason the user needs to know this. If your files are in some path not in DOCUMENT_ROOT, you're exposing what your server's file structure looks like.
Well, the only way this script could be reasonably run would be within the web root directory anyway, so only webspace files could ever be included. As far as I can tell, there isn't a way to back up in the directory structure and get into any parent directories or files. Also, this is actually an abbreviated version of the script. (that I edited together quickly) The full version actually auto-appends the .php extension and will only include files of .php type. I simply provided a stripped down version here with the assumption that anyone using it would add and modify to fit the need.

Here the single thing you're doing with user input is using it as an index into an array. I don't know of any security issues with that. I don't think you even have to escape the user input or manipulate in any way.
I completely agree with this: having an allowed list is the safer solution.

This way you also aren't required to forbid using .'s in the pathnames or any other limitation.
Just as a note: my code only limits files and directories that start with a ".", any .'s later in the filename are allowed.