Reliable and secure php templating system

[HitByLife]

Hello,

My first post in the php forum as I am no expert on php, I come from a graphical environment.

I have been using a simple template system written in php to design smaller websites but one was hacked into and when asked by the host if I am running any scripts, the only thing I could think of was the php script for the template:

Code: Select all

 
 
<?php
 
 if (isset($_GET["page"]))
  $page = addslashes($_GET["page"]);
 else
  $page = 'home';
 
 if (isset($_GET["dir"]))
 {
  $dir = addslashes($_GET["dir"]);
  $file = $dir;
  $file .= '/';
  $file .= $page;
  $file .= '.html';
 }
 else
 {
  $file = $page;
  $file .= '.html';
 }
   include ($file);
 
?>

The host replied that this script would allow remote inclusion of files onto the site and therefore the script should be removed. And there goes my template system...

Now, is this really vulnerable? I followed an article by A List Apart when creating it (http://www.alistapart.com/articles/phpcms/).

If the code is no good as it stands, would there be any workarounds to get this snippet safer?

Many thanks in advance,
J

[AdRock]

Can I just ask what this script is supposed to do?

[Rakuli]

Okay so the code is pretty dangerous because I can just specify any file I want to access on your site and PHP will server it up. This can give access to passwords whatever else you have on your site.

It would be much safer to specify an array of allowed files for your templating system

PHP: Select all

 

 

<?php

 

$pages = array(); // Initialise the page array



$pages['home'] = '/path/to/home.html';

$pages['other'] = '/path/to/other.html';



if (isset($_GET['page'] && isset($pages[$_GET['page']]))

    include($pages[$_GET['page']]);

else

    include($pages['home']);

So this is assuming that you *don't* want to be able to access any file you want from the query string.

By storing all the files you want access to in the $pages array, anything bar the index of the array will default to home.

so http://www.example.com/index.php?page=other
will include $pages['other'] but http://www.example.com/index.php?page=haxedYourSite
will only include $pages['home']

Cheers,

[HitByLife]

Hello, many thanks for replying so quickly.

AdRock, what this script does is calling html files from a template (a php file). The idea is to have one master page so to speak which contains the design of the site, and the code is placed on that page so that when you open a page of the type, say, template.php?page=yourpage, yourpage is insterted into the template. Yourpage is a plain html file with your page specific content.

Rakuli, thanks for that. However, then you must specify *all* the files that *can* be accessed, which can get pretty tedious if you have 100 pages or more to include. No?

[c010depunkk]

I wrote a tutorial about this for next month's newsletter. Should be available for reading in the next week or so...

[HitByLife]

Originally Posted by c010depunkk

I wrote a tutorial about this for next month's newsletter. Should be available for reading in the next week or so...

Then I'd better pay attention!

[AdRock]

Originally Posted by J. Anthonisen

The idea is to have one master page so to speak which contains the design of the site, and the code is placed on that page so that when you open a page of the type, say, template.php?page=yourpage, yourpage is insterted into the template. Yourpage is a plain html file with your page specific content.

I have done what you want and I use it for my site currently

This a small version of what I think you are trying to do

This is where you want the content to change

PHP: Select all

<div id="content3">
      <?php
       switch ($_GET['page'])
           {
   //Main website pages
       case "aboutus":
        include('aboutus.php');
        break;
       case "monthlysignin":
        include('monthlysignin.php');
        break;
       case "staff":
        include('staff.php');
        break;
             default:
        include('home.php');
           }
          ?>
  </div>

You'll see that I have a default page so if the user enters anything else, the home page is displayed

and how you would use the links

HTML: Select all

<div id="left">
      <ul id="menu">
   <li><a href="index.php">Home</a></li>
   <li><a href="index.php?page=aboutus">About Us</a></li>
   <li><a href="index.php?page=monthlysignin">Monthly Sign-in</a></li>
   <li><a href="index.php?page=staff">Staff</a></li>
   <li><a href="index.php?page=donate">Support Us</a></li>
   <li><a href="index.php?page=contactus">Contact Us</a></li>
      </ul>
  </div>

[Rakuli]

Yes it will be a large array but just including any file sent in the query string is asking to be haxed.

If you created a uniform naming structure for your files and directories you could simplify it somewhat.

EG.

PHP: Select all

<?php



$fileNames = array(); // Initialise the page array



$dirs = array(1 => 'path/one', 'path/two', 'path/three';





$fileName['index'] = '/index.html';



$fileNames['other'] = '/other.html';



if (isset($_GET['directory'] && isset($dirs[$_GET['directory']]))



    include($dirs[$_GET['directory']] . (isset($_GET['fileName'] && isset($fileNames[$_GET['fileName']]) ? $fileNames[$_GET['fileName']] : $fileNames['index']);



else



    include($dirs[1] . $fileNames['index']);

[HitByLife]

AdRock, that is basically what I have been doing, yes, but with the specific code I posted above and with links looking like this:

/template.php?page=somepage
/template.php?dir=somedirectory&page=somepage

Rakuli, I will give your alternative some thought.

Someone suggested this rewrite to me - what do you think of it?

PHP: Select all

<?php
 
if (isset($_GET["dir"]))
{
$dir = addslashes($_GET["dir"]);
 
$dir = $dir ."/";
if (is_dir($dir))
{
$file = 'http://www.yoursite.com/';
$file .= $dir;
$file .= $page;
$file .= '.html';
}
else
{ 
// You have no access here!
exit;
}
}
else
{
$file = 'http://www.yoursite.com/';
$file .= $page;
$file .= '.html';
}
include ($file);
?>

[Rakuli]

That last solution would be safer but still gives access to any html file on your site which I guess is less of a problem.

Probably better to use paths though instead of URL's.

EG

$file = 'path/to/tfile'; etc to avoid someone sending some encoded URL characters to cause a loop in your script.

[HitByLife]

Rakuli, thanks.

Most of the sites I have under development at the moment have no sensitive data stored, which currently goes for the site that got hacked into as well. What the hacker did there was to place a massmailer script and later on he used the site for phishing (not sure it was the same hacker). The result for me was a suspension of the service. Luckily it was not a clients site, but a personal site.

Now, will the above script stop the hacker from placing files on my server?

[HitByLife]

AdRock, can I ask something? I notice you use 'break' in your code - what's that for?

[Rakuli]

If that is all your script is doing with the query string then a hacker should not be able to place a file on your site with it.

PS - the switch statement is like an if else

swicth ($condition)
{
case 'this1' :
do this
break; // don't do any more with this1
case 'this2' :
do this
case 'this3' :
do this ( with thi2 && this3)
break; // no more with this 2 & 3
default :
do this cause there was no match
}

It's a structure from C, faster and more compact than a big if else statement.

[AdRock]

Originally Posted by J. Anthonisen

AdRock, can I ask something? I notice you use 'break' in your code - what's that for?

The break gets out of the switch statement because a condition has been met

[c010depunkk]

Like I said earlier, if you can wait till the next CC Newsletter comes out, I have a beautiful, elegant, dynamic solution to exactly this problem.

But you'll have to wait a few days, I'm not allowed to give away the goodies ahead of time