Page 1 of 1

Dynamic include via URL -- security implications and fixes?

Posted: Mon May 25, 2009 4:33 am
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.

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

Posted: Mon May 25, 2009 10:01 am
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;
}

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

Posted: Mon May 25, 2009 3:11 pm
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

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

Posted: Mon May 25, 2009 3:32 pm
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.

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

Posted: Mon May 25, 2009 3:57 pm
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?

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

Posted: Mon May 25, 2009 4:11 pm
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.

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

Posted: Mon May 25, 2009 4:20 pm
by Griven
I'll give the whitelist a shot.

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