Dynamic include via URL -- security implications and fixes?

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Post Reply
Griven
Forum Contributor
Posts: 165
Joined: Sat May 09, 2009 8:23 pm

Dynamic include via URL -- security implications and fixes?

Post by Griven »

Hey, all.

In my site, I'm using the following type of URL to include one page inside the main template:

Code: Select all

http://mysite/index.php?page=mypage
Where mypage is a PHP file outside of the web root.

My code to handle these requests is as follows:

Code: Select all

if (empty($_GET['page'])) {
          //Set a default page if it is not explicitly requested
    $page = 'home';
}
else {
          //Otherwise, assign it to the variable.
    $page = $_GET['page'];
}
 
    //Retrieve the specified page from the directory
$displaypage = @include('../system/pages/'. $page .'.php');
 
    //If the page does not exist, throw an error
if (!$displaypage) {
    @include('../system/pages/custerror/404.htm');
    exit;
}
What are the security implications (if any) and their fixes (again, if any) of this type of model?

Any insight is greatly appreciated.
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: Dynamic include via URL -- security implications and fixes?

Post by kaisellgren »

Yum, that script is delicious. 8)

You are allowing anyone to include any file on your entire file system. Does that sound good to you? It should not. You should white list the allowed choices of pages.

Code: Select all

$allowedPages = array('index','contact','downloads');
if (in_array($page,$allowedPages))
 include('../system/pages/'. $page .'.php');
else
{
 include('../system/pages/custerror/404.htm');
 exit;
}
User avatar
Darhazer
DevNet Resident
Posts: 1011
Joined: Thu May 14, 2009 3:00 pm
Location: HellCity, Bulgaria

Re: Dynamic include via URL -- security implications and fixes?

Post by Darhazer »

Additionally, if you want to include only pages from /system/pages/ folder you can do this, but always use the basename() function when dealing with file names:

Code: Select all

 
if (empty($_GET['page'])) {
          //Set a default page if it is not explicitly requested
    $page = 'home';
}
else {
          //Otherwise, assign it to the variable.
    $page = $_GET['page'];
}
 
    //Retrieve the specified page from the directory
   $path = dirname(__FILE__) . '/../system/pages/' . basename($page) . '.php';
   if (file_exists($path))
      include($path);
   else
     include('../system/pages/custerror/404.htm');
    exit;
}
 
Using @ in your code is ugly (and slower then checking), and include should not (although it can) return a value. The value is returned if there is a return statement in the included file, which in my opinion, is a feature that should be removed from PHP
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: Dynamic include via URL -- security implications and fixes?

Post by kaisellgren »

Darhazer wrote:

Code: Select all

$path = dirname(__FILE__) . '/../system/pages/' . basename($page) . '.php';
Is vulnerable to truncation attacks. Pass a null byte and you can include non .php files within the directory.
Griven
Forum Contributor
Posts: 165
Joined: Sat May 09, 2009 8:23 pm

Re: Dynamic include via URL -- security implications and fixes?

Post by Griven »

kaisellgren wrote:
Darhazer wrote:

Code: Select all

$path = dirname(__FILE__) . '/../system/pages/' . basename($page) . '.php';
Is vulnerable to truncation attacks. Pass a null byte and you can include non .php files within the directory.
How would you remedy that, then?
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: Dynamic include via URL -- security implications and fixes?

Post by kaisellgren »

Griven wrote:
kaisellgren wrote:
Darhazer wrote:

Code: Select all

$path = dirname(__FILE__) . '/../system/pages/' . basename($page) . '.php';
Is vulnerable to truncation attacks. Pass a null byte and you can include non .php files within the directory.
How would you remedy that, then?
Well, a simple trick would be to strip out null bytes:

Code: Select all

$page = str_replace(chr(0),'',$page);
Though it would be better if you utilize white listing and filter out non-accepted characters.
Griven
Forum Contributor
Posts: 165
Joined: Sat May 09, 2009 8:23 pm

Re: Dynamic include via URL -- security implications and fixes?

Post by Griven »

I'll give the whitelist a shot.

Thank you both--your help is very much appreciated.
Post Reply