possible injection when including files

[charlestonolaes]

Hi, what are the possible security risks when including files based on post variables?

here's part of my script

//PATH is the absolute path to the file... ex: /home/usr/public_html/site
$file = trim( $_POST['load'] );
$path = PATH . "/$file.php";

if(file_exists(path )){
include $path;
}

What security measures can I take to make this more secure? Should I just sanitize the variable with filter_var()?

[Cups]

You would be trying to defeat anyone trying to attempt directory traversal, so anything a containing a suspicious pattern of dots and slashes should be nuked - unless you maintain a white-list of files which are allowed to be loaded.

You could make this check simpler if you can append the ".php" yourself.

Then you just the existence of a dot should be enough for you to fail early, and get rid of this user.

$var = './bad';
if(strpos( $var, '.') !== false ) echo 'send away'; 

Here is an example of the white-list filtering method.

$permitted = array('about', 'home', 'departments');

if( isset($_POST['load']) && in_array($_POST['load'], $permitted) ) {

include PATH . '/'. $_POST['load'] . '.php';

} 

[charlestonolaes]

You would be trying to defeat anyone trying to attempt directory traversal, so anything a containing a suspicious pattern of dots and slashes should be nuked - unless you maintain a white-list of files which are allowed to be loaded.

You could make this check simpler if you can append the ".php" yourself.

Then you just the existence of a dot should be enough for you to fail early, and get rid of this user.

$var = './bad';
if(strpos( $var, '.') !== false ) echo 'send away'; 

Here is an example of the white-list filtering method.

$permitted = array('about', 'home', 'departments');

if( isset($_POST['load']) && in_array($_POST['load'], $permitted) ) {

include PATH . '/'. $_POST['load'] . '.php';

} 

That is awesome! I would probably do a preg_match on patterns like dots or slashes, thanks man!

[Cups]

That is awesome! I would probably do a preg_match on patterns like dots or slashes, thanks man!

No prob!

Yeah, you got it - my pastebin app while nice, does not handle forward slashes.

Someone else may have more to say on this. I must admit that including a file depending on a var coming from the internet gives me the willies, but I guess there is enough to keep you safe there.

Anyone else?

[ahundiak]

You can go one step further and transform the posted value into a completely unrelated file name:

$permitted = array('about', 'home', 'departments');
// Could be
$permitted = array('about' => 'AboutFileName.php', 'home' => 'HouseFileName.doc', 'departments' => 'ListOfDepartments.xml'); 

[charlestonolaes]

Creating a whitelist sounds much simpler but there are like 30+ files in different folders so I figured It would be easier to just look for unwanted patterns. Should I also check for url patterns like http?

[Hammer65]

Creating a whitelist sounds much simpler but there are like 30+ files in different folders so I figured It would be easier to just look for unwanted patterns. Should I also check for url patterns like http?

You wouldn't have to have a file whitelist necessarily. Depending on the directory structure you are using you could confirm that the file is in a specific directory. If it is then include it if not don't.

[Cups]

Yeah, maintaining a white-list does seem an overhead you may not need - but if you find yourself maintaining some kind of menu system - then maybe it could be an offshoot - especially if you bear AHundiaks idea in mind. eg

$permitted = array(
'Nice name' => 'real_file_name',
'About us' => 'AboutFileName.php',
 ); 

[charlestonolaes]

cool, thanks to everyone for their help. I've decided to check for any characters like dots or slashes and I also check if the file exists.

//$view is the post variable

if( !preg_match( '~^[http]|([\.|\\|/])|[0-9]~', $view, $match ) ){

//some stuff
    
     //$path = FULL_PATH . $view . '.php';
    if( file_exists($path) ){
           include $path;
    }

} 

[deathshadow60]

I just str_replace "/" -- no slashes, no directory changing -- no slashes, periods won't actually change directory either, it just breaks the path...

Cups method of checking against an array is a REALLY good idea, and I would suggest that what you do is use "glob" to make a list of valid choices. Just put all your sub-files that are valid choices in a directory (keeping it 755 or even 644) and then glob in your list.

$dirList=glob('includes/*.php');
$includeList=array();
foreach ($dirList as $data) $includeList[]=pathinfo($data,PATHINFO_FILENAME);

...

Then later on just:

if ($target=array_search($_POST['load'],$includeList) {
	include('includes/',$includeList[$target],'.php');
}

...

This means NOT sending $_POST to the include, and instead the value pulled from the filesystem -- that way no comparison trickery can be pulled to trick you into sending a invalid $_POST to the include or to file_exists.

Which is probably as secure as you're gonna get on doing that. It also means that by comparing to the proper list and using the proper list, you don't have to bother stripping out slashes or periods -- as they aren't in the set.

Another approach that might have some merit is to set them as the key in an array, as then you don't need to search, just check isset.

$dirList=glob('includes/*.php');
$includeList=array();
foreach ($dirList as $data) $includeList[pathinfo($data,PATHINFO_FILENAME)]=true;

if (isset($includeList[$_POST['load']])) {
	include('includes/',$includeList[$_POST['load']],'.php');
}

Either way is a decent approach.

[charlestonolaes]

oh cool, I never know glob() existed till now, thanks for that. I'll give that a go. thanks man

[the182guy]

Just create a simple function using regex or substr() on each character to ensure only a-z0-9. are possible characters.

[deathshadow60]

Just create a simple function using regex or substr() on each character to ensure only a-z0-9. are possible characters.

Also a decent approach -- though not sure what good substr would be for that -- wouldn't string as array indexing be better/faster? or again, str_replace since if the undesired characters (period and slashes) are present, it's an invalid name automatically.

$loadName=str_replace(array('.','/','\'),'',$_POST['load']);

[the182guy]

Also a decent approach -- though not sure what good substr would be for that -- wouldn't string as array indexing be better/faster? or again, str_replace since if the undesired characters (period and slashes) are present, it's an invalid name automatically.

$loadName=str_replace(array('.','/','\'),'',$_POST['load']);

If you didn't want to use regex, substr() can be used to examine each character in the input, to check if it's allowed or not.

A character whitelist e.g a-z0-9 is better than replacing from a blacklist. You're letting special characters in like % which is used by hackers to prematurely terminate a string for LFI exploits.

[deathshadow60]

If you didn't want to use regex, substr() can be used to examine each character in the input, to check if it's allowed or not.

Which would be painfully slow...

... and again, a job for treating a string as an array instead of using substr.
for example (swapping non-whitelist for underscore)

for ($t=0; $t<length($myString); $t++) {
  if (!in_array($myString[$t],$whitelistchars)) {
    $myString[$t]='_';
  }
}

Even so going through a string with user code that way would be PAINFULLY slow compared to even the regex.

A character whitelist e.g a-z0-9 is better than replacing from a blacklist. You're letting special characters in like % which is used by hackers to prematurely terminate a string for LFI exploits.

If you do file_exists and a lfi exploit actually returns positive, there's something horrifically wrong with your directory structure.

Though again, this is why I'd just check against the actual filenames using glob... and not allow the value from $_POST anywhere NEAR the require/include.

[the182guy]

Which would be painfully slow...

... and again, a job for treating a string as an array instead of using substr.
for example (swapping non-whitelist for underscore)

for ($t=0; $t<length($myString); $t++) {
  if (!in_array($myString[$t],$whitelistchars)) {
    $myString[$t]='_';
  }
}

Even so going through a string with user code that way would be PAINFULLY slow compared to even the regex.


If you do file_exists and a lfi exploit actually returns positive, there's something horrifically wrong with your directory structure.

Though again, this is why I'd just check against the actual filenames using glob... and not allow the value from $_POST anywhere NEAR the require/include.

Which is why regex is first choice. file_exists will always return true for a successful LFI attempt.

Why use glob? An a-z regex plus file_exists is sufficient. This is how it is done with the Joomla! CMS. If there are scripts that aren't intended to be included, in the target directory then the only solution is a whitelist of includable files.