Security considerations behind this code snippet in PHP

Question

Hi all,

I've reading some books on advanced PHP, and most of the time I find code like this:

$classes = array ("MyClass1", "MyClass2");

if (!in_array ($_GET['class'], $classes))
    throw new Exception ("Class not found!");

$params = $_GET;

$obj = new $_GET['class'];

if (!method_exists ($_GET['method'], $obj)
    throw new Exception ("Method not found!");

echo $obj->{$_GET['method']}();

On the book where I find this code, the author always mentions that this code is not secure for production environments, and the class name should be checked.

My question is, if the class name is being checked it's existence in a array (I added this, it wasn't in the book examples), what security considerations should I take more? The class name will bot be outputted so XSS filtering doesn't make much sense. Also the class name will not hit the database, so SQL injection filtering it's not needed.

Thanks in advance for all your answers.

Answer 1

You adding the check to see if the class is one of a limited set of values fixes the security issue you mention. This is known as a "whitelist" approach (where everything is by default disallowed except what you specifically allow) and is the right approach for this kind of thing.

The method GET parameter may have the same issue but I can't think of any circumstances where that might be exploited. After all a class only has a limited set of methods, particularly if they're your classes. Still it might be worth filtering them somehow just to be safe, like you can only call methods beginning with "z" or something.

Answer 2

It is open to code injection exploits.

You should always sanitize any data returned from a $_GET for this reason and never trust that the user input is free of executable code (or some other badness).

See http://stackoverflow.com/questions/38875/best-way-to-avoid-code-injection-in-php on this site which covers a lot more about code injection and sanitizing inputs.

Answer 3

While the class name is being checked through the array, the method name isn't. You could, potentially, have something like

class Foo
{
    function reset()
    {
        // rm -rf DocumentRoot
    }
}

Which, while unlikely, could potentially be there.

Put simply, never trust user input. In this case, I'd give a list of valid functions that can be called.

But then again, I'd never use code like this, I'd probably do some URL <-> function mapping

Answer 4

i didnt see a problem. but you can add a filter_var function on the $_GET , link use it : http://net.tutsplus.com/tutorials/php/sanitize-and-validate-data-with-php-filters/