CODEWITHSUNDEEP
phpsecurity
Dunderklumpen
Dunderklumpen

Reputation:

Dynamic Include Safety

Is there any way to safely include pages without putting them all in an array?

if (preg_match('/^[a-z0-9]+/', $_GET['page'])) {

$page = $_GET['page'].".php";
$tpl = $_GET['page'].".html";
if (file_exists($page)) include($page);
if (file_exists($tpl)) include($tpl);

}

What should I add to make this pretty safe?

I'm doing it this way bacause I don't like having to include stuff that has to be included on all pages. The "include header > content > include footer"-way. I don't wanna use any template engines/frameworks neither.

Thanks.

Upvotes: 3

Views: 2738

Answers (4)

Bretticus
Bretticus

Reputation: 900

I agree with Unkwntech. This is such an insecure way to include files into your website, I wish PHP programmers would do away with it altogether. Even so, an array with all possible matches is certainly safer. However, You'll find that the MVC pattern works better and it is more secure. I'd download code igniter and take a tutorial or two, you'll love it for the same reason you wanna use dynamic includes.

Upvotes: 0

Gumbo
Gumbo

Reputation: 655209

The weakness in your current implementation is that …

  1. the regular expression just tests the beginning of the string, so “images/../../secret” would pass, and
  2. without further validation, “index” would also be a valid value and would cause a recursion.

To make your implementation safe, it’s a good practice to put everything, that’s intended to be included, in its own directory (e.g. “includes” and “templates”). Based on this, you just have to ensure that there is no way out of this directory.

if (preg_match('/^[a-z0-9]+$/', $_GET['page'])) {
    $page = realpath('includes/'.$_GET['page'].'.php');
    $tpl = realpath('templates/'.$_GET['page'].'.html');
    if ($page && $tpl) {
        include $page;
        include $tpl;
    } else {
        // log error!
    }
} else {
    // log error!
}

Note: realpath returns the absolute path to the given relative path if file exists and false otherwise. So file_exists is not necessary.

Upvotes: 6

Syntax
Syntax

Reputation: 1314

You should never use user supplied information for includes. You should always have some sort of request handler that does this for you. While a regular expression may filter somethings it will not filter everything.

If you do not want your site to get hacked you do not allow your users to control the flow of the application by designating an include.

Upvotes: 1

UnkwnTech
UnkwnTech

Reputation: 90841

Despite what you stated about not wanting to store a list of available pages in an array it is likely going to be the best, non-db, solution.

$availFiles = array('index.php', 'forum.php');
if(in_array($_GET['page'].".php", $availFiles))
 {
   //Good
 }
else
 {
   //Not Good
 }

You could easily build the array dynamicly with either DB queries or by reading a file, or even reading the contents of a directory and filtering out the things you don't want available.

Upvotes: 4

Related Questions