SitePoint Sponsor

User Tag List

Results 1 to 16 of 16
  1. #1
    SitePoint Enthusiast
    Join Date
    Dec 2010
    Location
    Canada, Alberta
    Posts
    50
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    possible injection when including files

    Hi, what are the possible security risks when including files based on post variables?

    here's part of my script

    Code:
    //PATH is the absolute path to the file... ex: /home/usr/public_html/site
    $file = trim( $_POST['load'] );
    $path = PATH . "/$file.php";
    
    if(file_exists(path )){
    include $path;
    }
    What security measures can I take to make this more secure? Should I just sanitize the variable with filter_var()?

  2. #2
    SitePoint Wizard silver trophybronze trophy Cups's Avatar
    Join Date
    Oct 2006
    Location
    France, deep rural.
    Posts
    6,869
    Mentioned
    17 Post(s)
    Tagged
    1 Thread(s)
    You would be trying to defeat anyone trying to attempt directory traversal, so anything a containing a suspicious pattern of dots and slashes should be nuked - unless you maintain a white-list of files which are allowed to be loaded.

    You could make this check simpler if you can append the ".php" yourself.

    Then you just the existence of a dot should be enough for you to fail early, and get rid of this user.
    PHP Code:
    $var './bad';
    if(
    strpos$var'.') !== false ) echo 'send away'
    Here is an example of the white-list filtering method.
    PHP Code:
    $permitted = array('about''home''departments');

    if( isset(
    $_POST['load']) && in_array($_POST['load'], $permitted) ) {

    include 
    PATH '/'$_POST['load'] . '.php';


    Last edited by Cups; Aug 26, 2011 at 01:14. Reason: added strpos example

  3. #3
    SitePoint Enthusiast
    Join Date
    Dec 2010
    Location
    Canada, Alberta
    Posts
    50
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Cups View Post
    You would be trying to defeat anyone trying to attempt directory traversal, so anything a containing a suspicious pattern of dots and slashes should be nuked - unless you maintain a white-list of files which are allowed to be loaded.

    You could make this check simpler if you can append the ".php" yourself.

    Then you just the existence of a dot should be enough for you to fail early, and get rid of this user.
    PHP Code:
    $var './bad';
    if(
    strpos$var'.') !== false ) echo 'send away'
    Here is an example of the white-list filtering method.
    PHP Code:
    $permitted = array('about''home''departments');

    if( isset(
    $_POST['load']) && in_array($_POST['load'], $permitted) ) {

    include 
    PATH '/'$_POST['load'] . '.php';


    That is awesome! I would probably do a preg_match on patterns like dots or slashes, thanks man!

  4. #4
    SitePoint Wizard silver trophybronze trophy Cups's Avatar
    Join Date
    Oct 2006
    Location
    France, deep rural.
    Posts
    6,869
    Mentioned
    17 Post(s)
    Tagged
    1 Thread(s)
    That is awesome! I would probably do a preg_match on patterns like dots or slashes, thanks man!
    No prob!

    Yeah, you got it - my pastebin app while nice, does not handle forward slashes.

    Someone else may have more to say on this. I must admit that including a file depending on a var coming from the internet gives me the willies, but I guess there is enough to keep you safe there.

    Anyone else?

  5. #5
    SitePoint Guru
    Join Date
    Nov 2003
    Location
    Huntsville AL
    Posts
    689
    Mentioned
    4 Post(s)
    Tagged
    0 Thread(s)
    You can go one step further and transform the posted value into a completely unrelated file name:
    PHP Code:
    $permitted = array('about''home''departments');
    // Could be
    $permitted = array('about' => 'AboutFileName.php''home' => 'HouseFileName.doc''departments' => 'ListOfDepartments.xml'); 

  6. #6
    SitePoint Enthusiast
    Join Date
    Dec 2010
    Location
    Canada, Alberta
    Posts
    50
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Creating a whitelist sounds much simpler but there are like 30+ files in different folders so I figured It would be easier to just look for unwanted patterns. Should I also check for url patterns like http?

  7. #7
    SitePoint Wizard Hammer65's Avatar
    Join Date
    Nov 2004
    Location
    Lincoln Nebraska
    Posts
    1,161
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by charlestonolaes View Post
    Creating a whitelist sounds much simpler but there are like 30+ files in different folders so I figured It would be easier to just look for unwanted patterns. Should I also check for url patterns like http?
    You wouldn't have to have a file whitelist necessarily. Depending on the directory structure you are using you could confirm that the file is in a specific directory. If it is then include it if not don't.
    Visit my blog
    PHP && Life
    for technology articles and musings.

  8. #8
    SitePoint Wizard silver trophybronze trophy Cups's Avatar
    Join Date
    Oct 2006
    Location
    France, deep rural.
    Posts
    6,869
    Mentioned
    17 Post(s)
    Tagged
    1 Thread(s)
    Yeah, maintaining a white-list does seem an overhead you may not need - but if you find yourself maintaining some kind of menu system - then maybe it could be an offshoot - especially if you bear AHundiaks idea in mind. eg
    PHP Code:
    $permitted = array(
    'Nice name' => 'real_file_name',
    'About us' => 'AboutFileName.php',
     ); 

  9. #9
    SitePoint Enthusiast
    Join Date
    Dec 2010
    Location
    Canada, Alberta
    Posts
    50
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    cool, thanks to everyone for their help. I've decided to check for any characters like dots or slashes and I also check if the file exists.

    PHP Code:

    //$view is the post variable

    if( !preg_match'~^[http]|([\.|\\|/])|[0-9]~'$view$match ) ){

    //some stuff
        
         //$path = FULL_PATH . $view . '.php';
        
    if( file_exists($path) ){
               include 
    $path;
        }



  10. #10
    Non-Member bronze trophy
    Join Date
    Nov 2009
    Location
    Keene, NH
    Posts
    3,760
    Mentioned
    23 Post(s)
    Tagged
    0 Thread(s)
    I just str_replace "/" -- no slashes, no directory changing -- no slashes, periods won't actually change directory either, it just breaks the path...

    Cups method of checking against an array is a REALLY good idea, and I would suggest that what you do is use "glob" to make a list of valid choices. Just put all your sub-files that are valid choices in a directory (keeping it 755 or even 644) and then glob in your list.

    Code:
    $dirList=glob('includes/*.php');
    $includeList=array();
    foreach ($dirList as $data) $includeList[]=pathinfo($data,PATHINFO_FILENAME);
    
    ...
    Then later on just:

    Code:
    if ($target=array_search($_POST['load'],$includeList) {
    	include('includes/',$includeList[$target],'.php');
    }
    
    ...
    This means NOT sending $_POST to the include, and instead the value pulled from the filesystem -- that way no comparison trickery can be pulled to trick you into sending a invalid $_POST to the include or to file_exists.

    Which is probably as secure as you're gonna get on doing that. It also means that by comparing to the proper list and using the proper list, you don't have to bother stripping out slashes or periods -- as they aren't in the set.

    Another approach that might have some merit is to set them as the key in an array, as then you don't need to search, just check isset.

    Code:
    $dirList=glob('includes/*.php');
    $includeList=array();
    foreach ($dirList as $data) $includeList[pathinfo($data,PATHINFO_FILENAME)]=true;
    
    if (isset($includeList[$_POST['load']])) {
    	include('includes/',$includeList[$_POST['load']],'.php');
    }
    Either way is a decent approach.

  11. #11
    SitePoint Enthusiast
    Join Date
    Dec 2010
    Location
    Canada, Alberta
    Posts
    50
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    oh cool, I never know glob() existed till now, thanks for that. I'll give that a go. thanks man

  12. #12
    SitePoint Enthusiast
    Join Date
    Sep 2009
    Posts
    53
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Just create a simple function using regex or substr() on each character to ensure only a-z0-9. are possible characters.

  13. #13
    Non-Member bronze trophy
    Join Date
    Nov 2009
    Location
    Keene, NH
    Posts
    3,760
    Mentioned
    23 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by the182guy View Post
    Just create a simple function using regex or substr() on each character to ensure only a-z0-9. are possible characters.
    Also a decent approach -- though not sure what good substr would be for that -- wouldn't string as array indexing be better/faster? or again, str_replace since if the undesired characters (period and slashes) are present, it's an invalid name automatically.

    $loadName=str_replace(array('.','/','\'),'',$_POST['load']);

  14. #14
    SitePoint Enthusiast
    Join Date
    Sep 2009
    Posts
    53
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by deathshadow60 View Post
    Also a decent approach -- though not sure what good substr would be for that -- wouldn't string as array indexing be better/faster? or again, str_replace since if the undesired characters (period and slashes) are present, it's an invalid name automatically.

    $loadName=str_replace(array('.','/','\'),'',$_POST['load']);
    If you didn't want to use regex, substr() can be used to examine each character in the input, to check if it's allowed or not.

    A character whitelist e.g a-z0-9 is better than replacing from a blacklist. You're letting special characters in like % which is used by hackers to prematurely terminate a string for LFI exploits.

  15. #15
    Non-Member bronze trophy
    Join Date
    Nov 2009
    Location
    Keene, NH
    Posts
    3,760
    Mentioned
    23 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by the182guy View Post
    If you didn't want to use regex, substr() can be used to examine each character in the input, to check if it's allowed or not.
    Which would be painfully slow...

    ... and again, a job for treating a string as an array instead of using substr.
    for example (swapping non-whitelist for underscore)
    Code:
    for ($t=0; $t<length($myString); $t++) {
      if (!in_array($myString[$t],$whitelistchars)) {
        $myString[$t]='_';
      }
    }
    Even so going through a string with user code that way would be PAINFULLY slow compared to even the regex.

    Quote Originally Posted by the182guy View Post
    A character whitelist e.g a-z0-9 is better than replacing from a blacklist. You're letting special characters in like % which is used by hackers to prematurely terminate a string for LFI exploits.
    If you do file_exists and a lfi exploit actually returns positive, there's something horrifically wrong with your directory structure.

    Though again, this is why I'd just check against the actual filenames using glob... and not allow the value from $_POST anywhere NEAR the require/include.

  16. #16
    SitePoint Enthusiast
    Join Date
    Sep 2009
    Posts
    53
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by deathshadow60 View Post
    Which would be painfully slow...

    ... and again, a job for treating a string as an array instead of using substr.
    for example (swapping non-whitelist for underscore)
    Code:
    for ($t=0; $t<length($myString); $t++) {
      if (!in_array($myString[$t],$whitelistchars)) {
        $myString[$t]='_';
      }
    }
    Even so going through a string with user code that way would be PAINFULLY slow compared to even the regex.


    If you do file_exists and a lfi exploit actually returns positive, there's something horrifically wrong with your directory structure.

    Though again, this is why I'd just check against the actual filenames using glob... and not allow the value from $_POST anywhere NEAR the require/include.
    Which is why regex is first choice. file_exists will always return true for a successful LFI attempt.

    Why use glob? An a-z regex plus file_exists is sufficient. This is how it is done with the Joomla! CMS. If there are scripts that aren't intended to be included, in the target directory then the only solution is a whitelist of includable files.


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •