From ef184567248af5dbeea8cbd63be5cd2d965ad756 Mon Sep 17 00:00:00 2001 From: Laurent Destailleur Date: Mon, 9 May 2022 21:56:21 +0200 Subject: [PATCH] Fix regression in dol_eval --- htdocs/core/lib/functions.lib.php | 8 ++-- test/phpunit/SecurityTest.php | 62 +++++++++++++++++-------------- 2 files changed, 38 insertions(+), 32 deletions(-) diff --git a/htdocs/core/lib/functions.lib.php b/htdocs/core/lib/functions.lib.php index 22ff541f7d5..43a549655b9 100644 --- a/htdocs/core/lib/functions.lib.php +++ b/htdocs/core/lib/functions.lib.php @@ -8200,7 +8200,7 @@ 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=Accept all chars, 1=Accept only simple string with char 'a-z0-9\s$_->&|=';, 2=Accept also '!?():"\';,/' + * @param string $onlysimplestring 0=Accept all chars, 1=Accept only simple string with char 'a-z0-9\s^$_+-.*\/>&|=!?():"\',/';, 2=Accept also ';[]' * @return mixed Nothing or return result of eval */ function dol_eval($s, $returnvalue = 0, $hideerrors = 1, $onlysimplestring = '1') @@ -8218,7 +8218,7 @@ function dol_eval($s, $returnvalue = 0, $hideerrors = 1, $onlysimplestring = '1' // Test dangerous char (used for RCE), we allow only PHP variable testing. if ($onlysimplestring == '1') { //print preg_quote('$_->&|', '/'); - if (preg_match('/[^a-z0-9\s'.preg_quote('$_+-*/>&|=!?():"', '/').']/i', $s)) { + if (preg_match('/[^a-z0-9\s'.preg_quote('^$_+-.*/>&|=!?():"\',/', '/').']/i', $s)) { if ($returnvalue) { return 'Bad string syntax to evaluate (found chars that are not chars for simplestring): '.$s; } else { @@ -8228,7 +8228,7 @@ function dol_eval($s, $returnvalue = 0, $hideerrors = 1, $onlysimplestring = '1' } } elseif ($onlysimplestring == '2') { //print preg_quote('$_->&|', '/'); - if (preg_match('/[^a-z0-9\s'.preg_quote('^$_+-*/>&|=!?():"\';,/', '/').']/i', $s)) { + if (preg_match('/[^a-z0-9\s'.preg_quote('^$_+-.*/>&|=!?():"\',/;[]', '/').']/i', $s)) { if ($returnvalue) { return 'Bad string syntax to evaluate (found chars that are not chars for simplestring): '.$s; } else { @@ -8245,7 +8245,7 @@ function dol_eval($s, $returnvalue = 0, $hideerrors = 1, $onlysimplestring = '1' return ''; } } - if (strpos($s, '.') !== false) { + if (preg_match('/[^0-9]+\.[^0-9]+/', $s)) { // We refuse . if not between 2 numbers if ($returnvalue) { return 'Bad string syntax to evaluate (dot char is forbidden): '.$s; } else { diff --git a/test/phpunit/SecurityTest.php b/test/phpunit/SecurityTest.php index fe5cbd01462..b33ec06504a 100644 --- a/test/phpunit/SecurityTest.php +++ b/test/phpunit/SecurityTest.php @@ -584,34 +584,6 @@ class SecurityTest extends PHPUnit\Framework\TestCase return $result; } - /** - * testCheckLoginPassEntity - * - * @return void - */ - public function testCheckLoginPassEntity() - { - $login=checkLoginPassEntity('loginbidon', 'passwordbidon', 1, array('dolibarr')); - print __METHOD__." login=".$login."\n"; - $this->assertEquals($login, ''); - - $login=checkLoginPassEntity('admin', 'passwordbidon', 1, array('dolibarr')); - print __METHOD__." login=".$login."\n"; - $this->assertEquals($login, ''); - - $login=checkLoginPassEntity('admin', 'admin', 1, array('dolibarr')); // Should works because admin/admin exists - print __METHOD__." login=".$login."\n"; - $this->assertEquals($login, 'admin', 'The test to check if pass of user "admin" is "admin" has failed'); - - $login=checkLoginPassEntity('admin', 'admin', 1, array('http','dolibarr')); // Should work because of second authentication method - print __METHOD__." login=".$login."\n"; - $this->assertEquals($login, 'admin'); - - $login=checkLoginPassEntity('admin', 'admin', 1, array('forceuser')); - print __METHOD__." login=".$login."\n"; - $this->assertEquals($login, ''); // Expected '' because should failed because login 'auto' does not exists - } - /** * testEncodeDecode * @@ -919,6 +891,11 @@ class SecurityTest extends PHPUnit\Framework\TestCase print "result = ".$result."\n"; $this->assertContains('Bad string syntax to evaluate', $result); + $result=dol_eval("90402.38+267678+0", 1, 1, 1); + print "result = ".$result."\n"; + $this->assertEquals('358080.38', $result); + + global $leftmenu; $leftmenu = 'admintools'; $result=dol_eval('$conf->currency && preg_match(\'/^(admintools|all)/\',$leftmenu)', 1, 0, '2'); @@ -936,4 +913,33 @@ class SecurityTest extends PHPUnit\Framework\TestCase print "result = ".$result."\n"; $this->assertContains('Bad string syntax to evaluate', $result); } + + + /** + * testCheckLoginPassEntity + * + * @return void + */ + public function testCheckLoginPassEntity() + { + $login=checkLoginPassEntity('loginbidon', 'passwordbidon', 1, array('dolibarr')); + print __METHOD__." login=".$login."\n"; + $this->assertEquals($login, ''); + + $login=checkLoginPassEntity('admin', 'passwordbidon', 1, array('dolibarr')); + print __METHOD__." login=".$login."\n"; + $this->assertEquals($login, ''); + + $login=checkLoginPassEntity('admin', 'admin', 1, array('dolibarr')); // Should works because admin/admin exists + print __METHOD__." login=".$login."\n"; + $this->assertEquals($login, 'admin', 'The test to check if pass of user "admin" is "admin" has failed'); + + $login=checkLoginPassEntity('admin', 'admin', 1, array('http','dolibarr')); // Should work because of second authentication method + print __METHOD__." login=".$login."\n"; + $this->assertEquals($login, 'admin'); + + $login=checkLoginPassEntity('admin', 'admin', 1, array('forceuser')); + print __METHOD__." login=".$login."\n"; + $this->assertEquals($login, ''); // Expected '' because should failed because login 'auto' does not exists + } }