Is this safe, Security Wise?

FoxhoundX

This is what I am using for a portal type system. Is there any security risks that I have not noticed?

<? $module = $_GET['module']; 
if($module == "") { $module = "Home"; }
if (!file_exists($module . ".php")) {
   echo "Invalid Module.";
}
else {
Require_once($module . ".php");
}
?>

Weedpacket

You're not stopping them from requesting any PHP page whatsoever anywhere on your entire system.

Better to keep a list of PHP pages they are allowed to request, and only allow those pages to be requested.

drawmack

Yeah this could allow for a trickle attack, or maybe if they can locate a copy of phpMyAdmin on your server they can run that, even it is outside of the web folders.

mykg4orce

Create an array of file names and their respective relative names....like so:

$myArray = array( "mainpage" => 'path/to/file.php', "..." => '...');

Then instead of checking whether the file exists....which i hope does anyway, you can check if the key exists in the array...like so:

if (array_key_exists($varBeingCheckAgainst, $myArray)) statement;

where "$varBeingCheckAgainst" would contain "mainpage"....etc.

Weedpacket

The general principle is to be paranoid: don't let your users ask for anything and then try to weed out dangerous stuff - that's a losing battle. Instead, assume that they're going to try something nasty, and only honour requests for things you know are safe. Some guidelines are published by the OWASP. They rate "Unvalidated input" as the number one web application vulnerability.

mtimdog

Also, you may want to try include_once instead of require_once since require_once will always be called, even though it's in the conditional expression. If the condition isn't met, the code in the required file isn't executed....but if they modify it to be something like ?module=blah and you don't have a blah.php file, I'm pretty sure require_once will throw up a fatal error just like require will.

Weedpacket

Originally posted by mtimdog
Also, you may want to try include_once instead of require_once since require_once will always be called, even though it's in the conditional expression.

Not in my experience:

if(1==0)
{  require_once('nonesuch.php');
}
else
{ echo 'Foo!';
}

The only difference bewteen include and require is that the former throws a Warning when the requested file is not found, while the latter throws a fatal error.

swr

If you don't want to maintain a list of valid files, something like this should work (but is untested):

$module = $_GET['module']; 
if($module == "") { $module = "Home"; }
if (!preg_match('|^[\\w\\-\\/]+$|', $module)) {
    die("Invalid Module.");
}
$module = "/path/to/modules/$module";
if (!file_exists($module . ".php")) {
    die("Invalid Module.");
}
else {
    require_once($module . ".php");
}

That will only allow $module to contain letters, numbers, underscores, hyphens, and slashes. By disallowing everything else, including dots, it prevents the ".." attack. It also prepends the module directory, so they can't specify relative to root. They can only specify files that are inside your modules directory or a subdirectory of that.

mtimdog

In response to Weedpacket,
it will always be called Prior to PHP 4.0.2. I guess the important part is that I retained something "valuable" from the manual.

Weedpacket

Originally posted by mtimdog
In response to Weedpacket,
it will always be called Prior to PHP 4.0.2. I guess the important part is that I retained something "valuable" from the manual.

Oh, right you are.
'Course, you could also say that continuing to use 4.0.2 is itself a security risk these days... 😉

Weedpacket

Originally posted by swr
....They can only specify files that are inside your modules directory or a subdirectory of that.

I'm not in a position to test, but would this also cover any situations where escape sequences of some type were used in specifying the requested file?

swr

It should. It checks "allowed characters only" rather than "no disallowed characters" so it should be safe, unless there is some way to do escape sequences with alpha-numeric, underscore, hyphen, or slash.

fjharris

swr, is there a reason you save the GET variable again? Is it more efficient?

ie
$module = $_GET['module'];

As well, why use require_once() as opposed to include()?

Thanks,
Fraser

mtimdog

Originally posted by fjharris
swr, is there a reason you save the GET variable again? Is it more efficient?

ie
$module = $_GET['module'];

As well, why use require_once() as opposed to include()?

Thanks,
Fraser

Uses $_GET because register_globals is off

Uses require_once() because he only wants it required once. He could use include_once, there are differences which this thread has established above.

fjharris

mtimdog, I think you misunderstood me.
In my code I would happily use $_GET['module'].
Is there a reason to resave it as $module and then use $module in your code?

Thanks,
Fraser

mtimdog

Of course there's a reason....😃 😃

[edit]look below[/edit]

swr

Couple of reasons.

I find $module easier to type and read than $_GET['module']. If you're going to be using the variable a lot, that is a consideration. Nobody wants their code to be ugly, right?

Also, the next line is this:
if($module == "") { $module = "Home"; }
$_GET is for browser-submitted variables; I consider it bad form to be modifying it. If this were in a function, it would also bad form to be modifying a global variable when a local variable will do.