Security and includes based on URL data

Question

Does the following include statement present a security risk?

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

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

Answer 1

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)) {
  include($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.)

Answer 2

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) ) {
    include "pages/".$_GET["page"].".php";
} else {
    throw new Exception('Page is not valid !');
}

Good thing also is to check if file exists.

Answer 3

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.

Answer 4

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');
}

Answer 5

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

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

?page=http%3A%2F%2Fwww.blackhat.com%2Fbad.code%3F

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.

C.