PHP security Issue

[mattaproductions]

Yes awhile back when I was working on my website I asked for some help on php.

 

<?php
if (isset($_GET['page'])) {
include("{$_SERVER['DOCUMENT_ROOT']}/includes/{$_GET['page']}.php");
} else {
include("{$_SERVER['DOCUMENT_ROOT']}/includes/home.php");
}
?>

 

it's what I used to pull my pages and I thought it was a nice script until I post it one day because I was having a issue and it comes to find out it's a security flaw. Need help on taking a different approach or how to fix this. I don't know about how PHP....still learning.

 

EDIT: sorry for not posting it code format. fixed!

[.josh]

<?php
        $allowed = array('home','page1','page2','page3'); 
if (in_array($_GET['page'],$allowed)) {
         include("{$_SERVER['DOCUMENT_ROOT']}/includes/{$_GET['page']}.php");
         } else {
         include("{$_SERVER['DOCUMENT_ROOT']}/includes/home.php");
         }
         ?>

[mattaproductions]

That was a fast response, but here's the original thread on another forum. He somewhat explained why it was a flaw, but it doesn't seem like it fixed it.

 

<?php
if (isset($_GET['page']) && file_exists("{$_SERVER['DOCUMENT_ROOT']}/includes/{$_GET['page']}.php")) {
include("{$_SERVER['DOCUMENT_ROOT']}/includes/{$_GET['page']}.php");
} else {
include("{$_SERVER['DOCUMENT_ROOT']}/includes/sorry.php");
}
?>

 

That was the original code.

 

Now he said

 

And it is flawed.

 

For example visit this URL:

 

http://www.mattaproductions.com/includes/../images/banner.jpg

 

Now you didn't put banner.jpg in the includes directory did you? Click on that link then see what the browser does to the URL.

 

But this "trick" isn't really a trick, every developer should know not only why it works but also how it affects the security of their applications and computer systems. This doesn't just happen with URLs, it happens with paths in using files and directories.

 

Can someone better explain?

[roopurt18]

The security flaw is that your original code is blindly calling include() on whatever was passed through the URL.  The URL comes from the user, which means the user can edit it to any path that they believe might exist on your file system.  Without filtering what you call include() on, I can attempt to ask your script to open known files in known locations.  These files could contain passwords, keys, or all sorts of information.

 

CV's code creates an array of allowed values and only opens the page from the URL if it is one of the allowed values.  Therefore users can't attempt to open any random file on your file system.

[darkfreaks]

Try:

<?php

function clean($text){
$text=trim($text);
$text.=strip_tags($text);
$text.=htmlspecialchars($text,ENT_NOQUOTES);
$text.=filter_var($text,FITER_SANITIZE_STRING);/// PHP 5 only
$text.=mysql_real_escape_string($text);
return $text;
}
$server= clean($_SERVER['DOCUMENT_ROOT']);
$page= clean($_GET['page']);
if (isset($_GET['page']) && file_exists("$server/includes/$page.php")) {
include("$server/includes/$page.php");
} else {
include("$server/includes/sorry.php");
}
?>

[Andy-H]

.= 

 

???

That will return the text stripped of different values 5 times in one string...besides, you should clean/validate input and output differently depending on what your using it for...

[roopurt18]

Woe be unto those that use the code posted by darkfreaks.

[MadTechie]

That is a little like.. oow ooow I found a function i'll stick that in as well!

 

testing the concat!

Clean '<b>me</b>' 

 

Clean '<b>me</b>'Clean 'me'Clean '<b>me</b>'Clean 'me'Clean \'<b>me</b>\'Clean \'me\'Clean \'<b>me</b>\'Clean \'me\'

Wow..The clean function is as clean as my office!

[darkfreaks]

ok i redid the function now it returns \//\ which is alot better 

<?php
function clean($text){
$text=trim($text);
$text.=strip_tags($text,'<p><b></p></b>');
$text.=str_replace('/','',$text);
$text.=stripslashes($text);
while(strchr($text,'\\')) {
        $text = stripslashes($text);
    } 
$string="/\<b><p></p></b>\/";
$string.=clean($string);
echo $string;

?>

[Daniel0]

That functions sucks. It doesn't work, and even if you fixed it it would still suck. Please stop posting bad advice all over the place.

[MadTechie]

try something like this

*untested*

<?php
$path = $_SERVER['DOCUMENT_ROOT'];
$extension = '.php';
$redirectPages = array("NotFound" => "sorry.php","Warning" => "sorry.php");
$hiddenPages = array("private.php","hiddencash.php");

if ( preg_match("#^[a-z0-9_-]+$#i",$_GET['page']) )
{
$page = $_GET['page']
$filename = $path.$page.$extension;
if(file_exists($filename) && !in_array($page,$hiddenPages))
{
	include($filename);
}else{
	include($pages['NotFound']);
}
}else{
//add to security log
include($pages['Warning']);
}
?>

[mattaproductions]

Thanks you guys for the help. MadTechie I will try that when I get back up. I just got home from work and it's 5:11 a.m. Sucks! Anyways thanks again.

[gevans]

The original post by CV ws the simplest and best solution.

 

<?php
        $allowed = array('home','page1','page2','page3'); 
if (in_array($_GET['page'],$allowed)) {
         include("{$_SERVER['DOCUMENT_ROOT']}/includes/{$_GET['page']}.php");
         } else {
         include("{$_SERVER['DOCUMENT_ROOT']}/includes/home.php");
         }
         ?>

 

You've made an array of the pages that are allowed. If the GET value isn't in the array someone's being naughty so just chuck them to the home page, simple, easy and secure.

[.josh]

It is the simplest, and as secure as you can make it.  It's a whitelist. You're not on the list, you don't get in. The End.  But I wouldn't say its the best.  It does not track people who attempt to get around it. MadTechie's latest post takes the whitelist concept and expands on it and adds on to it error/security logging.

[Andy-H]

$page = $_GET['page'] // missed a semicolon here.

[MadTechie]

Meah.. i did say

*untested*

the idea is sound