Page 1 of 1

Sanitizing include($_GET['filename'])

Posted: Fri May 04, 2007 8:13 am
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?

Posted: Fri May 04, 2007 8:27 am
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.

Posted: Fri May 04, 2007 9:24 am
by Nathaniel
Ok. Hunky dory.

Thanks feyd.

Posted: Fri May 04, 2007 12:56 pm
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.

Posted: Fri May 04, 2007 2:27 pm
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' =)

Posted: Fri May 04, 2007 2:42 pm
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 ;))