Question

I am currently working on a website and I want to use PHP for dynamic site loading.

Right now I came up with this...

if (preg_match('/^[a-z0-9]+$/', isset($_GET['site']))) {
    $general      = realpath('includes/general/' . $_GET['site'] . '.php');
    $laboratories = realpath('includes/laboratories/' . $_GET['site'] . '.php');
    $validation   = realpath('includes/validation/' . $_GET['site'] . '.php');
    $training     = realpath('includes/training/' . $_GET['site'] . '.php');
    $reagents     = realpath('includes/reagents/' . $_GET['site'] . '.php');

    if ($general) {
        include $general;
    } else if ($laboratories) {
        include $laboratories;
    } else if ($validation) {
        include $validation;
    } else if ($training) {
        include $training;
    } else if ($reagents) {
        include $reagents;
    } else {
        $default = realpath('includes/general/home.php');
        include $default;
    }
} else {
    $default = realpath('includes/general/home.php');
    include $default;
}

What do you think about this? Is it safe?

Was it helpful?

Solution

As for each value that $_GET['site'] can carry there is only one file to include, you can store that mapping in a whitelist:

$include_path    = '/path/to/includes';
$include_default = '/general/home.php';
$includes        = [
    'home' => 'general/home.php',
    // ...    
    'lab-zero' => 'laboratories/lab-zero.php',
    // ...
];

$include = $include_default;

if (isset($includes[$_GET['site']])) {
    $include = $includes[$_GET['site']];
}

$path = $include_path . $include;
$real = realpath($path);

if ($real === $path && is_readable($real)) {
    include $real;
}

Only those $_GET['site'] parameters are then allowed, that do make sense (those which have been made plans for, those that have been configured in $includes array).

Additionally if the whitelist contains an error, that file is not included (realpath and is_readable checks).

As you can also see, the value of $_GET['site'] has been fully shielded from the include path parameter. This is only possible with a whitelist which makes this approach pretty stable.

Most importantly, this effectively protectes against traversal attacks which your code has been prone for by having a flaw in the checks and by white-listing against the file-system which is dangerous.

OTHER TIPS

I had to make some changes to get your code to work.

            <?php               
                $include_path    = 'includes/';
                $include_default = 'general/home.php';
                $includes        = [
                    'home' => 'general/home.php',                     
                    'laboratories' => 'laboratories/laboratories.php',
                    'validation' => 'validation/validation.php',
                    'training' => 'training/training.php',
                    'reagents' => 'reagents/reagents.php',
                ];

                $include = $include_default;

                if (isset($_GET['site'])) {             
                    $include = $includes[$_GET['site']];            
                }

                $path = $include_path . $include;
                $real = realpath($path);

                if (is_readable($real)) {               
                    include $real;
                }
            ?>

Is this still safe as

      if ($real === $path && is_readable($real)) { 

wouldn't work since it's comparing two different things.

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top