Securely instantiate PHP class from URL get parameter?

Question

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!

Seeing as it looks like you did not currently take into account even essential basics such as a path traversal attack, I think _you_ should probably rather go with a whitelist ... — CBroe, Apr 26 '18 at 11:55
make sure you remove dots and slashes from this get parameter. Actually try to only allow A-Za-z `$this->pageVal = preg_replace('/[^A-Za-z]/', '', $this->pageVal);` — sietse85, Apr 26 '18 at 11:58
So I would use this?: `$this->pageVal = preg_replace('/[^A-Za-z]/', '', filter_input(INPUT_GET, 'page')) ? preg_replace('/[^A-Za-z]/', '', filter_input(INPUT_GET, 'page')) : "index";` Sorry this is horrific to read here - maybe paste it in a textfile to read it? I combined it with the ternary oparation. — , Apr 26 '18 at 12:07
If you are asking about security, it's a clear sign you should keep out of this topic. Don't reinvent the wheel, there are lots of [routers out there](https://packagist.org/?q=router) already. — Mike Doe, Apr 26 '18 at 12:48
Thank you for your rating of my question and constructive feedback. — , Apr 27 '18 at 09:38

score 0 · Accepted Answer · answered Apr 26 '18 at 12:40

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.

Thank you! This is very helpful for me. Digging a bit deeper in this topic I read about "basename()" and "realpath()". What about combining them to `$this->pageVal = basename(realpath($_GET['page']));` to strip directory information and get the full path? — , Apr 26 '18 at 14:54
No - because if you let the user specify the directory then you've got directory traversal again & lose your whitelist. — symcbean, Apr 26 '18 at 15:26
Thank you for your help. I will have to go deeper into this topic. Your answer shows me that I am maybe a bit far away from an acceptable solution. — , Apr 27 '18 at 09:34

Securely instantiate PHP class from URL get parameter?

1 Answers1