diff --git a/htdocs/core/lib/functions.lib.php b/htdocs/core/lib/functions.lib.php index fcf8c8407bf..0fd073885de 100644 --- a/htdocs/core/lib/functions.lib.php +++ b/htdocs/core/lib/functions.lib.php @@ -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) { diff --git a/htdocs/core/lib/website2.lib.php b/htdocs/core/lib/website2.lib.php index 0323e67265a..c82484fc056 100644 --- a/htdocs/core/lib/website2.lib.php +++ b/htdocs/core/lib/website2.lib.php @@ -720,10 +720,19 @@ function checkPHPCode(&$phpfullcodestringold, &$phpfullcodestring) } } + $phpfullcodestringnew = $phpfullcodestring; + // Then check forbidden commands if (!$error) { - $forbiddenphpstrings = array('$$', '$_', '}['); - //$forbiddenphpstrings = array_merge($forbiddenphpstrings, array('_ENV', '_SESSION', '_COOKIE', '_GET', '_POST', '_REQUEST', 'ReflectionFunction')); + 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('$$', '$_', '}['); + } 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; diff --git a/test/phpunit/SecurityTest.php b/test/phpunit/SecurityTest.php index 45f962a3b47..743b6d33c8e 100644 --- a/test/phpunit/SecurityTest.php +++ b/test/phpunit/SecurityTest.php @@ -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"; diff --git a/test/phpunit/WebsiteTest.php b/test/phpunit/WebsiteTest.php index bbf80cad4c5..0bdfdb5ca44 100644 --- a/test/phpunit/WebsiteTest.php +++ b/test/phpunit/WebsiteTest.php @@ -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 = ''; + $result = checkPHPCode($t, $s); + print __METHOD__." result checkPHPCode=".$result."\n"; + $this->assertEquals($result, 0, 'checkPHPCode detect string as dangerous when it is legitimate'); + // Dangerous