Sanitizing include($_GET['filename'])

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
User avatar
Nathaniel
Forum Contributor
Posts: 396
Joined: Wed Aug 31, 2005 5:58 pm
Location: Arkansas, USA

Sanitizing include($_GET['filename'])

Post by Nathaniel »

Hey guys,

As Maugrim pointed out in this thread, my Suite Tester has no sanitation in the include statement. But I'm not sure what to do about it.

Checking it against a list of OK files isn't ideal, because that list of files is stored as a JavaScript array.

I suppose I could splatter warnings everywhere about password-protecting the directory, but that's less than ideal too. I know many people won't. People like me. :D

Does anyone else have any ideas?

My code as it stands:

Code: Select all

		function SuiteTester_TestRunner($test_file)//$test_file is raw $_GET input
		{
			
			$this->TestSuite();
			
			$existing_classes = get_declared_classes();
			
			include($test_file);
			
			$classes = $this->_selectRunnableTests($existing_classes, get_declared_classes());
			
			if ( $this->noRunnableTests($classes) )
			{
			    $this->addTestCase( 
			    	new SuiteTester_BadTestSuite($test_file, 
    	    	    	    	"Something is wrong with " . $test_file . ".  It may be MISSING, or simply have no runnable test cases.")
			    );
			}
			else
			{
				$this->addTestCase(
					$this->_createGroupFromClasses($test_file, $classes)
				);
			}
		}
Where do guys suggest I head with this?
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

Unless you want to come up with some form of authentication scheme for each script to load, I would just cover the basics like verifying the file exists, is readable.. the only thing I may add is that the file contains classes that descend from a unit testing root. .. which it appears you're doing in some fashion.
User avatar
Nathaniel
Forum Contributor
Posts: 396
Joined: Wed Aug 31, 2005 5:58 pm
Location: Arkansas, USA

Post by Nathaniel »

Ok. Hunky dory.

Thanks feyd.
programmingjeff
Forum Commoner
Posts: 26
Joined: Fri Jan 05, 2007 10:56 am

Post by programmingjeff »

When I include files that are selected by the user ($_GET), I usually get VERY strict on the filenames. I make sure that all my filenames are only lower-case alpha characters, all of which end in .php. That way, I can easily check if the $_GET input is correct without worrying about what to filter out. If the input is correct and the file exists (all of these files usually exist in an isolated directory), then I know it's safe to include the file.
User avatar
Weirdan
Moderator
Posts: 5978
Joined: Mon Nov 03, 2003 6:13 pm
Location: Odessa, Ukraine

Post by Weirdan »

I'd check that the beginning of realpath($test_file) matches to some predefined value:

Code: Select all

$allowed_path = dirname(realpath(__FILE__)) . DIRECTORY_SEPARATOR . 'tests' . DIRECTORY_SEPARATOR;
if (substr(realpath($file), 0, strlen($allowed_path)) == $allowed_path) {
  include($file);
}
At least you would be sure you're protected from $_GET['file'] being equal to '../../../../../../etc/passwd' =)
timvw
DevNet Master
Posts: 4897
Joined: Mon Jan 19, 2004 11:11 pm
Location: Leuven, Belgium

Post by timvw »

I'd go with Weirdan's approach.. But include realpath($file)... Or make sure that $file is a full/real path.. (Avoding include_patch tricks ;))
Post Reply