CODEWITHSUNDEEP
phpsecurity
jadeallencook
jadeallencook

Reputation: 686

Security Flaws?

What are the repercussions of using this code?

<?php
if(file_exists("pages/" . $_GET["page"] . ".php")){
    include("pages/" . $_GET["page"] . ".php");
    } else{
        include("pages/home.php");
        }
?>

I've made it so that you cannot load anything without a ".php" extension, so I'm thinking it's pretty safe to use. If you use:

website.com/index.php?page=../index

In the url it will create an infinite loop. You can't load an external URL as far as I know.

Example:

website.com/index.php?page=anothersite.com/virus

But I'm not really sure, any suggestions? Or is this alright to use?

Upvotes: 4

Views: 484

Answers (5)

user2425057
user2425057

Reputation: 15

I will result into a serious vulnerability known as Local file inclusion, the GET parameters are not filtered better filter it as mentioned by jack or simply use php realpath() function which is a bit simple.

Upvotes: 0

Ja͢ck
Ja͢ck

Reputation: 173562

Using a ../../ pattern combined with a null byte can cause the path to "escape" its enclosure.

"../../../etc/passwd\0.php"

There are two things you can do about it:

  1. Use a white list of possible values you're willing to accept.

  2. Sanitize the path first, e.g.

    $path = preg_replace('/[^a-z]/', '');
// now use $path as you would

Upvotes: 2

Gumbo
Gumbo

Reputation: 655239

As already pointed out by zerkms, depending on the PHP version, file_exists and include may not be safe when handling NULL bytes. Only since PHP version 5.4.3, filesystem functions are said to be NULL byte safe.

So you should validate the value before using it, e.g., by using a white list of allowed values:

$allowedPages = array(/* … */);
if (in_array($_GET["page"], $allowedPages)) {
    // allowed
}

You can also expand this white list to any existing file below the document root directory:

if (strpos($_GET["page"], "\0") !== false) {
    // NULL byte detected
}
$path = realpath("pages/" . $_GET["page"] . ".php");
$base = realpath($_SERVER['DOCUMENT_ROOT']) . "/";
if ($path !== false && substr($path, 0, strlen($base)) === $base) {
    // allowed
}

However, this still may be used to bypass other access control measures like HTTP authorization based on the location.

Upvotes: 2

蒋艾伦
蒋艾伦

Reputation: 464

First of all, it's insecure to include page according to URL parameters, which canbe easily messed up by users, you'd better avoid that.

Second, if you want to avoid including external url, you may need to configure php.ini according to http://www.php.net/manual/en/filesystem.configuration.php#ini.allow-url-include

If you're not still into configure php.ini, your can trim the "http://", "https://" strings in $_GET['page'] at first, which is a good behavior, and make a .htaccess file to redirect unexist page request to home.php, you also have to make sure mod_rewrite is on your server.

RewriteEngine On
RewriteCond %{REQUEST_FILENAME} -s [OR]
RewriteCond %{REQUEST_FILENAME} -l [OR]
RewriteCond %{REQUEST_FILENAME} -d

RewriteRule ^.*$ - [NC,L]
RewriteRule ^.*$ home.php [NC,L]

Upvotes: 0

creack
creack

Reputation: 121492

Doing this, as you pointed out can result in an infinite loopy hat will cause php to crash.

Php crashing can result in a lot of bad things. Eg you can upload temporary files that won't be removed. This could ultimately lead to huge leaks and attacks.

Upvotes: 0

Related Questions