Safe Dynamic Include

Question

I been using this php dynamic include code on my site. But I think it not safe, how can write safer and better code to replace this:

$page = (empty($_GET['page'])) ? '' : $_GET['page'].".html";

if (empty($page))
{ 
 $page = 'index.html';
}

else
 {
 $page = $page;

 }

 include($page);

Thank you very much

Answer 1

You can define some sort of whitelist to determine what files are allowed to be included in this fashion and check that list before doing the include.

Another way would be to only allow includes within a certain directory (or child directories) and verify that the requested files are within that directory.

Answer 2

In terms of security, always allow policy is a bad policy. Do not assume that the request is valid and safe. Always deny upfront, and use a whitelist:

switch($_GET['page']): 
   case 'page-a': case 'page-b': case 'other-page':
      include $_GET['page'] . '.html';
      break;
    default: 
      include 'index.php';
endswitch;

If the whitelist is hard to maintain, try to narrow the possibilities to a single path, use basename:

$name = basename($_GET['page']);
include 'includes/' . $name . '.html';

This way you don’t have to worry to much about security, as long as you keep all the contents of this directory (and all include paths) safe (notice that someone could upload a compromised file to that directory).

If the above fails, try to use realpath() and make sure that the file is in your specified directory tree.

Answer 3

I would recommend, checking for invalid file characters in the $page var (ie \ / : * < > ? ) and also check if the files exists using php's file_exists() method.

Something like

$bad_chars = array("\\", "/", ":", "*", "?", ">", "<", "Foo", "Bar");
$file = str_replace($bad_chars, "", $file);

if (!file_exists('/' . $file))
{
    echo 'bad file name';
}

Answer 4

What happens when someone puts "http://example.com/unsafe_file" in the page parameter? Your script will probably go out and happily download the remote unsafe script, and then execute any php code that it finds there. That could include, among other things, an exploit to own your server. Or print your database passwords. Or reveal sensitive information. The bottom line is, this kind of include is definitely not safe, and shouldn't be used.

Instead, you should use a whitelist of pages to allow and check the paramter against that, or retrieve the page contents form a database. But just including files should not be used.

Answer 5

In general, I would not make any calls to include or require using unsanitized user input. It's just too risky. What if the user requests a file that's outside of the web root?

You should either limit what can be included using a white list, or something else that doesn't require user input to such a powerful call like include or require. Otherwise, you open yourself up to Remote File Inclusion attacks. There are ways to mitigate RFI, by disabling allow_url_fopen, but it's best just to avoid the situation altogether.

Answer 6

You can use preg_replace() to make sure that the page name doesn't contain anything harmful:

// $page can only contain letters, numbers and '_'
$page = preg_replace('/[^a-zA-Z0-9_]/', '', $_GET['page']);
include "pages/$page.html";