Take the 2-minute tour ×

Does the following include statement present a security risk?

include "pages/".$_GET["page"].".php";

If yes:
- why?
- what would be a safer approach?

share|improve this question
add comment

5 Answers

up vote 3 down vote accepted

Yes. For starters, you should never be using GET variables directly without some kind of validation, ever.

In addition, you shouldn't allow arbitrary specification of a path to include.

Instead, you should either restrict includes to paths within a certain directly (and check that the specified path refers to a file in that directory) if you absolutely require that much dynamic-ness, or you should instead pass tokens that refer to things you'd want to include (and then do something like a lookup into an associative array to see what file a given token refers to).

An example of the latter would be something like this:

$allowed_pages = array(
  "page1" => "pages/page1.php",
  "page2" => "pages/page2.php",
  "foobar" => "pages/page7.php",
  "stuff" => "pages/blarg.php"

$page = $_GET['page'];
if(array_key_exists($allowed_pages, $page)) {

(In this case, the fact that only specified keys are allowed acts as both validation and restriction on what may be included.)

share|improve this answer
More examples on how to exploit this kind of things here : ha.ckers.org/blog/20100128/micro-php-lfi-backdoor the comments have a lot of things too. –  Arkh Jan 29 '10 at 13:10
add comment

It is risk because in $_GET["page"] can be a path that you do not except - in example ../../settings.php on something else.

It should be done like:

$allowedPages = array('news', 'contact', ...);
if ( in_array($_GET["page"], $allowedPages) ) {
} else {
    throw new Exception('Page is not valid !');

Good thing also is to check if file exists.

share|improve this answer
add comment

You're essentially giving them the ability to execute ANY piece of PHP that exists on your system. That's a pretty big unknown.

Fundamentally this is risky because even if there is no "dangerous" code to point to now, there may be in the future.

share|improve this answer
They can read any file, because include in php doesn't have to contain php code. If there is no <? ?>, then php will just dump the contents of the file in there. –  SoapBox Jan 29 '10 at 11:59
Definitely should have noted that as well! –  altCognito Jan 29 '10 at 12:56
add comment

As well as having a white list, you could do something like this

$include = $_GET['page'];

if ( ! preg_match('/^[a-z-_]+$/i', $include)) {
    throw new Exception('Access denied');
share|improve this answer
add comment


The fact that you've hard-coded a prefix prevents attacks like:


But because $_GET['page'] may include '..' then someone can force inclusion of any file with a php extension from your system. Are you confident that this will never result in a security compromise?

Using a whitelist as suggested by others is a lot safer, and also removes the requirement to prefix the path to avoid remote inclusion vulnerabilities.


share|improve this answer
add comment

Your Answer


