diff --git a/ChangeLog b/ChangeLog index 82771d980ab..0e57d0be48a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -21,6 +21,7 @@ Following changes may create regressions for some external modules, but were nec * The method get_substitutionarray_shipment_lines() has been removed. Use the generic get_substitutionarray_lines() instead. * Recheck setup of your module workflow to see if you need to enable the new setting to have shipment set to billed automatically when an invoice from a shipment is validated (and if your process is to make invoice on shipment and not on order). +* It was possible to use a variable $soc or $right inside a php code condition of some extrafields properties, this is no more true (this vars are no more defined globaly). ***** ChangeLog for 18.0.1 compared to 18.0.0 ***** diff --git a/htdocs/core/lib/functions.lib.php b/htdocs/core/lib/functions.lib.php index 6d5073aae58..2ae73a96a64 100644 --- a/htdocs/core/lib/functions.lib.php +++ b/htdocs/core/lib/functions.lib.php @@ -9145,6 +9145,33 @@ function dol_getIdFromCode($db, $key, $tablename, $fieldkey = 'code', $fieldid = } } +/** + * Check if a variable with name $var start with $text. + * Can be used to forge dol_eval() conditions. + * + * @param $var string Variable + * @param $regextext string Text that must be a valid regex string + * @param $matchrule int 1=Test if start with, 0=Test if equal + * @return boolean|string True or False, text if bad use. + */ +function isStringVarMatching($var, $regextext, $matchrule = 1) +{ + if ($matchrule == 1) { + if ($var == 'mainmenu') { + global $mainmenu; + return (preg_match('/^'.$regextext.'/', $mainmenu)); + } elseif ($var == 'leftmenu') { + global $leftmenu; + return (preg_match('/^'.$regextext.'/', $leftmenu)); + } else { + return 'This variable is not accessible with dol_eval'; + } + } else { + return 'This value for matchrule is not implemented'; + } +} + + /** * Verify if condition in string is ok or not * @@ -9153,15 +9180,15 @@ function dol_getIdFromCode($db, $key, $tablename, $fieldkey = 'code', $fieldid = */ function verifCond($strToEvaluate) { - global $user, $conf, $langs; + global $conf; // Read of const is done with getDolGlobalString() but we need $conf->currency for example + global $user, $langs; global $leftmenu; - global $rights; // To export to dol_eval function //print $strToEvaluate."
\n"; $rights = true; if (isset($strToEvaluate) && $strToEvaluate !== '') { //var_dump($strToEvaluate); - $rep = dol_eval($strToEvaluate, 1, 1, '1'); // The dol_eval must contains all the global $xxx for all variables $xxx found into the string condition + $rep = dol_eval($strToEvaluate, 1, 1, '1'); // The dol_eval() must contains all the "global $xxx;" for all variables $xxx found into the string condition $rights = $rep && (!is_string($rep) || strpos($rep, 'Bad string syntax to evaluate') === false); //var_dump($rights); } @@ -9175,32 +9202,35 @@ function verifCond($strToEvaluate) * @param string $s String to evaluate * @param int $returnvalue 0=No return (used to execute eval($a=something)). 1=Value of eval is returned (used to eval($something)). * @param int $hideerrors 1=Hide errors - * @param string $onlysimplestring '0' (deprecated, used for computed property of extrafields)=Accept all chars, '1' (most common use)=Accept only simple string with char 'a-z0-9\s^$_+-.*>&|=!?():"\',/@';', '2' (rarely used)=Accept also '[]' + * @param string $onlysimplestring '0' (deprecated, used for computed property of extrafields)=Accept all chars, + * '1' (most common use)=Accept only simple string with char 'a-z0-9\s^$_+-.*>&|=!?():"\',/@';', + * '2' (rarely used)=Accept also '[]' * @return mixed Nothing or return result of eval + * @see verifCond() */ function dol_eval($s, $returnvalue = 0, $hideerrors = 1, $onlysimplestring = '1') { - // Only global variables can be changed by eval function and returned to caller - global $db, $langs, $user, $conf, $website, $websitepage; + // Only this global variables can be read by eval function and returned to caller + global $conf; // Read of const is done with getDolGlobalString() but we need $conf->currency for example + global $db, $langs, $user, $website, $websitepage; global $action, $mainmenu, $leftmenu; global $mysoc; - global $objectoffield; + global $objectoffield; // To allow the use of $objectoffield in computed fields // Old variables used - global $rights; global $object; - global $obj; // To get $obj used into list when dol_eval is used for computed fields and $obj is not yet $object - global $soc; // For backward compatibility + global $obj; // To get $obj used into list when dol_eval() is used for computed fields and $obj is not yet $object + //global $rights; + //global $soc; // For backward compatibility if (!in_array($onlysimplestring, array('0', '1', '2'))) { - return 'Bad call of dol_eval. Parameter onlysimplestring must be 0, 1 or 2'; + return "Bad call of dol_eval. Parameter onlysimplestring must be '0' (deprecated), '1' or '2'"; } try { // Test on dangerous char (used for RCE), we allow only characters to make PHP variable testing if ($onlysimplestring == '1') { - // We must accept: '1 && getDolGlobalInt("doesnotexist1") && $conf->global->MAIN_FEATURES_LEVEL' - // We must accept: 'isModEnabled("barcode") || preg_match(\'/^AAA/\',$leftmenu)' + // We must accept: '1 && getDolGlobalInt("doesnotexist1") && getDolGlobalString("MAIN_FEATURES_LEVEL")' // We must accept: '$user->hasRight("cabinetmed", "read") && !$object->canvas=="patient@cabinetmed"' if (preg_match('/[^a-z0-9\s'.preg_quote('^$_+-.*>&|=!?():"\',/@', '/').']/i', $s)) { if ($returnvalue) { @@ -9209,10 +9239,21 @@ function dol_eval($s, $returnvalue = 0, $hideerrors = 1, $onlysimplestring = '1' dol_syslog('Bad string syntax to evaluate (found chars that are not chars for simplestring): '.$s); return ''; } - // TODO - // We can exclude all parenthesis ( that are not '($db' and 'getDolGlobalInt(' and 'getDolGlobalString(' and 'preg_match(' and 'isModEnabled(' - // ... } + $scheck = preg_replace('/->[a-zA-Z0-9_]+\(/', '->__METHOD__', $s); + $scheck = preg_replace('/\s[a-zA-Z0-9_]+\(/', ' __FUNCTION__', $scheck); + $scheck = preg_replace('/(\^|\')\(/', '__REGEXSTART__', $scheck); // To allow preg_match('/^(aaa|bbb)/'... or isStringVarMatching('leftmenu', '(aaa|bbb)') + //print 'scheck='.$scheck." : ".strpos($scheck, '(')."\n"; + if (strpos($scheck, '(') !== false) { + if ($returnvalue) { + return 'Bad string syntax to evaluate (found call of a function or method without using direct name): '.$s; + } else { + dol_syslog('Bad string syntax to evaluate (found call of a function or method without using direct name): '.$s); + return ''; + } + } + // TODO + // We can exclude $ char that are not: $db, $langs, $leftmenu, $topmenu, $user, $langs, $objectoffield, $object..., } elseif ($onlysimplestring == '2') { // We must accept: (($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" if (preg_match('/[^a-z0-9\s'.preg_quote('^$_+-.*>&|=!?():"\',/@[]', '/').']/i', $s)) { @@ -9222,10 +9263,17 @@ function dol_eval($s, $returnvalue = 0, $hideerrors = 1, $onlysimplestring = '1' dol_syslog('Bad string syntax to evaluate (found chars that are not chars for simplestring): '.$s); return ''; } - // TODO - // We can exclude all parenthesis ( that are not '($db' and 'getDolGlobalInt(' and 'getDolGlobalString(' and 'preg_match(' and 'isModEnabled(' and 'hasRight(' - // ... } + if (strpos($scheck, '(') !== false) { + if ($returnvalue) { + return 'Bad string syntax to evaluate (found call of a function or method without using direct name): '.$s; + } else { + dol_syslog('Bad string syntax to evaluate (found call of a function or method without using direct name): '.$s); + return ''; + } + } + // TODO + // We can exclude $ char that are not: $db, $leftmenu, $topmenu, $user, $langs, $object..., } if (is_array($s) || $s === 'Array') { return 'Bad string syntax to evaluate (value is Array) '.var_export($s, true); diff --git a/htdocs/core/lib/security.lib.php b/htdocs/core/lib/security.lib.php index 7818b9d2e7e..0a2d2572889 100644 --- a/htdocs/core/lib/security.lib.php +++ b/htdocs/core/lib/security.lib.php @@ -434,19 +434,21 @@ function restrictedArea(User $user, $features, $object = 0, $tableandshare = '', // Get more permissions checks from hooks $parameters = array('features'=>$features, 'originalfeatures'=>$originalfeatures, 'objectid'=>$objectid, 'dbt_select'=>$dbt_select, 'idtype'=>$dbt_select, 'isdraft'=>$isdraft); - $reshook = $hookmanager->executeHooks('restrictedArea', $parameters); + if (!empty($hookmanager)) { + $reshook = $hookmanager->executeHooks('restrictedArea', $parameters); - if (isset($hookmanager->resArray['result'])) { - if ($hookmanager->resArray['result'] == 0) { - if ($mode) { - return 0; - } else { - accessforbidden(); // Module returns 0, so access forbidden + if (isset($hookmanager->resArray['result'])) { + if ($hookmanager->resArray['result'] == 0) { + if ($mode) { + return 0; + } else { + accessforbidden(); // Module returns 0, so access forbidden + } } } - } - if ($reshook > 0) { // No other test done. - return 1; + if ($reshook > 0) { // No other test done. + return 1; + } } // Features/modules to check diff --git a/htdocs/core/modules/modApi.class.php b/htdocs/core/modules/modApi.class.php index acceaa822d1..9621904ebe6 100644 --- a/htdocs/core/modules/modApi.class.php +++ b/htdocs/core/modules/modApi.class.php @@ -163,7 +163,6 @@ class modApi extends DolibarrModules 'langs'=>'modulebuilder', 'position'=>100, 'perms'=>'1', - //'enabled'=>'isModEnabled("api") && preg_match(\'/^(devtools)/\',$leftmenu)', 'enabled'=>'isModEnabled("api")', 'target'=>'_apiexplorer', 'user'=>0); diff --git a/test/phpunit/SecurityTest.php b/test/phpunit/SecurityTest.php index 545306f5734..cf9e223d737 100644 --- a/test/phpunit/SecurityTest.php +++ b/test/phpunit/SecurityTest.php @@ -960,7 +960,7 @@ class SecurityTest extends PHPUnit\Framework\TestCase print "result = ".$result."\n"; $this->assertEquals('Parent project not found', $result); - $result=dol_eval('$a=function() { }; $a;', 1, 1, ''); + $result=dol_eval('$a=function() { }; $a;', 1, 1, '0'); print "result = ".$result."\n"; $this->assertContains('Bad string syntax to evaluate', $result); @@ -999,12 +999,22 @@ class SecurityTest extends PHPUnit\Framework\TestCase print "result = ".$result."\n"; $this->assertTrue($result); - // Same with syntax error + // Same with a value that does not match $leftmenu = 'XXX'; $result=dol_eval('$conf->currency && preg_match(\'/^(AAA|BBB)/\',$leftmenu)', 1, 1, '1'); print "result = ".$result."\n"; $this->assertFalse($result); + $leftmenu = 'AAA'; + $result=dol_eval('$conf->currency && isStringVarMatching(\'leftmenu\', \'(AAA|BBB)\')', 1, 1, '1'); + print "result = ".$result."\n"; + $this->assertTrue($result); + + $leftmenu = 'XXX'; + $result=dol_eval('$conf->currency && isStringVarMatching(\'leftmenu\', \'(AAA|BBB)\')', 1, 1, '1'); + print "result = ".$result."\n"; + $this->assertFalse($result); + // Case with param onlysimplestring = 1 @@ -1015,6 +1025,10 @@ class SecurityTest extends PHPUnit\Framework\TestCase $result=dol_eval("(\$a.'aa')", 1, 0); print "result = ".$result."\n"; $this->assertContains('Bad string syntax to evaluate', $result); + + $result=dol_eval('$a="abs" && $a(5)', 1, 0); + print "result = a".$result."\n"; + $this->assertContains('Bad string syntax to evaluate', $result); }