Random local file inclusion vulnerability question

[maxxd]

Hey y'all. So, I was reading a post here where @Jacques1 linked to an article about local file inclusion vulnerability, and it got me thinking. At the end, the article mentions converting characters to hexadecimal to get around the updir stop; this piqued my interest so I checked some work I'm doing right now for a client. I've got the following set-up:

 

This is the front-end controller of sorts....

//set up the custom post types we're going to create for this site
$cpts = array(
	'news_story',
	'team_member',
);

require_once('includes/Functions.php');
$fn = \Client\Functions::getInstance($cpts);

Now, in my Functions.php file, I've got my class that includes the following methods:

/**
 *	Constuctor method.
 *	Private - Singleton pattern
 *	@param		array		$cpts		Allowable custom post types for the system to attempt to create
 *	@return		void
 */
	private function __construct(array $cpts){
                $this->_cpts = $cpts;
		$this->createCPTs();
        }
/**
 *	Returns the singleton instance of this class.
 *	@param		array		Array of strings describing the necessary custom
 *							post types for the site.
 *	@return		\Client\Functions
 */
	public static function getInstance(array $cpts){
            if(empty(self::$_inst)){
                if(!is_array($cpts)){
                    $cpts = array();
                }
                self::$_inst = new self($cpts);
            }
            return self::$_inst;
        }

/**
 *	Create the site custom post type(s).
 *	@return		void
 */
	private function createCPTs(){
            foreach($this->_cpts as $i => $cpt){
		$fn = str_replace(array('.','/',' '), '', ucwords(str_replace('_', ' ', $cpt)));
		if(file_exists(dirname(__FILE__)."/cpts/{$fn}.php") && is_readable(dirname(__FILE__)."/cpts/{$fn}.php")){
			require_once(dirname(__FILE__)."/cpts/{$fn}.php");
			$fn = "\\Client\\{$fn}";
			$this->_cpts["client_{$cpt}"] = new $fn();
			unset($this->_cpts[$i]);
		}
	    }
        }

To my eye, this seems secure. By doing a string replace for both the '.' and the '/' character, I believe I'm stopping a local file inclusion vulnerability by basically voiding both directory traversing and specific file names - by removing the slash and the dot separately, it shouldn't match anything of interest on the server, right? I mean it's not like 'varwwwmysitehtaccess' is the same as '/var/www/mysite/.htaccess', './.htaccess', or '../../../.htaccess', right?

 

Any opinions? Just want a sanity check - it's been a long day...

[Jacques1]

What about backslashes? Or null byte injections?

 

Neither blacklisting nor trying to fix invalid input is a good approach. When you maintain a blacklist, there's a good chance you've missed one of the more obscure cases. And when you change the input, you may create the very vulnerability you're trying to prevent. For example, a seemingly valid mitigation would be to just remove all “/..“ substrings, but then “//....“ will in fact be turned into “/..“. I know that's not your approach, but it demonstrates the underlying problem.

 

You should validate against a whitelist, i. e. reject everything that you haven't explicitly marked as safe. In your case, probably the only valid characters are the alphanumeric ones, the underscore and maybe the hyphen. If the name doesn't match this pattern, there's something wrong, and the script should be aborted. An even safer approach would be to mantain an actual list of possible names, but that may be impractical.

[maxxd]

What about backslashes? Or null byte injections?

 

Neither blacklisting nor trying to fix invalid input is a good approach. When you maintain a blacklist, there's a good chance you've missed one of the more obscure cases. And when you change the input, you may create the very vulnerability you're trying to prevent. For example, a seemingly valid mitigation would be to just remove all “/..“ substrings, but then “//....“ will in fact be turned into “/..“. I know that's not your approach, but it demonstrates the underlying problem.

 

You should validate against a whitelist, i. e. reject everything that you haven't explicitly marked as safe. In your case, probably the only valid characters are the alphanumeric ones, the underscore and maybe the hyphen. If the name doesn't match this pattern, there's something wrong, and the script should be aborted. An even safer approach would be to mantain an actual list of possible names, but that may be impractical.

 

Thanks, Jacques1 - I was hoping you'd chime in on this. Unfortunately, as you say maintaining a list of possible names is a bit impractical, though I may look into it further as I near the end of the project and (hopefully) fewer surprises pop up.

 

As I understand it, the null byte injection weakness was fixed in 5.3 - at least for file_exists(), so wouldn't doing that check before including the file provide the necessary validation?

 

Either way, I very much like the idea of validating the content of the file name, and feel kinda silly for not thinking of it earlier. Although, honestly, regex is possibly my least favorite part of coding. But, time to buck up and figure it out.

 

Thanks for the input!

[Jacques1]

As I understand it, the null byte injection weakness was fixed in 5.3 - at least for file_exists(), so wouldn't doing that check before including the file provide the necessary validation?

 

I think you're drawing the wrong conclusions.

 

Null byte injections are a fundamental problem for all features implemented in C, which includes the PHP core. Sure, the PHP developers may have fixed some of the most important functions, but the problem may resurface at any time, be it in other core functions or in a PHP extension you're using.

 

It actually has happened again and still happens. If you search the PHP change log for “null byte injection”, you'll find an alarming number of vulnerabilities accross all PHP versions, including the latest ones.

 

So instead of relying on PHP to do the right thing, you should fix the problem and not pass any invalid paths to the lower layers of PHP.

 

 

 

Either way, I very much like the idea of validating the content of the file name, and feel kinda silly for not thinking of it earlier. Although, honestly, regex is possibly my least favorite part of coding. But, time to buck up and figure it out.

 

You don't need a regex. The ctype_alnum() function checks a string for alphanumeric characters (according to the current locale).

[maxxd]

I think you're drawing the wrong conclusions.

 

Null byte injections are a fundamental problem for all features implemented in C, which includes the PHP core. Sure, the PHP developers may have fixed some of the most important functions, but the problem may resurface at any time, be it in other core functions or in a PHP extension you're using.

 

It actually has happened again and still happens. If you search the PHP change log for “null byte injection”, you'll find an alarming number of vulnerabilities accross all PHP versions, including the latest ones.

 

So instead of relying on PHP to do the right thing, you should fix the problem and not pass any invalid paths to the lower layers of PHP.

 

Wow - thanks for the explanation!

 

You don't need a regex. The ctype_alnum() function checks a string for alphanumeric characters (according to the current locale).

 

I love learning new things - didn't even know that function existed!

 

Thanks - it's much appreciated.