Wavyzz/dolibarr-fork
2
0
Fork 0
forked from Wavyzz/dolibarr
Code Pull Requests Activity

Fix security test blocking $_SESSION...

Browse Source
This commit is contained in:
Laurent Destailleur (aka Eldy)
2024-12-23 14:07:08 +01:00
parent fcc344f9da
commit 8733e9d57e
4 changed files with 52 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files

18
htdocs/core/lib/functions.lib.php
View File

@@ -10524,7 +10524,7 @@ function verifCond($strToEvaluate, $onlysimplestring = '1')
* @param int<0,1> $hideerrors 1=Hide errors
* @param string $onlysimplestring '0' (deprecated, do not use it anymore)=Accept all chars,
* '1' (most common use)=Accept only simple string with char 'a-z0-9\s^$_+-.*>&|=!?():"\',/@';',
* '2' (used for example for the compute property of extrafields)=Accept also '[]'
* '2' (used for example for the compute property of extrafields)=Accept also '<[]'
* @return void|string Nothing or return result of eval (even if type can be int, it is safer to assume string and find all potential typing issues as abs(dol_eval(...)).
* @see verifCond(), checkPHPCode() to see sanitizing rules that should be very close.
* @phan-suppress PhanPluginUnsafeEval
@@ -10552,12 +10552,12 @@ function dol_eval($s, $returnvalue = 1, $hideerrors = 1, $onlysimplestring = '1'
if ($onlysimplestring == '1' || $onlysimplestring == '2') {
// We must accept with 1: '1 && getDolGlobalInt("doesnotexist1") && getDolGlobalString("MAIN_FEATURES_LEVEL")'
// We must accept with 1: '$user->hasRight("cabinetmed", "read") && !$object->canvas=="patient@cabinetmed"'
// We must accept with 2: (($reloadedobj = new Task($db)) && ($reloadedobj->fetchNoCompute($object->id) > 0) && ($secondloadedobj = new Project($db)) && ($secondloadedobj->fetchNoCompute($reloadedobj->fk_project) > 0)) ? $secondloadedobj->ref : "Parent project not found"
// We must accept with 2: (($reloadedobj = new Task($db)) && ($reloadedobj->fetchNoCompute($object->id) <= 99) && ($secondloadedobj = new Project($db)) && ($secondloadedobj->fetchNoCompute($reloadedobj->fk_project) > 0)) ? $secondloadedobj->ref : "Parent project not found"
// Check if there is dynamic call (first we check chars are all into use a whitelist chars)
// Check if there is dynamic call (first we check chars are all into a whitelist chars)
$specialcharsallowed = '^$_+-.*>&|=!?():"\',/@';
if ($onlysimplestring == '2') {
$specialcharsallowed .= '[]';
$specialcharsallowed .= '<[]';
}
if (getDolGlobalString('MAIN_ALLOW_UNSECURED_SPECIAL_CHARS_IN_DOL_EVAL')) {
$specialcharsallowed .= getDolGlobalString('MAIN_ALLOW_UNSECURED_SPECIAL_CHARS_IN_DOL_EVAL');
@@ -10571,6 +10571,16 @@ function dol_eval($s, $returnvalue = 1, $hideerrors = 1, $onlysimplestring = '1'
}
}
// Check if there is a < or <= without spaces before/after
if (preg_match('/<=?[^\s]/', $s)) {
if ($returnvalue) {
return 'Bad string syntax to evaluate (mode '.$onlysimplestring.', found a < or <= without space before and after): '.$s;
} else {
dol_syslog('Bad string syntax to evaluate (mode '.$onlysimplestring.', found a < or <= without space before and after): '.$s, LOG_WARNING);
return '';
}
}
// Check if there is dynamic call (first we use black list patterns)
if (preg_match('/\$[\w]*\s*\(/', $s)) {
if ($returnvalue) {

15
htdocs/core/lib/website2.lib.php
View File

@@ -720,10 +720,19 @@ function checkPHPCode(&$phpfullcodestringold, &$phpfullcodestring)
}
}
$phpfullcodestringnew = $phpfullcodestring;
// Then check forbidden commands
if (!$error) {
if (getDolGlobalString("WEBSITE_DISALLOW_DOLLAR_UNDERSCORE")) {
$phpfullcodestring = preg_replace('/\$_COOKIE\[/', '__DOLLARCOOKIE__', $phpfullcodestring);
$phpfullcodestring = preg_replace('/\$_FILES\[/', '__DOLLARFILES__', $phpfullcodestring);
$phpfullcodestring = preg_replace('/\$_SESSION\[/', '__DOLLARSESSION__', $phpfullcodestring);
$forbiddenphpstrings = array('$$', '$_', '}[');
//$forbiddenphpstrings = array_merge($forbiddenphpstrings, array('_ENV', '_SESSION', '_COOKIE', '_GET', '_POST', '_REQUEST', 'ReflectionFunction'));
} else {
$forbiddenphpstrings = array('$$', '}[');
}
//$forbiddenphpstrings = array_merge($forbiddenphpstrings, array('_ENV', '_FILES', '_SESSION', '_COOKIE', '_GET', '_POST', '_REQUEST', 'ReflectionFunction'));
$forbiddenphpstrings = array_merge($forbiddenphpstrings, array('_ENV', 'ReflectionFunction'));
$forbiddenphpfunctions = array();
@@ -818,8 +827,8 @@ function checkPHPCode(&$phpfullcodestringold, &$phpfullcodestring)
// No need to block $conf->global->aaa() because PHP try to run the method aaa of $conf->global and not the function into $conf->global->aaa.
// Then check if installmodules does not block dynamic PHP code change.
if ($phpfullcodestringold != $phpfullcodestring) {
// Then check if installmodules.lock does not block dynamic PHP code change.
if ($phpfullcodestringold != $phpfullcodestringnew) {
if (!$error) {
$dolibarrdataroot = preg_replace('/([\\/]+)$/i', '', DOL_DATA_ROOT);
$allowimportsite = true;

23
test/phpunit/SecurityTest.php
View File

@@ -623,7 +623,13 @@ class SecurityTest extends CommonClassTest
$s = '(($reloadedobj = new Task($db)) && ($reloadedobj->fetchNoCompute($object->id) > 0) && ($secondloadedobj = new Project($db)) && ($secondloadedobj->fetchNoCompute($reloadedobj->fk_project) > 0)) ? $secondloadedobj->ref : \'Parent project not found\'';
$result = (string) dol_eval($s, 1, 1, '2');
print "result4 = ".$result."\n";
$this->assertEquals('Parent project not found', $result);
$this->assertEquals('Parent project not found', $result, 'Test 4');
$s = '4 < 5';
$result = (string) dol_eval($s, 1, 1, '2');
print "result5 = ".$result."\n";
$this->assertEquals('1', $result, 'Test 5');
/* not allowed. Not a one line eval string
$result = (string) dol_eval('if ($a == 1) { }', 1, 1);
@@ -633,16 +639,25 @@ class SecurityTest extends CommonClassTest
// Now string not allowed
$s = '4 <5';
$result = (string) dol_eval($s, 1, 1, '2'); // in mode 2, char < is allowed only if followed by a space
print "result = ".$result."\n";
$this->assertStringContainsString('Bad string syntax to evaluate', $result, 'Test 4 <5 - The string was not detected as evil');
$s = '4 < 5';
$result = (string) dol_eval($s, 1, 1, '1'); // in mode 1, char < is always forbidden
print "result = ".$result."\n";
$this->assertStringContainsString('Bad string syntax to evaluate', $result, 'Test 4 < 5 - The string was not detected as evil');
$s = 'new abc->invoke(\'whoami\')';
$result = (string) dol_eval($s, 1, 1, '2');
print "result = ".$result."\n";
$this->assertEquals('Bad string syntax to evaluate: new abc__forbiddenstring__(\'whoami\')', $result, 'The string was not detected as evil');
$this->assertStringContainsString('Bad string syntax to evaluate', $result, 'The string was not detected as evil');
$s = 'new ReflectionFunction(\'abc\')';
$result = (string) dol_eval($s, 1, 1, '2');
print "result = ".$result."\n";
$this->assertEquals('Bad string syntax to evaluate: new __forbiddenstring__(\'abc\')', $result, 'The string was not detected as evil');
$this->assertStringContainsString('Bad string syntax to evaluate', $result, 'The string was not detected as evil');
$result = dol_eval('$a=function() { }; $a', 1, 1, '0'); // result of dol_eval may be an object Closure
print "result5 = ".json_encode($result)."\n";

6
test/phpunit/WebsiteTest.php
View File

@@ -145,6 +145,12 @@ class WebsiteTest extends CommonClassTest
print __METHOD__." result checkPHPCode=".$result."\n";
$this->assertEquals($result, 0, 'checkPHPCode detect string as dangerous when it is legitimate');
$t = '';
$s = '<?php echo $_SESSION["eee"] ?>';
$result = checkPHPCode($t, $s);
print __METHOD__." result checkPHPCode=".$result."\n";
$this->assertEquals($result, 0, 'checkPHPCode detect string as dangerous when it is legitimate');
// Dangerous