2
\$\begingroup\$

Currently I'm using this code:

<?php
/*
Clean, Simple, Minimal.
*/
$pages = array('home', 'login', 'register', 'logout', 'test');
if (isset($_GET['p']))
{
    $page = $_GET['p'];
    if (in_array($page, $pages))
    {
       include('pages/' . $page . '.php');
    }
} 
 else {
    echo 'Hi';
}
?>

I want this to be as secure and efficient as possible, I've seen multiple versions of this. I wasn't sure how to do what i really wanted, I wanted it so that if the requested file .php exists in /pages/ it'll show it, and else throw an error. But last time i did that I was vulnerable to LFI.

\$\endgroup\$
  • \$\begingroup\$ I like your current approach, it is clean, safe, and relatively fast. If you don't mind maintaining the whitelist array, it's the best it's going to get. It is faster than the approach of Kid, and depending on the size of the array also faster than the one of wesley. A whitelist is also as safe as its going to get (it is generally recommended over cleaning the input). \$\endgroup\$ – tim Sep 11 '14 at 13:18
  • 1
    \$\begingroup\$ You can't really write something that is both as secure as possible and as fast as possible. Security often requires you to sacrifice efficiency. If you throw flexibility and/or usability into the mix, you can only try to balance things as well as you can. Sometimes you find yourself writing less secure code, because you've weighed out the pro's and cons: if the security risks (and consequences) are minimal, but the usability and speed gains are sizable, sometimes you choose the less secure option. Bottom line: Premature optimization is the root of all evil \$\endgroup\$ – Elias Van Ootegem Sep 11 '14 at 13:59
1
\$\begingroup\$

Assuming nobody but you can place files in the directory, this will be safe because you are checking the input page against a pre-defined list of page files. Additionally, this code makes the white list dynamic and throws an exception if the page is not found.

As @tim said in the comment, he found this code to be about 5 times slower then yours. So that's good to keep in mind if you ever decide to use this.

Every .php page you put in the pages/ directory will automatically be callable.

if (isset($_GET['p'])) {
    $page  = $_GET['p'] . '.php';
    $pages = array_map('basename', glob('pages/*.php'));

    if (in_array($page, $pages)) {
        include('pages/' . $page);
    } else {
        throw new Exception('Page "' . $page . '" does not exist.');
    }
} else {
    echo 'Hi';
}
\$\endgroup\$
  • \$\begingroup\$ I do see the advantage of this approach (the whitelist array doesn't have to be maintained), but it doesn't seem all that efficient (when I profiled it, it was 5 times as slow as the code of @Will), it also doesn't offer improved security over the original code. \$\endgroup\$ – tim Sep 11 '14 at 13:08
  • \$\begingroup\$ With efficiency I meant to say that it's dynamic instead of hard-coded. I should rephrase it as I haven't tested it for efficiency. \$\endgroup\$ – Kid Diamond Sep 11 '14 at 13:19
0
\$\begingroup\$

Riding on @Kid Diamond's answer, I've shortened it as follows:

$page = (isset($_GET['p']) ? trim($_GET['p']) : "nexist") . ".php";

if(!in_array($page, array_map('basename', glob('pages/*.php')))){
    throw new Exception('Requested page does not exist.');
    # die('Requested page does not exist.');  # Or die here if this is not catchable
}

include('pages/' . $page);

Basically, we're doing the isset check inline, as well as concatenating the php extension after check. Though this relies on the file nexist not existing - you can replace with any other name. Additionally, we're not echoing $page on in_array fail - rather we simply inform the user that the requested page does not exist.

\$\endgroup\$
0
\$\begingroup\$

So as I said in the comments, I like your approach. A Whitelist is always safer than cleaning the user input.

But what if you have a lot of pages? Then your approach might become slow, as it runs in linear time. You might order the values in the array, so that often visited pages are located at the beginning, but this doesn't really solve the problem.

You could also use the approach of @wesley veldeman, which runs in constant time, but it cleans the user input instead of using a whitelist, which is less secure.

A better approach might be this:

if (!isset($_GET['p'])) {
    echo 'Please specify a page.';
    return;
}

$page = $_GET['p'];
$pages = array(
    'home'      => 'home', 
    'login'     => 'login', 
    'register'  => 'register', 
    'logout'    => 'logout', 
    'test'      => 'test'); 
if (isset($pages[$page])) {
    include('pages/' . $page . '.php');
} else {
    echo 'This page does not exist.';
}

It also runs in constant time, and it uses a whitelist approach. Of course, you still have to maintain the whitelist with this approach, but I don't really see this as a downside, as it gives you more control over it.

And as @Elias Van Ootegem said in the comments: If your current approach is not a bottle neck, you can just keep it, no need to optimize something that isn't too slow.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.