3
if (file_exists("pages/$page.php")) {
  include($page.'.php');
}

Is this safe?

With Safe i mean that you cant include remote scripts etc

1
  • I think your specific code is safe since you are always checking for a PHP file, and inside the pages directory. Just make sure you don't miss them out accidentally somewhere. Aug 27, 2009 at 16:53

6 Answers 6

7

Certainly not, especially if $page = "./configuration"

I would recommend replacing it with something like this:

$pages = array("blog", "home", "login");
if (in_array(strtolower($page), $pages))
  include("pages/$page.php");

EDIT: You can generate that list of acceptable pages with this code.

$pages = array();
if ($dh = opendir("pages")) {
  while (($file = readdir($dh)) !== false) {
    if (strstr($file, ".php") !== false) // make sure it is a .php file
      $pages[] = substr($file, -4); // remove the .php
  }
  closedir($dh);
}
3
  • is there a way not to have to add all pages to an array?
    – Tony
    Aug 27, 2009 at 16:40
  • 2
    The point is, you want to list your pages in the array. So you can limit what you can access. Let me give you some alternate code. Aug 27, 2009 at 16:43
  • if you want to be safe, then no
    – knittl
    Aug 27, 2009 at 16:45
2

The code you posted has a typo i believe. It should be:

if (file_exists("pages/$page.php")) {
  include("pages/$page.php");
}

It however leads to code injection, if PHP settings allow it, remote file inclusion.

You need to make sure the page that you include can not be any arbitrary page.

Usually you'll see this type of code in a "Loader" class employing the Factory Method, however, in good implementations it restricts the files and classes it will load to a certain directory, or to a certain predefined set of files.

4
  • I am interested in what values of $page would lead to remote file inclusion. If you didn't have 'pages/' at the beginning then you could maybe inject '../' but otherwise I don't see how it's dangerous. Aug 27, 2009 at 16:51
  • 2
    ../ can still be injected with pages/ at the beginning: pages/../../../../path/to/important/file.
    – ceejayoz
    Aug 27, 2009 at 16:53
  • You can include any file on the file system readable by the PHP process and thus pretty much have all access the PHP process has, including it's ability to make a HTTP request to a remote site for some worm or worse.
    – bucabay
    Aug 27, 2009 at 18:01
  • Everything the web server writes to becomes a target since you now have the ability to read it from PHP. Log files, uploads, caches, database files, all take user input, directly and indirectly.
    – bucabay
    Aug 27, 2009 at 18:08
1

If $page is never set, PHP will then try to find what it could be following the variables_order directive inside your php.ini. This directive tells PHP the order in which to find variables. Since the default for this is EGPCS, a cunning hacker then then call your script and tell it to include any file that PHP has access too.

Ex:

www.example.com/?page=dbConfig.ini

1

Storing all possible page names in an array is the safest approach, but you can also be reasonably safe by simply validating the supplied page name and ensuring that you don't have any "dangerous" files in your pages directory.

$page = basename($_GET['page']);
if (file_exists("pages/$page")) {
  include("pages/$page");
} else {
  include("pages/default.php");
}
1

Use basename($_REQUEST['page']) to prevent potential access to other directories then check if it exists.

http://php.mirror.facebook.net/manual/en/function.basename.php

0

As ceejayoz commented in bucabay's answer, the requested page could have "../" which allows a user to easily break out of where they should be. My answer to another question should serve you well.

https://stackoverflow.com/a/12361051/1524693

If the link dies: Basically you check the realpath() of the include directory and the requested file, if the realpath() of the file starts with the realpath() of the include directory, it is safe to include. (I used strpos()==0 to check if the file path starts with the include path)

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.