From 27ac538e74d88965c1398bc2b8b824eb105a925e Mon Sep 17 00:00:00 2001 From: "Laurent Destailleur (aka Eldy)" Date: Wed, 18 Dec 2024 15:54:48 +0100 Subject: [PATCH 01/10] Fix security. Make bypass of checkPHPCode more difficult --- htdocs/core/lib/website2.lib.php | 34 ++++++++++++++++++++------------ test/phpunit/WebsiteTest.php | 20 +++++++++++++++---- 2 files changed, 37 insertions(+), 17 deletions(-) diff --git a/htdocs/core/lib/website2.lib.php b/htdocs/core/lib/website2.lib.php index 8ed23b7d88b63..ef67cd4ca0424 100644 --- a/htdocs/core/lib/website2.lib.php +++ b/htdocs/core/lib/website2.lib.php @@ -694,8 +694,8 @@ function showWebsiteTemplates(Website $website) /** * Check a new string containing only php code (including " * @param string $phpfullcodestring PHP new string. For example "" @@ -742,21 +742,21 @@ function checkPHPCode(&$phpfullcodestringold, &$phpfullcodestring) $forbiddenphpmethods = array('invoke', 'invokeArgs'); // Method of ReflectionFunction to execute a function foreach ($forbiddenphpstrings as $forbiddenphpstring) { - if (preg_match('/'.preg_quote($forbiddenphpstring, '/').'/ms', $phpfullcodestring)) { + if (preg_match('/'.preg_quote($forbiddenphpstring, '/').'/ims', $phpfullcodestring)) { $error++; setEventMessages($langs->trans("DynamicPHPCodeContainsAForbiddenInstruction", $forbiddenphpstring), null, 'errors'); break; } } - foreach ($forbiddenphpfunctions as $forbiddenphpcommand) { - if (preg_match('/'.$forbiddenphpcommand.'\s*\(/ms', $phpfullcodestring)) { + foreach ($forbiddenphpfunctions as $forbiddenphpfunction) { + if (preg_match('/'.$forbiddenphpfunction.'\s*\(/ims', $phpfullcodestring)) { $error++; - setEventMessages($langs->trans("DynamicPHPCodeContainsAForbiddenInstruction", $forbiddenphpcommand), null, 'errors'); + setEventMessages($langs->trans("DynamicPHPCodeContainsAForbiddenInstruction", $forbiddenphpfunction), null, 'errors'); break; } } foreach ($forbiddenphpmethods as $forbiddenphpmethod) { - if (preg_match('/->'.$forbiddenphpmethod.'/ms', $phpfullcodestring)) { + if (preg_match('/->'.$forbiddenphpmethod.'/ims', $phpfullcodestring)) { $error++; setEventMessages($langs->trans("DynamicPHPCodeContainsAForbiddenInstruction", $forbiddenphpmethod), null, 'errors'); break; @@ -764,14 +764,14 @@ function checkPHPCode(&$phpfullcodestringold, &$phpfullcodestring) } } - // This char can be used to execute RCE for example using with echo `ls` + // This char can be used to execute RCE for example by using echo `ls` if (!$error) { $forbiddenphpchars = array(); if (!getDolGlobalString('WEBSITE_PHP_ALLOW_DANGEROUS_CHARS')) { // If option is not on, we disallow functions to execute commands $forbiddenphpchars = array("`"); } foreach ($forbiddenphpchars as $forbiddenphpchar) { - if (preg_match('/'.$forbiddenphpchar.'/ms', $phpfullcodestring)) { + if (preg_match('/'.$forbiddenphpchar.'/ims', $phpfullcodestring)) { $error++; setEventMessages($langs->trans("DynamicPHPCodeContainsAForbiddenInstruction", $forbiddenphpchar), null, 'errors'); break; @@ -779,17 +779,25 @@ function checkPHPCode(&$phpfullcodestringold, &$phpfullcodestring) } } - // Deny dynamic functions '${a}(' or '$a[b](' => So we refuse '}(' and '](' + // Deny code to call a function obfuscated with comment, like "exec/*...*/ ('ls')"; if (!$error) { - if (preg_match('/[}\]]\(/ims', $phpfullcodestring)) { + if (preg_match('/\*\/\s*\(/ims', $phpfullcodestring)) { + $error++; + setEventMessages($langs->trans("DynamicPHPCodeContainsAForbiddenInstruction", $forbiddenphpmethod), null, 'errors'); + } + } + + // Deny dynamic functions '${a}(' or '$a[b](' => So we refuse '}(' and '](' + if (!$error) { + if (preg_match('/[}\]]\s*\(/ims', $phpfullcodestring)) { $error++; setEventMessages($langs->trans("DynamicPHPCodeContainsAForbiddenInstruction", ']('), null, 'errors'); } } - // Deny dynamic functions '$xxx(' + // Deny dynamic functions '$xxx(' or '$xxx (' or '$xxx" (' if (!$error) { - if (preg_match('/\$[a-z0-9_\-\/\*]+\(/ims', $phpfullcodestring)) { + if (preg_match('/\$[a-z0-9_\-\/\*\"]+\s*\(/ims', $phpfullcodestring)) { $error++; setEventMessages($langs->trans("DynamicPHPCodeContainsAForbiddenInstruction", '$...('), null, 'errors'); } diff --git a/test/phpunit/WebsiteTest.php b/test/phpunit/WebsiteTest.php index 6ccccb17b23cd..a067202ef85fc 100644 --- a/test/phpunit/WebsiteTest.php +++ b/test/phpunit/WebsiteTest.php @@ -65,11 +65,11 @@ print "Load permissions for admin user nb 1\n"; $user->fetch(1); $user->loadRights(); - - if (empty($user->rights->website)) { - $user->rights->website = new stdClass(); - } } +if (empty($user->rights->website)) { + $user->rights->website = new stdClass(); +} + $conf->global->MAIN_DISABLE_ALL_MAILS = 1; @@ -143,6 +143,18 @@ public function testCheckPHPCode() print __METHOD__." result checkPHPCode=".$result."\n"; $this->assertEquals($result, 1, 'checkPHPCode did not detect the string was dangerous'); + $t = ''; + $s = ''; + $result = checkPHPCode($t, $s); + print __METHOD__." result checkPHPCode=".$result."\n"; + $this->assertEquals($result, 1, 'checkPHPCode did not detect the string was dangerous'); + + $t = ''; + $s = ''; + $result = checkPHPCode($t, $s); + print __METHOD__." result checkPHPCode=".$result."\n"; + $this->assertEquals($result, 1, 'checkPHPCode did not detect the string was dangerous'); + $t = ''; $s = ';").($_^"/"); ?>'; $result = checkPHPCode($t, $s); From 23a0c427ea96fb085761fef9cc2f20d2d1845c73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20FRANCE?= Date: Wed, 18 Dec 2024 17:56:41 +0100 Subject: [PATCH 02/10] fix CI --- htdocs/imports/import.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/htdocs/imports/import.php b/htdocs/imports/import.php index 4d42caf99def5..b66cfc1f399f4 100644 --- a/htdocs/imports/import.php +++ b/htdocs/imports/import.php @@ -375,7 +375,7 @@ $label = $objimport->array_import_label[$key]; print '
'; print img_object($objimport->array_import_module[$key]['module']->getName(), $entityicon, 'class="pictofixedwidth"'); - print dolPrintHtml($label); + print dolPrintHTML($label); print '
'; print ''; if ($objimport->array_import_perms[$key]) { From 6145b2acef3dec5799d7c29b307ec374a3842f10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20FRANCE?= Date: Wed, 18 Dec 2024 18:01:13 +0100 Subject: [PATCH 03/10] fix CI --- htdocs/core/lib/website2.lib.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/htdocs/core/lib/website2.lib.php b/htdocs/core/lib/website2.lib.php index ef67cd4ca0424..01abafc8a113b 100644 --- a/htdocs/core/lib/website2.lib.php +++ b/htdocs/core/lib/website2.lib.php @@ -783,7 +783,7 @@ function checkPHPCode(&$phpfullcodestringold, &$phpfullcodestring) if (!$error) { if (preg_match('/\*\/\s*\(/ims', $phpfullcodestring)) { $error++; - setEventMessages($langs->trans("DynamicPHPCodeContainsAForbiddenInstruction", $forbiddenphpmethod), null, 'errors'); + setEventMessages($langs->trans("DynamicPHPCodeContainsAForbiddenInstruction", $phpfullcodestring), null, 'errors'); } } From 55fe6f9caa2f22dc66e55c3f6e6d50060322598d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20FRANCE?= Date: Wed, 18 Dec 2024 18:06:08 +0100 Subject: [PATCH 04/10] fix CI --- htdocs/core/lib/website2.lib.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/htdocs/core/lib/website2.lib.php b/htdocs/core/lib/website2.lib.php index 01abafc8a113b..18b55bc4a3244 100644 --- a/htdocs/core/lib/website2.lib.php +++ b/htdocs/core/lib/website2.lib.php @@ -783,7 +783,7 @@ function checkPHPCode(&$phpfullcodestringold, &$phpfullcodestring) if (!$error) { if (preg_match('/\*\/\s*\(/ims', $phpfullcodestring)) { $error++; - setEventMessages($langs->trans("DynamicPHPCodeContainsAForbiddenInstruction", $phpfullcodestring), null, 'errors'); + setEventMessages($langs->trans("DynamicPHPCodeContainsAForbiddenInstruction", "exec/*...*/ ('ls')"), null, 'errors'); } } From 7f4b2b08b44757fb5a66129021f1e854a5fa792b Mon Sep 17 00:00:00 2001 From: "Laurent Destailleur (aka Eldy)" Date: Wed, 18 Dec 2024 19:00:33 +0100 Subject: [PATCH 05/10] Complete phpunit and tests to avoid use of non expected function --- htdocs/comm/card.php | 50 +++++++------- htdocs/core/lib/functions.lib.php | 11 ++- htdocs/core/lib/website2.lib.php | 25 +++++-- test/phpunit/AllTests.php | 2 + test/phpunit/SecurityLoginTest.php | 106 +++++++++++++++++++++++++++++ test/phpunit/SecurityTest.php | 46 +++---------- test/phpunit/WebsiteTest.php | 29 +++++++- 7 files changed, 200 insertions(+), 69 deletions(-) create mode 100644 test/phpunit/SecurityLoginTest.php diff --git a/htdocs/comm/card.php b/htdocs/comm/card.php index 57ce524af6bdf..5ed10dc2bd11e 100644 --- a/htdocs/comm/card.php +++ b/htdocs/comm/card.php @@ -1712,35 +1712,37 @@ } // Add invoice - if ($user->socid == 0) { - if (isModEnabled('deplacement') && $object->status == 1) { - $langs->load("trips"); - print ''; - } + if (isModEnabled('deplacement') && $object->status == 1) { + $langs->load("trips"); + print ''; + } + + if (isModEnabled('invoice') && $object->status == 1) { + if (!$user->hasRight('facture', 'creer')) { + $langs->load("bills"); + print ''; + } else { + $langs->loadLangs(array("orders", "bills")); - if (isModEnabled('invoice') && $object->status == 1) { - if (!$user->hasRight('facture', 'creer')) { - $langs->load("bills"); - print ''; + if ($object->client != 0 && $object->client != 2) { + print ''; } else { - $langs->loadLangs(array("orders", "bills")); - - if (isModEnabled('order')) { - if ($object->client != 0 && $object->client != 2) { - if (!empty($orders2invoice) && $orders2invoice > 0) { - print ''; - } else { - print ''; - } - } else { - print ''; - } - } + print ''; + } + } + } + if (isModEnabled('invoice') && $object->status == 1) { + if ($user->hasRight('facture', 'creer')) { + if (isModEnabled('order')) { if ($object->client != 0 && $object->client != 2) { - print ''; + if (!empty($orders2invoice) && $orders2invoice > 0) { + print ''; + } else { + print ''; + } } else { - print ''; + print ''; } } } diff --git a/htdocs/core/lib/functions.lib.php b/htdocs/core/lib/functions.lib.php index a3849578d8b97..6f47d17176133 100644 --- a/htdocs/core/lib/functions.lib.php +++ b/htdocs/core/lib/functions.lib.php @@ -10586,6 +10586,7 @@ function dol_eval($s, $returnvalue = 1, $hideerrors = 1, $onlysimplestring = '1' while ($scheck && $savescheck != $scheck) { $savescheck = $scheck; $scheck = preg_replace('/->[a-zA-Z0-9_]+\(/', '->__METHOD__', $scheck); // accept parenthesis in '...->method(...' + $scheck = preg_replace('/::[a-zA-Z0-9_]+\(/', '->__METHOD__', $scheck); // accept parenthesis in '...::method(...' $scheck = preg_replace('/^\(/', '__PARENTHESIS__ ', $scheck); // accept parenthesis in '(...'. Must replace with __PARENTHESIS__ with a space after to allow following substitutions $scheck = preg_replace('/\s\(/', '__PARENTHESIS__ ', $scheck); // accept parenthesis in '... (' like in 'if ($a == 1)'. Must replace with __PARENTHESIS__ with a space after to allow following substitutions $scheck = preg_replace('/^!?[a-zA-Z0-9_]+\(/', '__FUNCTION__', $scheck); // accept parenthesis in 'function(' and '!function(' @@ -10646,22 +10647,26 @@ function dol_eval($s, $returnvalue = 1, $hideerrors = 1, $onlysimplestring = '1' $forbiddenphpstrings = array('$$', '$_', '}['); $forbiddenphpstrings = array_merge($forbiddenphpstrings, array('_ENV', '_SESSION', '_COOKIE', '_GET', '_POST', '_REQUEST', 'ReflectionFunction')); + // We list all forbidden function as keywords we don't want to see (we don't mind it if is "kewyord(" or just "keyword", we don't want "keyword" at all) $forbiddenphpfunctions = array(); // @phpcs:ignore $forbiddenphpfunctions = array_merge($forbiddenphpfunctions, array("base64"."_"."decode", "rawurl"."decode", "url"."decode", "str"."_rot13", "hex"."2bin")); // name of forbidden functions are split to avoid false positive - $forbiddenphpfunctions = array_merge($forbiddenphpfunctions, array("fopen", "file_put_contents", "fputs", "fputscsv", "fwrite", "fpassthru", "require", "include", "mkdir", "rmdir", "symlink", "touch", "unlink", "umask")); $forbiddenphpfunctions = array_merge($forbiddenphpfunctions, array("override_function", "session_id", "session_create_id", "session_regenerate_id")); $forbiddenphpfunctions = array_merge($forbiddenphpfunctions, array("get_defined_functions", "get_defined_vars", "get_defined_constants", "get_declared_classes")); - $forbiddenphpfunctions = array_merge($forbiddenphpfunctions, array("function", "call_user_func")); + $forbiddenphpfunctions = array_merge($forbiddenphpfunctions, array("function", "call_user_func", "call_user_func_array")); $forbiddenphpfunctions = array_merge($forbiddenphpfunctions, array("require", "include", "require_once", "include_once")); $forbiddenphpfunctions = array_merge($forbiddenphpfunctions, array("exec", "passthru", "shell_exec", "system", "proc_open", "popen")); $forbiddenphpfunctions = array_merge($forbiddenphpfunctions, array("dol_eval", "executeCLI", "verifCond")); // native dolibarr functions $forbiddenphpfunctions = array_merge($forbiddenphpfunctions, array("eval", "create_function", "assert", "mb_ereg_replace")); // function with eval capabilities $forbiddenphpfunctions = array_merge($forbiddenphpfunctions, array("dol_compress_dir", "dol_decode", "dol_delete_file", "dol_delete_dir", "dol_delete_dir_recursive", "dol_copy", "archiveOrBackupFile")); // more dolibarr functions + $forbiddenphpfunctions = array_merge($forbiddenphpfunctions, array("fopen", "file_put_contents", "fputs", "fputscsv", "fwrite", "fpassthru", "mkdir", "rmdir", "symlink", "touch", "unlink", "umask")); + $forbiddenphpfunctions = array_merge($forbiddenphpfunctions, array("require", "include")); $forbiddenphpmethods = array('invoke', 'invokeArgs'); // Method of ReflectionFunction to execute a function - $forbiddenphpregex = 'global\s+\$|\b('.implode('|', $forbiddenphpfunctions).')\b'; + $forbiddenphpregex = 'global\s*\$'; + $forbiddenphpregex .= '|'; + $forbiddenphpregex .= '\b('.implode('|', $forbiddenphpfunctions).')\b'; $forbiddenphpmethodsregex = '->('.implode('|', $forbiddenphpmethods).')'; diff --git a/htdocs/core/lib/website2.lib.php b/htdocs/core/lib/website2.lib.php index ef67cd4ca0424..78a5e6a002947 100644 --- a/htdocs/core/lib/website2.lib.php +++ b/htdocs/core/lib/website2.lib.php @@ -722,13 +722,15 @@ function checkPHPCode(&$phpfullcodestringold, &$phpfullcodestring) // Then check forbidden commands if (!$error) { - $forbiddenphpstrings = array('$$', '}['); - $forbiddenphpstrings = array_merge($forbiddenphpstrings, array('ReflectionFunction')); + $forbiddenphpstrings = array('$$', '$_', '}['); + //$forbiddenphpstrings = array_merge($forbiddenphpstrings, array('_ENV', '_SESSION', '_COOKIE', '_GET', '_POST', '_REQUEST', 'ReflectionFunction')); + $forbiddenphpstrings = array_merge($forbiddenphpstrings, array('_ENV', 'ReflectionFunction')); $forbiddenphpfunctions = array(); + //$forbiddenphpfunctions = array_merge($forbiddenphpfunctions, array("base64"."_"."decode", "rawurl"."decode", "url"."decode", "str"."_rot13", "hex"."2bin")); // name of forbidden functions are split to avoid false positive $forbiddenphpfunctions = array_merge($forbiddenphpfunctions, array("override_function", "session_id", "session_create_id", "session_regenerate_id")); $forbiddenphpfunctions = array_merge($forbiddenphpfunctions, array("get_defined_functions", "get_defined_vars", "get_defined_constants", "get_declared_classes")); - $forbiddenphpfunctions = array_merge($forbiddenphpfunctions, array("call_user_func")); + $forbiddenphpfunctions = array_merge($forbiddenphpfunctions, array("call_user_func", "call_user_func_array")); //$forbiddenphpfunctions = array_merge($forbiddenphpfunctions, array("require", "include", "require_once", "include_once")); if (!getDolGlobalString('WEBSITE_PHP_ALLOW_EXEC')) { // If option is not on, we disallow functions to execute commands $forbiddenphpfunctions = array_merge($forbiddenphpfunctions, array("exec", "passthru", "shell_exec", "system", "proc_open", "popen")); @@ -736,8 +738,10 @@ function checkPHPCode(&$phpfullcodestringold, &$phpfullcodestring) $forbiddenphpfunctions = array_merge($forbiddenphpfunctions, array("eval", "create_function", "assert", "mb_ereg_replace")); // function with eval capabilities } if (!getDolGlobalString('WEBSITE_PHP_ALLOW_WRITE')) { // If option is not on, we disallow functions to write files + $forbiddenphpfunctions = array_merge($forbiddenphpfunctions, array("dol_compress_dir", "dol_decode", "dol_delete_file", "dol_delete_dir", "dol_delete_dir_recursive", "dol_copy", "archiveOrBackupFile")); // more dolibarr functions $forbiddenphpfunctions = array_merge($forbiddenphpfunctions, array("fopen", "file_put_contents", "fputs", "fputscsv", "fwrite", "fpassthru", "mkdir", "rmdir", "symlink", "touch", "unlink", "umask")); } + //$forbiddenphpfunctions = array_merge($forbiddenphpfunctions, array("require", "include")); $forbiddenphpmethods = array('invoke', 'invokeArgs'); // Method of ReflectionFunction to execute a function @@ -748,13 +752,22 @@ function checkPHPCode(&$phpfullcodestringold, &$phpfullcodestring) break; } } - foreach ($forbiddenphpfunctions as $forbiddenphpfunction) { - if (preg_match('/'.$forbiddenphpfunction.'\s*\(/ims', $phpfullcodestring)) { + /* replaced with next block + foreach ($forbiddenphpfunctions as $forbiddenphpfunction) { // Check "function(" but also "'function'(" and "function (" + if (preg_match('/'.$forbiddenphpfunction.'[\'\s]*\(/ims', $phpfullcodestring)) { + $error++; + setEventMessages($langs->trans("DynamicPHPCodeContainsAForbiddenInstruction", $forbiddenphpfunction), null, 'errors'); + break; + } + }*/ + foreach ($forbiddenphpfunctions as $forbiddenphpfunction) { // Check "function" whatever is "function(" or "function'(" or "function (" or "function" + if (preg_match('/\b'.$forbiddenphpfunction.'\b/ims', $phpfullcodestring)) { $error++; setEventMessages($langs->trans("DynamicPHPCodeContainsAForbiddenInstruction", $forbiddenphpfunction), null, 'errors'); break; } } + foreach ($forbiddenphpmethods as $forbiddenphpmethod) { if (preg_match('/->'.$forbiddenphpmethod.'/ims', $phpfullcodestring)) { $error++; @@ -803,7 +816,7 @@ function checkPHPCode(&$phpfullcodestringold, &$phpfullcodestring) } } - // No need to block $conf->global->aaa() because PHP try to run method aaa an not function into $conf->global->aaa. + // 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) { diff --git a/test/phpunit/AllTests.php b/test/phpunit/AllTests.php index 12e58357aa479..ae5b2c0359574 100644 --- a/test/phpunit/AllTests.php +++ b/test/phpunit/AllTests.php @@ -134,6 +134,8 @@ public static function suite() $suite->addTestSuite('SecurityTest'); require_once dirname(__FILE__).'/SecurityGETPOSTTest.php'; $suite->addTestSuite('SecurityGETPOSTTest'); + require_once dirname(__FILE__).'/SecurityLoginTest.php'; + $suite->addTestSuite('SecurityLoginTest'); require_once dirname(__FILE__).'/UserTest.php'; $suite->addTestSuite('UserTest'); diff --git a/test/phpunit/SecurityLoginTest.php b/test/phpunit/SecurityLoginTest.php new file mode 100644 index 0000000000000..2e4d190567c3b --- /dev/null +++ b/test/phpunit/SecurityLoginTest.php @@ -0,0 +1,106 @@ + + * Copyright (C) 2023 Alexandre Janniaux + * Copyright (C) 2024 Frédéric France + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * or see https://www.gnu.org/ + */ + +/** + * \file test/phpunit/SecurityTest.php + * \ingroup test + * \brief PHPUnit test + * \remarks To run this script as CLI: phpunit filename.php + */ + +global $conf,$user,$langs,$db; +//define('TEST_DB_FORCE_TYPE','mysql'); // This is to force using mysql driver +//require_once 'PHPUnit/Autoload.php'; + +if (! defined('NOREQUIRESOC')) { + define('NOREQUIRESOC', '1'); +} +if (! defined('NOCSRFCHECK')) { + define('NOCSRFCHECK', '1'); +} +if (! defined('NOTOKENRENEWAL')) { + define('NOTOKENRENEWAL', '1'); +} +if (! defined('NOREQUIREMENU')) { + define('NOREQUIREMENU', '1'); // If there is no menu to show +} +if (! defined('NOREQUIREHTML')) { + define('NOREQUIREHTML', '1'); // If we don't need to load the html.form.class.php +} +if (! defined('NOREQUIREAJAX')) { + define('NOREQUIREAJAX', '1'); +} +if (! defined("NOLOGIN")) { + define("NOLOGIN", '1'); // If this page is public (can be called outside logged session) +} +if (! defined("NOSESSION")) { + define("NOSESSION", '1'); +} + +require_once dirname(__FILE__).'/../../htdocs/main.inc.php'; // We force include of main.inc.php instead of master.inc.php even if we are in CLI mode because it contains a lot of security components we want to test. +require_once dirname(__FILE__).'/../../htdocs/core/lib/security.lib.php'; +require_once dirname(__FILE__).'/../../htdocs/core/lib/security2.lib.php'; +require_once dirname(__FILE__).'/CommonClassTest.class.php'; + +if (empty($user->id)) { + print "Load permissions for admin user nb 1\n"; + $user->fetch(1); + $user->loadRights(); +} +$conf->global->MAIN_DISABLE_ALL_MAILS = 1; + + +/** + * Class for PHPUnit tests + * + * @backupGlobals disabled + * @backupStaticAttributes enabled + * @remarks backupGlobals must be disabled to have db,conf,user and lang not erased. + */ +class SecurityLoginTest extends CommonClassTest +{ + /** + * 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, 'Error'); // Expected '' because should failed because login 'auto' does not exists + } +} diff --git a/test/phpunit/SecurityTest.php b/test/phpunit/SecurityTest.php index 294d1596fec84..9b5db827bc3b5 100644 --- a/test/phpunit/SecurityTest.php +++ b/test/phpunit/SecurityTest.php @@ -636,13 +636,13 @@ public function testDolEval() $this->assertEquals('Bad string syntax to evaluate: new __forbiddenstring__(\'abc\')', $result); - $result = (string) dol_eval('$a=function() { }; $a;', 1, 1, '0'); - print "result5 = ".$result."\n"; - $this->assertStringContainsString('Bad string syntax to evaluate', $result); + $result = dol_eval('$a=function() { }; $a', 1, 1, '0'); // result of dol_eval may be an object Closure + print "result5 = ".json_encode($result)."\n"; + $this->assertStringContainsString('Bad string syntax to evaluate', json_encode($result)); - $result = (string) dol_eval('$a=function() { }; $a;', 1, 1, '1'); - print "result6 = ".$result."\n"; - $this->assertStringContainsString('Bad string syntax to evaluate', $result); + $result = dol_eval('$a=function() { }; $a();', 1, 1, '1'); + print "result6 = ".json_encode($result)."\n"; + $this->assertStringContainsString('Bad string syntax to evaluate', json_encode($result)); $result = (string) dol_eval('$a=exec("ls");', 1, 1); print "result7 = ".$result."\n"; @@ -723,6 +723,11 @@ public function testDolEval() $result = (string) dol_eval('($a = "ex") && ($b = "ec") && ($cmd = "$a$b") && $cmd ("curl localhost:5555")', 1, 0); print "result22 = ".$result."\n"; $this->assertStringContainsString('Bad string syntax to evaluate', $result, 'Test 22'); + + + $result = (string) dol_eval('\'exec\'("aaa")', 1, 0); + print "result1 = ".$result."\n"; + $this->assertStringContainsString('Bad string syntax to evaluate', json_encode($result), 'Cant find the string Bad string syntaxwhen i should'); } /** @@ -966,33 +971,4 @@ public function testDolHtmlWithNoJs() return 0; } - - - /** - * 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, 'Error'); // Expected '' because should failed because login 'auto' does not exists - } } diff --git a/test/phpunit/WebsiteTest.php b/test/phpunit/WebsiteTest.php index a067202ef85fc..bbf80cad4c59e 100644 --- a/test/phpunit/WebsiteTest.php +++ b/test/phpunit/WebsiteTest.php @@ -132,11 +132,22 @@ public function testDolStripPhpCode() */ public function testCheckPHPCode() { - global $user; + global $conf, $user; // Force permission so this is not the permission that will affect result of checkPHPCode $user->rights->website->writephp = 1; + // 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 + $t = ''; $s = ''; $result = checkPHPCode($t, $s); @@ -155,11 +166,27 @@ public function testCheckPHPCode() print __METHOD__." result checkPHPCode=".$result."\n"; $this->assertEquals($result, 1, 'checkPHPCode did not detect the string was dangerous'); + $t = ''; + $s = ''; + $result = checkPHPCode($t, $s); + print __METHOD__." result checkPHPCode=".$result."\n"; + $this->assertEquals($result, 1, 'checkPHPCode did not detect the string was dangerous'); + $t = ''; $s = ';").($_^"/"); ?>'; $result = checkPHPCode($t, $s); print __METHOD__." result checkPHPCode=".$result."\n"; $this->assertEquals($result, 1, 'checkPHPCode did not detect the string was dangerous'); + + // Dangerous but legitimate due to option WEBSITE_PHP_ALLOW_EXEC + + $conf->global->WEBSITE_PHP_ALLOW_EXEC = 1; + + $t = ''; + $s = ''; + $result = checkPHPCode($t, $s); + print __METHOD__." result checkPHPCode=".$result."\n"; + $this->assertEquals($result, 0, 'checkPHPCode did not accept the exec. it should when WEBSITE_PHP_ALLOW_EXEC is set.'); } /** From 8bab8f90c7167cb87ec2465426af9a90eae30996 Mon Sep 17 00:00:00 2001 From: "Laurent Destailleur (aka Eldy)" Date: Wed, 18 Dec 2024 19:47:53 +0100 Subject: [PATCH 06/10] Debug v21 - Must use USF --- htdocs/comm/action/card.php | 6 ++-- htdocs/core/class/html.form.class.php | 48 ++++++++++++++++----------- htdocs/societe/card.php | 4 +-- htdocs/societe/list.php | 2 +- 4 files changed, 35 insertions(+), 25 deletions(-) diff --git a/htdocs/comm/action/card.php b/htdocs/comm/action/card.php index e7f8a9b1139a9..b00fe376ee1bd 100644 --- a/htdocs/comm/action/card.php +++ b/htdocs/comm/action/card.php @@ -1546,8 +1546,8 @@ function init_repeat() $listofuserid[$firstelem['id']]['transparency'] = (GETPOSTISSET('transparency') ? GETPOST('transparency', 'alpha') : 0); // 0 by default when refreshing } } - print '
'; - print $form->select_dolusers_forevent(($action == 'create' ? 'add' : 'update'), 'assignedtouser', 1, array(), 0, '', array(), 0, 0, 0, 'AND u.statut != 0', 1, $listofuserid, $listofcontactid, $listofotherid); + print '
'; + print $form->select_dolusers_forevent(($action == 'create' ? 'add' : 'update'), 'assignedtouser', 1, array(), 0, '', array(), 0, 0, 0, 'u.statut:<>:0', 1, $listofuserid, $listofcontactid, $listofotherid); print '
'; print ''; @@ -2164,7 +2164,7 @@ function setdatefields() print ''.$langs->trans("ActionAssignedTo").''; print '
'; - print $form->select_dolusers_forevent(($action == 'create' ? 'add' : 'update'), 'assignedtouser', 1, array(), 0, '', array(), 0, 0, 0, 'AND u.statut != 0', 1, $listofuserid, $listofcontactid, $listofotherid); + print $form->select_dolusers_forevent(($action == 'create' ? 'add' : 'update'), 'assignedtouser', 1, array(), 0, '', array(), 0, 0, 0, 'u.statut:<>:0', 1, $listofuserid, $listofcontactid, $listofotherid); print '
'; /*if (in_array($user->id,array_keys($listofuserid))) { diff --git a/htdocs/core/class/html.form.class.php b/htdocs/core/class/html.form.class.php index a291083bf6262..a47f07f170394 100644 --- a/htdocs/core/class/html.form.class.php +++ b/htdocs/core/class/html.form.class.php @@ -2228,7 +2228,13 @@ public function select_dolusers($selected = '', $htmlname = 'userid', $show_empt $sql .= " AND u.fk_soc IS NULL"; } if (!empty($morefilter)) { - $sql .= " " . $morefilter; + $errormessage = ''; + $sql .= forgeSQLFromUniversalSearchCriteria($morefilter, $errormessage); + if ($errormessage) { + $this->errors[] = $errormessage; + dol_syslog(__METHOD__.' '.implode(',', $this->errors), LOG_ERR); + return -1; + } } //Add hook to filter on user (for example on usergroup define in custom modules) @@ -2425,28 +2431,28 @@ public function select_dolusers($selected = '', $htmlname = 'userid', $show_empt * Return select list of users. Selected users are stored into session. * List of users are provided into $_SESSION['assignedtouser']. * - * @param string $action Value for $action - * @param string $htmlname Field name in form - * @param int<0,1> $show_empty 0=list without the empty value, 1=add empty value - * @param int[] $exclude Array list of users id to exclude - * @param int<0,1> $disabled If select list must be disabled - * @param int[]|string $include Array list of users id to include or 'hierarchy' to have only supervised users - * @param int[]|int $enableonly Array list of users id to be enabled. All other must be disabled - * @param string $force_entity '0' or Ids of environment to force - * @param int $maxlength Maximum length of string into list (0=no limit) - * @param int<0,1> $showstatus 0=show user status only if status is disabled, 1=always show user status into label, -1=never show user status - * @param string $morefilter Add more filters into sql request - * @param int $showproperties Show properties of each attendees - * @param int[] $listofuserid Array with properties of each user - * @param int[] $listofcontactid Array with properties of each contact - * @param int[] $listofotherid Array with properties of each other contact - * @return string HTML select string + * @param string $action Value for $action + * @param string $htmlname Field name in form + * @param int<0,1> $show_empty 0=list without the empty value, 1=add empty value + * @param int[] $exclude Array list of users id to exclude + * @param int<0,1> $disabled If select list must be disabled + * @param int[]|''|'hierarchy'|'hierarchyme' $include Array list of users id to include or 'hierarchy' to have only supervised users + * @param int[]|int $enableonly Array list of users id to be enabled. All other must be disabled + * @param string $force_entity '0' or Ids of environment to force + * @param int $maxlength Maximum length of string into list (0=no limit) + * @param int<0,1> $showstatus 0=show user status only if status is disabled, 1=always show user status into label, -1=never show user status + * @param string $morefilter Add more filters into sql request (Example: '(employee:=:1)'). This value must not come from user input. + * @param int $showproperties Show properties of each attendees + * @param int[] $listofuserid Array with properties of each user + * @param int[] $listofcontactid Array with properties of each contact + * @param int[] $listofotherid Array with properties of each other contact + * @return string HTML select string * @see select_dolgroups() */ public function select_dolusers_forevent($action = '', $htmlname = 'userid', $show_empty = 0, $exclude = null, $disabled = 0, $include = array(), $enableonly = array(), $force_entity = '0', $maxlength = 0, $showstatus = 0, $morefilter = '', $showproperties = 0, $listofuserid = array(), $listofcontactid = array(), $listofotherid = array()) { // phpcs:enable - global $langs; + global $langs, $user; $userstatic = new User($this->db); $out = ''; @@ -2480,7 +2486,11 @@ public function select_dolusers_forevent($action = '', $htmlname = 'userid', $sh $out .= ' (' . $langs->trans("Owner") . ')'; } if ($nbassignetouser > 1 && $action != 'view') { - $out .= ' '; + if ($user->hasRight('agenda', 'allactions', 'create') || $userstatic->id != $user->id) { + // If user has all permission, he should be ableto remove a assignee. + // If user has not all permission, he can onlyremove assignee of other (he can't remove itself) + $out .= ' '; + } } // Show my availability if ($showproperties) { diff --git a/htdocs/societe/card.php b/htdocs/societe/card.php index e4a3c6c47d5ec..9e149817d8308 100644 --- a/htdocs/societe/card.php +++ b/htdocs/societe/card.php @@ -1960,7 +1960,7 @@ function manageprospectcustomer(element) { print ''.$form->editfieldkey('AllocateCommercial', 'commercial_id', '', $object, 0).''; print ''; // TODO Use select_doluser in multiselect mode - $userlist = $form->select_dolusers($selected, '', 0, null, 0, '', '', '0', 0, 0, 'AND u.statut = 1', 0, '', '', 0, 2); + $userlist = $form->select_dolusers($selected, '', 0, null, 0, '', '', '0', 0, 0, 'u.statut:=:1', 0, '', '', 0, 2); // Note: If user has no right to "see all thirdparties", we force selection of sale representative to him, so after creation he can see the record. $selected = (GETPOSTISARRAY('commercial') ? GETPOST('commercial', 'array:int') : (GETPOSTINT('commercial') > 0 ? array(GETPOSTINT('commercial')) : array($user->id))); print img_picto('', 'user').$form->multiselectarray('commercial', $userlist, $selected, 0, 0, 'quatrevingtpercent widthcentpercentminusx', 0, 0); @@ -2812,7 +2812,7 @@ function init_check_no_email(input) { print ''; print ''.$form->editfieldkey('AllocateCommercial', 'commercial_id', '', $object, 0).''; print ''; - $userlist = $form->select_dolusers('', '', 0, null, 0, '', '', 0, 0, 0, 'AND u.statut = 1', 0, '', '', 0, 1); + $userlist = $form->select_dolusers('', '', 0, null, 0, '', '', 0, 0, 0, 'u.statut:=:1', 0, '', '', 0, 1); $arrayselected = GETPOST('commercial', 'array'); if (empty($arrayselected)) { $arrayselected = $object->getSalesRepresentatives($user, 1); diff --git a/htdocs/societe/list.php b/htdocs/societe/list.php index aaa8868746761..70fe8a2f34f33 100644 --- a/htdocs/societe/list.php +++ b/htdocs/societe/list.php @@ -1290,7 +1290,7 @@ } // If the user can view prospects other than his' -$userlist = $form->select_dolusers('', '', 0, null, 0, '', '', 0, 0, 0, 'AND u.statut = 1', 0, '', '', 0, 1); +$userlist = $form->select_dolusers('', '', 0, null, 0, '', '', 0, 0, 0, 'u.statut:=:1', 0, '', '', 0, 1); $userlist[-2] = $langs->trans("NoSalesRepresentativeAffected"); if ($user->hasRight("societe", "client", "voir") || $socid) { $moreforfilter .= '
'; From 93884e3424afc6addea5810608bfce0de1bba1cc Mon Sep 17 00:00:00 2001 From: "Laurent Destailleur (aka Eldy)" Date: Wed, 18 Dec 2024 20:23:21 +0100 Subject: [PATCH 07/10] Fix on auto event, can't change the owner of event. --- htdocs/comm/action/card.php | 3 ++- htdocs/core/class/html.form.class.php | 18 ++++++++++++++++-- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/htdocs/comm/action/card.php b/htdocs/comm/action/card.php index b00fe376ee1bd..338e63e57280e 100644 --- a/htdocs/comm/action/card.php +++ b/htdocs/comm/action/card.php @@ -2164,7 +2164,8 @@ function setdatefields() print ''.$langs->trans("ActionAssignedTo").''; print '
'; - print $form->select_dolusers_forevent(($action == 'create' ? 'add' : 'update'), 'assignedtouser', 1, array(), 0, '', array(), 0, 0, 0, 'u.statut:<>:0', 1, $listofuserid, $listofcontactid, $listofotherid); + $canremoveowner = ($object->type != 'systemauto'); + print $form->select_dolusers_forevent(($action == 'create' ? 'add' : 'update'), 'assignedtouser', 1, array(), 0, '', array(), 0, 0, 0, 'u.statut:<>:0', 1, $listofuserid, $listofcontactid, $listofotherid, $canremoveowner); print '
'; /*if (in_array($user->id,array_keys($listofuserid))) { diff --git a/htdocs/core/class/html.form.class.php b/htdocs/core/class/html.form.class.php index a47f07f170394..a0e1cd1985b4f 100644 --- a/htdocs/core/class/html.form.class.php +++ b/htdocs/core/class/html.form.class.php @@ -2446,10 +2446,11 @@ public function select_dolusers($selected = '', $htmlname = 'userid', $show_empt * @param int[] $listofuserid Array with properties of each user * @param int[] $listofcontactid Array with properties of each contact * @param int[] $listofotherid Array with properties of each other contact + * @param int $canremoveowner 1 if we can remove owner, 0=no way * @return string HTML select string * @see select_dolgroups() */ - public function select_dolusers_forevent($action = '', $htmlname = 'userid', $show_empty = 0, $exclude = null, $disabled = 0, $include = array(), $enableonly = array(), $force_entity = '0', $maxlength = 0, $showstatus = 0, $morefilter = '', $showproperties = 0, $listofuserid = array(), $listofcontactid = array(), $listofotherid = array()) + public function select_dolusers_forevent($action = '', $htmlname = 'userid', $show_empty = 0, $exclude = null, $disabled = 0, $include = array(), $enableonly = array(), $force_entity = '0', $maxlength = 0, $showstatus = 0, $morefilter = '', $showproperties = 0, $listofuserid = array(), $listofcontactid = array(), $listofotherid = array(), $canremoveowner = 1) { // phpcs:enable global $langs, $user; @@ -2485,8 +2486,21 @@ public function select_dolusers_forevent($action = '', $htmlname = 'userid', $sh $ownerid = $value['id']; $out .= ' (' . $langs->trans("Owner") . ')'; } + // Add picto to delete owner/assignee if ($nbassignetouser > 1 && $action != 'view') { - if ($user->hasRight('agenda', 'allactions', 'create') || $userstatic->id != $user->id) { + $canremoveassignee = 1; + if ($i == 0) { + // We are on the owner of the event + if (!$canremoveowner) { + $canremoveassignee = 0; + } + if ($userstatic->id == $user->id && !$user->hasRight('agenda', 'allactions', 'create')) { + $canremoveassignee = 0; // Can't remove myself if i am the owner + } + } else { + // We are not on the owner of the event but on a secondary assignee + } + if ($canremoveassignee) { // If user has all permission, he should be ableto remove a assignee. // If user has not all permission, he can onlyremove assignee of other (he can't remove itself) $out .= ' '; From 50695e4dcf6ec0a3a63cc94c96bedfc3947fea62 Mon Sep 17 00:00:00 2001 From: "Laurent Destailleur (aka Eldy)" Date: Wed, 18 Dec 2024 21:19:15 +0100 Subject: [PATCH 08/10] Debug v21 --- htdocs/comm/action/card.php | 32 +++++++++++++++------------ htdocs/core/class/html.form.class.php | 9 ++++---- 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/htdocs/comm/action/card.php b/htdocs/comm/action/card.php index 338e63e57280e..2e02f37ebec26 100644 --- a/htdocs/comm/action/card.php +++ b/htdocs/comm/action/card.php @@ -1811,7 +1811,7 @@ function init_repeat() print "\n".''; +//print 'Click'; + print "\n"; diff --git a/htdocs/langs/en_US/main.lang b/htdocs/langs/en_US/main.lang index 16db52a977034..4099d76274578 100644 --- a/htdocs/langs/en_US/main.lang +++ b/htdocs/langs/en_US/main.lang @@ -798,7 +798,7 @@ Notes=Notes AddNewLine=Add new line AddFile=Add file FreeZone=Free-text product -FreeLineOfType=Free-text item, type: +FreeLineOfType=Free-text item, type CloneMainAttributes=Clone object with its main attributes ReGeneratePDF=Re-generate PDF PDFMerge=PDF Merge diff --git a/htdocs/theme/eldy/global.inc.php b/htdocs/theme/eldy/global.inc.php index 5db98f3af5cbb..e6b4e156a26e7 100644 --- a/htdocs/theme/eldy/global.inc.php +++ b/htdocs/theme/eldy/global.inc.php @@ -748,8 +748,12 @@ /* CSS for placeholder */ .placeholder { color: #ccc; } +select.placeholder { color: #ccc; } ::-webkit-input-placeholder { color: #ccc; } input:-moz-placeholder { color: #ccc; } +select.placeholder option:not(.opacitymediumbycolor):not(.opacitymedium) { + color: var(--colortext); +} input[name=price], input[name=weight], input[name=volume], input[name=surface], input[name=sizeheight], input[name=net_measure], select[name=incoterm_id] { margin-right: 6px; } fieldset { @@ -766,6 +770,7 @@ opacity: 0.7; } + .formconsumeproduce { background: #f3f3f3; padding: 20px 0px 0px 0px; diff --git a/htdocs/theme/md/style.css.php b/htdocs/theme/md/style.css.php index 5d31a78fe2236..b333f13f1cad1 100644 --- a/htdocs/theme/md/style.css.php +++ b/htdocs/theme/md/style.css.php @@ -933,11 +933,15 @@ /* CSS for placeholder */ .placeholder { color: #ccc; } +select.placeholder { color: #ccc; } ::-webkit-input-placeholder { color:#ccc; } :-moz-placeholder { color:#bbb; } /* firefox 18- */ ::-moz-placeholder { color:#bbb; } /* firefox 19+ */ :-ms-input-placeholder { color:#ccc; } /* ie */ input:-moz-placeholder { color:#ccc; } +select.placeholder option:not(.opacitymediumbycolor):not(.opacitymedium) { + color: var(--colortext); +} input[name=price], input[name=weight], input[name=volume], input[name=surface], input[name=sizeheight], input[name=net_measure], select[name=incoterm_id] { margin-right: 6px; } fieldset { @@ -4490,7 +4494,7 @@ padding-top: 4px; padding-bottom: 3px; } -.liste_titre_create td, .liste_titre_create th, .liste_titre_create .tagtd +.liste_titre_create td:not(.linecoldescription), .liste_titre_create th, .liste_titre_create .tagtd { border-top-width: 1px; border-top-color: var(--colortopbordertitle1); @@ -4512,6 +4516,10 @@ border-top-style: solid; } +td.linecoldescription { + padding: 6px 10px 6px 12px !important; /* t r b l */ +} + table.liste th, table.noborder th, table.noborder tr.liste_titre td, table.noborder tr.box_titre td { padding: 8px 8px 8px 10px; /* t r b l */ }