hi all - the following code seems good in terms of security (to the best of my knowledge) but it seems to think up to 3-4 seconds before it starts loading, can someone please help and take a look to see if there is another way but serve the same purpose? Your help is appreciated. Thanks.
/*** the array of allowed pages ***/
$ok_p = array('pricing',
'about',
'plan',
'register',
'support',
'terms',
'privacy',
'sitemap');
/*** check if file name is in array ***/
if(!in_array($_GET['pages'], $ok_p)){
echo '<div class=warn><h2>Error!</h2><p>The page entered is unavailable.</p>';
}
else
{
/*** assign the file name ***/
$file = "./pages/".$_GET['p'].".php";
if(!file_exists($file))
{
echo '<div class=warn><h2>Error!</h2><p>The file requested is unavailable.</p>';
}
else
{
/*** include the file ***/
include $file;
}
}
Hell no that isn’t safe. If they know where your php tmp directory is they can post a file against that php file and use your get param to force it to execute, then you’re pwned.
NEVER pass unfiltered user input to an include, eval, system or exec statement.
Hmm, unfiltered…, I thought by using array to identify the file names as well as ensuring the file exists using file_exist, i am good. Can you or someone else expand further?
But i guess what i was trying to find out is if someone can spot the problem and point out why it’s taking so long to load.
Ok. Michael is right that you don’t want to use unsanitized user input to do something like an include, but you are sanitizing this input by restricting it to a pre-determined set. If it’s limited to a pre-determined set of files that you know exist, then the file_exists() is unnecessary. I think your code could be reduced to something like this:
<?php
/*** the array of allowed pages ***/
$ok_p = array(
'pricing',
'about',
'plan',
'register',
'support',
'terms',
'privacy',
'sitemap'
);
// array
if ( in_array( $_GET[ 'page' ], $ok_p ) ) {
include "./pages/{$_GET[ 'page' ]}.php";
}
// if
else {
?>
<div class="warn"><h2>Error!</h2><p>The page entered is unavailable.</p></div>
<?php
}
// else
Not sure why it’s taking so long to load. What environment are you running the script in?
If they successfully inject a file onto your site then it will exist. Suppose I send a post to your script with the file I want to execute on your machine as the upload. PHP will put the file in a temporary directory determined by its config, but the defaults are fairly well known to hackers. At that point they just need to include the file - it exists so file_exists does nothing to stop the attack.
In the code the OP posted, the include is limited to specific filenames in a specific directory. Of course it’s correct that you shouldn’t be doing an include using unsanitized user input that could refer to a file that the user could have injected into the filesystem, and that in that situation file_exists() wouldn’t do anything to protect you.
Hell no that isn’t safe. If they know where your php tmp directory is they can post a file against that php file and use your get param to force it to execute, then you’re pwned.
While I agree more filtering is needed just for safety sake, in this example the OP is effectively filtering by allowing a minimal whitelist through in the first step of validation.
Even if you did inject a PHP script unless it’s name is in the array $ok_p
In anycase, including dynamic template like this is a bad idea in general. You should be pulling static HTML files via file_get_contents() not include() unless it’s absolutely required like in template composition.
This is a common technique in implementing a templated site by new PHP developers and will likely bite you in the butt eventually. My biggest worry, is that those included files are not properly protected via file permissions, and someone on your shared server adds some sneaky code. Or worse, someone finds a whole in your code somewhere else and over writes one of the included files whitelisted as “safe”.
If they’re static files, then by all means, use file_get_contents(). But if they’re PHP scripts then obviously include() is called for.
In anycase, including dynamic template like this is a bad idea in general.
That’s a very broad statement. I wouldn’t say that in general. It depends on the use case.
This is a common technique in implementing a templated site by new PHP developers and will likely bite you in the butt eventually.
It will likely burn you? I wouldn’t say that either.
My biggest worry, is that those included files are not properly protected via file permissions, and someone on your shared server adds some sneaky code.
That’s a legitimate concern in general, and something the OP would be wise to consider, but what leads you to believe that those files in particular are likely to have permission problems? If the script that pulls them in [using include() or file_get_contents()] had a permission problem, then that script could be compromised itself.
Or worse, someone finds a whole in your code somewhere else and over writes one of the included files whitelisted as “safe”.
The problem there would be the permissions on those files and the insecure script that allowed them to be overwritten, not the legitimate use of include() (assuming it is legitimate).
So to avoid this solutions like smarty get tried, and much later you realize that maybe include wasn’t the enemy - it was misuse of include. After all, include is at the heart of my template engine…
protected function parseTemplate($template, array $output = array() ) {
// Extract the global output first.
extract( $this->getArrayCopy() );
// Now the template's local output, which overwrites any global stuff.
if (!empty($output)) {
extract( $output );
}
ob_start();
if (!include($this->getTemplate($template))) {
throw new FatalException('Unable to read template file '.$template);
}
return ob_get_clean();
}
What’s going on here is more elaborate than what the OP is doing by a fair stretch - and by doing the include in a function scope I control the variables that file gets to see. Not only that but the logic by which the template is determined is more sophisticated which is to be expected - I’ve been doing this 7 years now.
Include isn’t what kills - it’s getting scope muddled up and losing track of what is going on in each include that causes problems.
I never intended to suggest “include” was evil - I know from past experience that what OP described is likely a mild misuse of include. Include itself is a absolute required feature of PHP no doubt.
It’s commonly used in most popular template engines, even Smarty at it’s core.