Avoiding using include($_GET['page'])

Question

Basicaly, I've been working on a website for months now, and i'm about to open it. Before the opening I'm going over potential security issues, and i've made some research over the web to find what are the cummon security issues in php, I knew already how to fix most of them, although I really have an issue with one of them : I want to avoit using include($_GET['page']), I've made some research but haven't found anything really handy to use, although is there any way I can just "secure" my code as it is?
Here is what I've coded to prevent security issues:

if (!isset($_GET['page'])) 
{
echo redirect_tempo(500, 'index.php?page=home');    
}
    elseif ($_GET['page']=="index") 
    {
    echo redirect_tempo(500, 'index.php?page=home');        
    }
        elseif (file_exists($_GET['page'].".php"))
        {
        require $_GET['page'].'.php';
        }
            else 
            {
            echo redirect_tempo(500, 'index.php?page=404');
            }

Note that redirect_temp() is basically just a header().

Is this enough, can I improve it, or do I just need to completely change it?

This is vulnerable to directory traversal attacks: http://en.wikipedia.org/wiki/Directory_traversal_attack - you should use a whitelist of allowable values. — ajshort, Mar 03 '13 at 08:25
IMHO, include() is fine to use. You just need to make sure you are not doing something that is `including` user submitted data without first making sure there is nothing wrong with it. Doing things like `include($_GET['page'])` are the dangerous things. — bretterer, Mar 03 '13 at 08:25
You must also add a .htaccess file to redirect everything to your index.php, otherwise people could just go directly to php files you might not want them to be able to access directly (include-only files). — Lie Ryan, Mar 03 '13 at 10:19
Well, this is actually taken care of by using a script called at the beginning of each files, although it is not very handy, I may as well use the .htaccess trick from now on, much more practical on the long term. Thank you for bringin this to my attention. — bastienbot, Mar 03 '13 at 10:28

score 3 · Accepted Answer · answered Mar 03 '13 at 08:30

This part is dangerous:

elseif (file_exists($_GET['page'].".php"))

I can give the path "../../../../etc/passwd" (I know, that's kind of old, but it could be anything) and it would read the file (given enough permissions).

A simple fix could be:

elseif (file_exists(basename($_GET['page']) . ".php"))

Don't forget to also apply the same thing for the actual require:

require basename($_GET['page']) . '.php';

You could also apply basename() further up, so that you don't have to apply the function twice

See also: basename

Alright, it makes sense, I never though of using basename() in this particular case. Thank you so much for your help :) — bastienbot, Mar 03 '13 at 08:50

Adam Plocher · Answer 2 · 2013-03-03T08:34:24.627

There's nothing wrong with doing your method. Just add some basic validation, so you know that a malicious user isn't going to try to access other paths.

I would recommend validating that slashes and dots aren't in the string (because of paths like ../ or / or \ in Windows). And maybe even hard-code a .PHP at the end to prevent other file types from being accessed.

if (strstr($_GET['page'], ".") || strstr($_GET['page'], "/") || strstr($_GET['page'], "\\")
{
    echo "error";
    exit;
}

include($_GET['page'] .".php");

score 0 · Answer 3 · answered Mar 03 '13 at 08:29

Going with my comment, I would do something like this.

switch($_GET['page']) {
    case 'index' :
        redirect_tempo(500, 'index.php?page=home');
    break;
    ........
    default :
        die('NO!');
    break;
}

However this is not the best way of going about this.

score 0 · Answer 4 · answered Mar 03 '13 at 08:32

0

Your code looks good. But it would be better if you also validate your GET value before using. You can check whether it is numeric only or alphabets only or alphanumeric. It may be single word or multiple words. Check it as per your requirement.

answered Mar 03 '13 at 08:32

Nilambar Sharma

1,732
6
18
23

score 0 · Answer 5 · answered Mar 03 '13 at 08:43

This will include all the allowed pages, so $_GET['page'] == search will include search.php, but $_GET['page'] == something will include home.php :]

$allowed_pages = array('home', 'search', 'signup', 'signin', 'loggedin');
if(in_array($_GET["page"], $allowed_pages)) {
  $page = $_GET["page"]; 
} else { 
  $page = "home"; 
}
  $page = str_replace('/', '', $page);

if(file_exists("page_includes/$page.php")) {
  include("page_includes/$page.php");
} else {
  include("page_includes/home.php");
}

Lie Ryan · Answer 6 · 2013-03-03T09:39:42.487

I'd use a whitelist:

$all_pages = array(
    'index' => 'index.php',
    'about' => 'misc/about.php',
    '404' => '404.php',
    ...
);
function reverse($filename) {
    /* maps filenames to page names */
    return preg_replace('#/pages/(.*).php#', '$1', $filename);
}
/* add all php files in pages/ directory to $all_pages */
foreach (glob("pages/*.php") as $filename) {
    $all_pages[reverse($filename)] = $filename;
}

/* for debugging: make sure this only prints stuffs you want to include */
/* var_dump($all_pages); */

/* the actual check is simple */
$page_name = isset($_GET['page']) ? $_GET['page'] : "index";
$include_name = isset($all_pages[$page_name]) ? $all_pages[$page_name] : "404";
require $include_name;

score 0 · Answer 7 · answered Mar 22 '13 at 03:27

0

A lot of these answers are forgetting about null-byte attacks. Appending ".php" to prevent things like ../../../../../etc/passwd will not work on many deployments. This issue was only fixed properly in PHP 5.3.4.

answered Mar 22 '13 at 03:27

Mark Stanislav

979
4
11

Avoiding using include($_GET['page'])

7 Answers7

Linked