Create a class based on user input

Question

It's not safe to require $input . '.php'. To then institate a class. How can I make it secure, without needing to use say a whitelist of classes that can be inistiated.

Ex 1. (bad code).

<?php

$input = $_GET['controller'];

require $input . '.php';

new $input;

?>

Answer 1

Disclaimer

I should start by saying that defining static routes in your system is secure by design whereas this answer, even though I've made efforts to mitigate security issues, should be thoroughly tested and understood before trusting its operation.

The basics

First, make sure the controller contains a valid variable name using a regular expression as taken from the manual; this weeds out obvious erroneous entries:

$controller = filter_input(INPUT_GET, FILTER_VALIDATE_REGEXP, [
    'options' => [
        'regexp' => '/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$/',
        'flags' => FILTER_NULL_ON_FAILURE,
    ]
]);

if ($controller !== null) {
    // load and use controller
    require_once("$controller.php");
    $c = new $controller();
}

Enforcing hierarchy

This works well, but what if someone tries to load an internal class instead? It might horribly fail the application.

You could introduce an abstract base class or interface that all your controllers must extend or implement:

abstract class Controller {}

// e.g. controller for '?controller=admin'
class Admin extends Controller {}

Btw, to avoid name conflicts, you could define these inside a separate namespace.

And this is how you would enforce such a hierarchy:

if ($controller !== null) {
    // load and use controller
    require_once("$controller.php");
    if (is_subclass_of($controller, 'Controller')) {
        $c = new $controller();
    }
}

I'm using is_subclass_of() to type check before instantiating the class.

Auto loading

Instead of using a require_once() in this case, you could use an auto loader instead:

// register our custom auto loader
spl_autoload_register(function($class) {
    $file = "$class.php"; // admin -> admin.class.php
    if (file_exists($file)) {
        require_once $file; // this can be changed
    }
});

This is also the place where you can normalize the class name, so that it maps better to a file name, as well as enforcing a custom namespace, e.g. "App\\$class.php".

This reduces the code by one line, but making the loading more flexible:

if ($controller !== null) {
    // check hierarchy (this will attempt auto loading)
    if (class_exists($controller) && is_subclass_of($controller, 'Controller')) {
        $c = new $controller();
    }
}

All this code assumes you have proper error handling code in place; for implementation suggestions you can look at this answer.

Answer 2

A couple of suggestions:

• Put your controller classes in its own dedicated folder, containing ONLY controller classes

• Make your filter as strict as possible eg.

  /* is $_GET['controller'] set? */
  if (!isset($_GET['controller'])) {
      // load error or default controller???
  }
  
  $loadController = $_GET['controller'];
  
  /* replace any characters NOT matching a-z or _ (whitelist approach), case insensitive */
  $loadController = preg_replace('/[^a-z_]+/i', '', $loadController);
  
  /* verify var is not empty now :) */
  if (!$loadController) {
      // load error or default controller???
  }
  
  /* if your classes are named in a certain fashion, eg. "Classname", format the incoming text to match ** NEVER TRUST USER INPUT ** */
  $loadController = ucfirst(strtolower($loadController));
  

• Check if the file exists Why not file_exists? see desc

  /* avoiding using file_exists as it also matches folders... */
  if (!is_file($myControllerClassesPath.$loadController.'.php')) {
      // load error or default controller???
  }
  

• Then require the file, and verify that the class itself exists

  require($myControllerClassesPath.$loadController.'.php');
  
  /* of course, this assumes filename === classname, adjust accordingly */
  if (!class_exists($loadController)) {
      // load error or default controller???
  }
  

• Then of course, new instance of X

  new $loadController;
  

Answer 3

Most anwsers are using a variation of auto_load instead of include to make it more secure. But the provided examples fail to mention that the way they use it, auto_load is just a fancy include. Instead of including the file manually and then calling the class, the file is included automatically. This offers no security advantage as one could still call any available class.

In my opion using include instead of require and then catching the error is the best practise and easiest to implement. To make it secure you must add an extra part to the filenames you allow to be included. EG: "Controller". Now if you have a class called Home, then you call the file homeController.php. This way we can only require files that end with 'Controller.php'.

Just as an extra precaution I added basename() to the input to prevent network access on windows systems

<?php
//EG GET ?controller=home
$input = isset($_GET['controller']) ? $_GET['controller'] : "";
if (empty($input))
  die('No controller');

$input = basename($input);
$filename = $input.'Controller.php';

//since only valid files can be included, you dont need to check for valid chars or anything. Just make sure that only your controller files end with 'Controller.php'
//use the @ to hide the warning when the file does not exist
if ((@include $filename) !== 1)
  die('Unknown controller');

//no error, so we included a valid controller and now we can call it.
$controller = new $input();
?>

keep in mind that if you run on a none Windows server, your filenames are case-sensitive while your PHP classes are not. so if somebody would enter controller=HOME then the include would fail.

You could prevent this issue by makeing all files like 'homeController.php' with a lower case class prefix. Then you can use $filename = strtolower($input).'Controller.php';

Answer 4

Consider using spl_autoload_register(). This would you save a lot of effort in validating files/classes etc.

<?php
function autoloadClasses($class) {
    if (file_exists('core/'.$class.'.php')) {
        include 'core/'.$class . '.php';
    }
}
spl_autoload_register('autoloadClasses');

?>

Then save a file name dart.php in core-folder (filename and classname has to be the same)

When you then create an object: new dart(); the file will be included when needed.

More information about this: http://php.net/manual/en/function.spl-autoload-register.php

Answer 5

If you have few classes/files, you can get all php files within the folder where classes are stored and check if the class you want to include/require is one of them.

So something like this:

$classDir = '/path/to/classes';
$classList = glob($classDir.'/*.php');
$classAbsolutePath = $classDir.'/'.$_GET['class'];

if (in_array($classAbsolutePath, $classList)) {
    require $classAbsolutePath;
}

If you have sub directories you have to modify this code according to that. By the way, this is not the best solution, regarding performances, especially if you have a lot of files and a lot of subdirectories. Also, in_array() is not very efficient so you should avoid it if you have big arrays.

In my opinion the best way to do something like this is to have a whitelist. You can generate it automatically via code. Every time you rebuild or deploy your project you can regenerate the list so that you will always have a valid one.

Answer 6

I would suggest you to introduce special tag into allowed files. Then before you include the file, read it as plain text and look for the tag. Only if the tag is present, include it. The tag can be inside PHP comment at the beginning of allowed files.

$class = $_GET['class'];
if (preg_match('/^[a-zA-Z]+$/', $class))
{
    $file = $class.".php";
    if (is_file($file)) {
    {
        $content = file_get_contents($file);
        if (strpos($content, "THECLASSMAGIC") !== false)
        {
            require($file);
        }
    }
    else
    {
        die(...);
    }
}
else
{
    die(...);
}

Answer 7

First add this function.

function __autoload ( $class ) {
     $path = "../path/to/class/dir/" . $class . TOKEN . ".php";
     if ( file_exists ($path) ) {
          require_once ( $path );
     } else {
          // class not found.
     }
}

Then simply access class,

$class = new input();

It will check if file "../path/to/class/dir/input_secretToken.php" exists, and include it automatically.

Here TOKEN is a secret word defined in config file, and used as suffix to all class files. So, only class file with token suffix will load.

Answer 8

As far as security goes, there's nothing wrong with accepting resources' identifier from input, whether it's an image or some code. But it is inevitable to avoid some sort of authorization if one is expected (obviously it's a paradox to have authorization but not to have one). So if you insist on not having an ACL (or 'white list' as you call it) I have to say that what you want is not possible.

On the other hand, if you can come to terms with ACL, then the rest is straightforward. All you need to do is to see your controllers as resources and group your users into roles (this last part is optional). Then specify which role or user can access which controller. Here's how it's done using Zend Framework.

$acl = new Zend_Acl();

$acl->addRole(new Zend_Acl_Role('guest'))
    ->addRole(new Zend_Acl_Role('member'))
    ->addRole(new Zend_Acl_Role('admin'));

$parents = array('guest', 'member', 'admin');
$acl->addRole(new Zend_Acl_Role('someUser'), $parents);

$acl->add(new Zend_Acl_Resource('someController'));

$acl->deny('guest', 'someController');
$acl->allow('member', 'someController');

Then when some requests is arrived you can question its authorization simply like this:

if ($acl->isAllowed('currentUser', $_GET['controller'])) {
    $ctrlClass = $_GET['controller'];
    $controller = new $ctrlClass();
}

Assuming that some autoloader is already set.

Answer 9

In what instance are you going to allow a user to instantiate a controller via a query string paramater, but not have an idea of what they’re actually trying to instantiate? Sounds like a recipe for disaster.

Saying that, I’d just restrict input to letters only (assuming your classes are named MyClass.php, MyOtherClass.php etc) and locked to a particular directory.

<?php

$className = $_GET['file'];
$dir = '/path/to/classes/';
$file = $dir . $className . '.php';

if (preg_match('/^[a-zA-Z]+$/', $className) && is_file($file)) {
    require($file);
    $class = new $className;
}
else {
    die('Class not found');
}