The solution to this type of problem is to not depend on end user server configurations for your security, but to get rid of junk like this:
require((substr($config, 0, 5) != "@FILE") ? $reflect_base."configs/$config.config.php" : $modx->config['base_path'].trim(substr($config, 5)));
which is unreadable, and hides from easy code scans the fact you are allowing, with NO tests, a possible file import execution.
There’s two problems with this code: first, you’re putting on one line what should be broken up into separate statements, and second, you’re not testing for the file’s existence, or for efforts to inject a url into this, and were assuming that a standalone file in your release wouldn’t be accessed by someone.
Avoid at all costs the bad habit of putting too much in one statement, construct the path, test the path, then once the path is tested, you can then, and only then, include it.
/*
Test the input data any time you're going to allow this type of get query set
path data. Do not blame end users for failing to do this properly.
*/
$tests = array( 'http', 'ftp', '://', '.php', '.inc', '.html', '.htm', '.pl', '.hta', '../', './', '%c0%af', '%255c', '..' );
// then strip out any possible url / external file data, adjust above list to suite your taste and needs
$reflect_base = str_replace( $tests, '', $reflect_base );
// then construct the full path, if it exists on server, fine, if not, die
$path = ( substr($config, 0, 5) != "@FILE") ? $reflect_base . "configs/$config.config.php" : $modx->config['base_path'] . trim( substr($config, 5) );
// then check all possible hacks
if ( !file_exists( $path ) || !$modx->config['base_path'] || !$config )
{
die ( 'The file does not appear to exist. Better luck next time. Have a nice day.' );
}
else
{
require( $path );
}
Please don’t blame the user’s server configurations on your failing to test this properly in the first place. That’s not how you get a solid, secure system, lock it down so you aren’t depending on the end user’s configurations to have secure code.
Generally I like how you all do this cms, I like the programming APIs, but this response was in my opinion not how you get secure code, get a fix out for what you control first, make sure you aren’t using methods that can lead to repeats of this type of error, then let the end users worry about their server settings, but that should have nothing to do with the inherent security of the code you’re releasing.
☆ A M B ☆
MODX Staff
MODX Staff
