views:

191

answers:

3
/* define page path */
define("PAGE_DIR", "pages/");

if (file_exists(PAGE_DIR."$_GET[page].php")) include(PAGE_DIR."$_GET[page].php");

How safe is this? Could you for example include a page on another webserver if the page is in a folder called pages?

Thanks

+6  A: 

This isn't safe at all - Think about what happens if $_GET[page] contains ../../../somewhere/else/

You should explicitly have a list of allowed pages.

Edit: I don't think it could include a file from a different server, but it's still not a good thing to be doing.

Greg
Absolutely right. ++;
Jonathan Sampson
+1  A: 

Doing anything with $_GET or $_POST prior to validating/sanitizing the data is dangerous. Assume that all users are out to get you, and sanitize the data prior to using it.

Jonathan Sampson
+2  A: 

It's never good practice to pass unsanitized user input directly to a command, especially something like include(). You don't necessarily know how the underlying webserver/OS is going to handle, for example, relative paths, extended characters, etc. Any of these, used maliciously or otherwise, could result in the user seeing something they're not supposed to see.

One possible exploit: user passes in the relative path to a malicious script in a known location on the server. http://webserver/yourscript.php?page=%2e%2e%2f%2e%2e%2f%2e%2e%2fhome/bad_user/evil_script

which your function could translate to pages/../../../home/bad_user/evil_script.php, which include will happily include, sometimes. So your web page when served could very well execute bad_user's php script, which he could use to do all kinds of nasty stuff.

At the very least you should assign $_GET['path'] to a new variable and addslashes().

ithcy
using addslashes() will do nothing to a unix path like ../../foo.php. addslashes() is only meant to be used to escape strings used for db queries, so it puts a slash in front of quotes and backslashes.
Chris Kite
true, i didn't remember addslashes() correctly.
ithcy