CODEWITHSUNDEEP
phpsecurity
bastienbot
bastienbot

Reputation: 123

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

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?

Upvotes: 1

Views: 4292

Answers (7)

Mark Stanislav
Mark Stanislav

Reputation: 979

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.

Upvotes: 0

Lie Ryan
Lie Ryan

Reputation: 64845

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;

Upvotes: 0

Wiggler Jtag
Wiggler Jtag

Reputation: 669

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");
}

Upvotes: 0

Adam Plocher
Adam Plocher

Reputation: 14233

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");

Upvotes: 0

Nilambar Sharma
Nilambar Sharma

Reputation: 1740

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.

Upvotes: 0

Ja͢ck
Ja͢ck

Reputation: 173572

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

Upvotes: 3

bretterer
bretterer

Reputation: 5781

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.

Upvotes: 0

Related Questions