Secure include?

[Maggan]

Was wondering if anybody would care to look over this include script snippet, using the controversial index.php?x= and argue if it's secure or not.

<?php

		function antihack($value)
{
     $value1 = str_replace( "http", "<b>Huh?!</b> ", $value );
	 $value2 = str_replace(":", "<b>Trying to hack me?</b> ", $value1 );
     $value3 = str_replace( "//", "<b>Sucks to be you</b> ", $value2);
     $value4 = str_replace( "www", "<b>neener, neener</b> ", $value3);
     return $value4;
}
			$page="main.php";
			$content=$_GET['x'];

		if ($_GET['x'] == "") $content = $page; 
		else $content=$_GET['x']; 
		$content = antihack($content); 
		include($content);
		?>

[devinemke]

a simpler and more secure idea is to simply place all of the allowable include files in an array and use [man]in_array[/man] to validate. another thing i usually do is have the GET var not be the actual file name but just a pointer that i use the constuct the included file, like this:

// the GET var is called "page" and equals "products"
include('main_' . $_GET['page'] . '.php'); // the included file is main_products.php

[Maggan]

Originally posted by devinemke
a simpler and more secure idea is to simply place all of the allowable include files in an array and use [man]in_array[/man] to validate. another thing i usually do is have the GET var not be the actual file name but just a pointer that i use the constuct the included file, like this:

// the GET var is called "page" and equals "products"
include('main_' . $_GET['page'] . '.php'); // the included file is main_products.php

[/B]

Oh, I've used that type plenty of times but for it has too many limits particularly for the current project I'm working on.

The project deals with database content called upon through $_GET.

Further more, it has an easy-to-use admin panel for the non-html savy which creates new databases for each new project they've made. My guess is it would be all moot if you created an array to include all files plus their user-created variables.

I'm really curious to find out if there's a security flaw to the code written above.

[Weedpacket]

You missed "..". Users won't be able to create databases with names that have "www" in them. You don't check that $_GET['x'] exists before using it.

[Maggan]

Originally posted by Weedpacket
You missed "..". Users won't be able to create databases with names that have "www" in them. You don't check that $_GET['x'] exists before using it.

I'm slow today. I missed ".."

What would be risky about that? And yeah, I did consider the flaws with the users not being able to create a database with www in it, but for this script's particular use I don't think that will be an issue. However, if I remove that string replace altogether, will that be a security risk? Can PHP include a remote file without the [url]http://?[/url]

I'll work in an if statement. problem is my brain is fried and all it wants to do is check $content exists. So how exactly do I check if $_GET['x'] exist?

ETA: Wait, I thought that was what I was doing albeit a bit unconventionally here:

if ($_GET['x'] == "")

Also, thanks for moving the thread to the proper place :-)b

[onion2k]

If one of your include files is called "http_explained.php" for example, your code will stop it being included despite being a perfectly valid file.

I also don't understand why you're running antihack() twice.

Plus, if the file is a 'hack', your include() statement will throw a warning.

[Maggan]

Originally posted by onion2k
If one of your include files is called "http_explained.php" for example, your code will stop it being included despite being a perfectly valid file.

Something that really won't be a problem with the collected script's intended use.

I also don't understand why you're running antihack() twice.

Me neither. Was very tired when I wrote this last night. Removed now anyway.

Plus, if the file is a 'hack', your include() statement will throw a warning.

Yes...

Does anyone have an idea of what (preferably as specific as possible for this amateur) the security issues with this script might be? I would really appreciate some views on that.

[devinemke]

i think what onion2k means is that your function only throws errors but does not halt the include. i would suggest that you have your antihack function return a simple boolean so that you can logically trap the error:

<?php
function antihack($string)
{
	$bad_stuff = array(
		'http',
		':',
		'//',
		'www'
	);

foreach ($bad_stuff as $value)
{
	if (strstr($string, $value)) {return FALSE;}
}

return TRUE;
}

if ($_GET['x'] == '') {$content = 'main.php';}
else {$content = $_GET['x'];}

if (antihack($content)) {include($content);}
else {echo 'Houston...we have a problem';}
?>

[PHPMagician]

Are includes the best way to do this in the first place? I mean, I don't know excactly what you are trying to do, but I find often it is inappropiate to use includes for simple things. Also, is the code gonna work within and control structure (ie. if).

[Maggan]

Originally posted by PHPMagician
Are includes the best way to do this in the first place? I mean, I don't know excactly what you are trying to do, but I find often it is inappropiate to use includes for simple things. Also, is the code gonna work within and control structure (ie. if).

Well, since the only other option I could think of was iFrame, I would say so. It may not be necessary, but it's a preference. Plus, the rest of the script's in PHP and using sometimes specific includes.

The snippet I wrote up here is used on the indexx.php page, after the splash page, to call on content. Some of this content is php files with various content retrieved from databases through $_GET variables.

And no, the particular place this snippet's written is not within another if statement.

Thanks for that snippet devinemke,

a complete halt and a proper warning message does make more sense.

[rehfeld]

the thing is even though your checking for bad stuff, your still betting you have considered all possibilites. the hacker may easily know a few more tricks than you do.

if you MUST construct your include using the GET variable directly, this would be pretty safe, as long as you keep php files you dont want included out of the pages/ dir

it will prevent ../../config.php type attacks

$include_dir = 'pages/';
$include_file = $include_dir.'/'.$_GET['page'] . '.php';


if ($include_dir === dirname($include_file)) {
    if (is_readable($include_file) && is_file($include_file)) {
        include($include_file);
    }
}

but the BEST thing is too simply NOT allow ANYTHING you have not SPECIFICALLY allowed, like this

$available_files = array(
    'homepage.php',
    'page1.php',
    'path/to/page2.php',
    'static.html',
);

$include = 'homepage.php'; // default

if (isSet($_GET['page'])) {
    if (in_array($_GET['page'], $available_files)) {
        $include = $_GET['page'];
    }
}

if (is_readable($include)) {
    include ($include);
} else {
    exit('page not avail, the webmaster forgot to upload the file even though he defined it as an available file lol');
}