Securely instantiate PHP class from URL get parameter?

Issue

I’ve written a little code-snippet that should instantiate a php class based on a get-parameter.

(Code edited based on the suggestions from @sietse85 and @CBroe:)

  $this->pageVal = preg_replace('/[^A-Za-z]/', '', filter_input(INPUT_GET, 'page')) ? preg_replace('/[^A-Za-z]/', '', filter_input(INPUT_GET, 'page')) : "index";

  $file = $this->moduleDir . $this->pageVal . ".php";
  if (file_exists($file)) {
    require_once $file;
    $class = new $this->pageVal($this);
  } else {
    header($_SERVER["SERVER_PROTOCOL"] . " 404 Not Found", true, 404);
    $this->loadPage("404");
  }

In this similar question is warned to do something like this:
Call PHP function from url?

In other questions people sometimes get warned when they use unsecure code – now I tried to remove some security issues (in my code) based on these warnings by using filter_input and requiere only files that exist. Perhaps this is not enough or not the right procedure?

Should I whitelist existing pages and possible parameters too or do something else to avoid security issues or would this not be necessary?

Like this:

$existingPages = ["index", "profile", "login", "register"];

if(in_array(filter_input(INPUT_GET, 'page'), $existingPages)) {

  //GO ON WITH PROCESSING
  $this->pageVal = filter_input(...)

}

If the background of my question is not clear from your point of view please describe the problem to help me to specify it.

Thank you!

Solution

I tried to remove security issues (in my code) based on these warnings by using filter_input

Sorry – you didn’t remove the security issues, this code is still vulnerable to directory traversal. The filter_input() function is IMHO a very inappropriate function. You should never change the representation of input but you should validate it. However filter_input() needs to be told how you want to validate it. And it doesn’t have an option for a partial filename.

Should I whitelist existing pages

If you fix the directory traversal issue, then you have whitelisted the scripts which can be run – it will only be possible to run scripts in the designated directory.

Consider:

$this->pageVal = basename($_GET['file']);
$file = $this->moduleDir . $this->pageVal . ".php";
if (!is_readable($file)) {
    trigger_warning("User attempted to access non-existent code: " 
      . base64encode($_GET['page']), E_USER_WARNING);
    $this->pageVal = 'index';
    $file = $this->moduleDir . $this->pageVal . ".php";
}
require_once $file;

Using a regex to remove/retain arbitrary characters is not an elegant solution.

Answered By – symcbean

Answer Checked By – Mary Flores (AngularFixing Volunteer)

Leave a Reply

Your email address will not be published.