From 0dfa7bdbcc93704333576aa5e634301be74976d0 Mon Sep 17 00:00:00 2001 From: Laurent Destailleur Date: Tue, 6 Jul 2021 00:47:43 +0200 Subject: [PATCH 1/3] Add option MAIN_RESTRICTHTML_ONLY_VALID_HTML --- htdocs/core/lib/functions.lib.php | 17 ++++++++++++++--- test/phpunit/SecurityTest.php | 26 +++++++++++++++++++++++++- 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/htdocs/core/lib/functions.lib.php b/htdocs/core/lib/functions.lib.php index 663158e1ef3..c622ebf1aba 100644 --- a/htdocs/core/lib/functions.lib.php +++ b/htdocs/core/lib/functions.lib.php @@ -778,6 +778,16 @@ function checkVal($out = '', $check = 'alphanohtml', $filter = null, $options = do { $oldstringtoclean = $out; + if (!empty($conf->global->MAIN_RESTRICTHTML_ONLY_VALID_HTML)) { + $dom = new DOMDocument; + try { + $dom->loadHTML($out, LIBXML_ERR_NONE|LIBXML_HTML_NOIMPLIED|LIBXML_HTML_NODEFDTD|LIBXML_NONET|LIBXML_NOWARNING|LIBXML_NOXMLDECL); + } catch(Exception $e) { + return 'InvalidHTMLString'; + } + $out = $dom->saveHTML(); + } + // Ckeditor use the numeric entitic for apostrophe so we force it to text entity (all other special chars are correctly // encoded using text entities). This is a fix for CKeditor. $out = preg_replace('/'/i', ''', $out); @@ -794,7 +804,8 @@ function checkVal($out = '', $check = 'alphanohtml', $filter = null, $options = // We should also exclude non expected attributes if (!empty($conf->global->MAIN_RESTRICTHTML_REMOVE_ALSO_BAD_ATTRIBUTES)) { - $out = dol_string_onlythesehtmlattributes($out); + // Warning, the function may add a LF so we are forced to trim to compare with old $out without having always a difference and an infinit loop. + $out = trim(dol_string_onlythesehtmlattributes($out)); } } while ($oldstringtoclean != $out); break; @@ -6311,7 +6322,7 @@ function dol_string_onlythesehtmltags($stringtoclean, $cleanalsosomestyles = 1, $stringtoclean = preg_replace('/:|�+58|:/i', '', $stringtoclean); // refused string ':' encoded (no reason to have a : encoded like this) to disable 'javascript:...' $stringtoclean = preg_replace('/javascript\s*:/i', '', $stringtoclean); - $temp = strip_tags($stringtoclean, $allowed_tags_string); + $temp = strip_tags($stringtoclean, $allowed_tags_string); // Warning: This remove also undesired changing string obfuscated with that pass injection detection into harmfull string if ($cleanalsosomestyles) { // Clean for remaining html tags $temp = preg_replace('/position\s*:\s*(absolute|fixed)\s*!\s*important/i', '', $temp); // Note: If hacker try to introduce css comment into string to bypass this regex, the string must also be encoded by the dol_htmlentitiesbr during output so it become harmless @@ -6361,8 +6372,8 @@ function dol_string_onlythesehtmlattributes($stringtoclean, $allowed_attributes } $return = $dom->saveHTML(); - //$return = 'aaaa

bb

ssdd

'."\n

aaa

aa

bb

"; + $return = preg_replace('/^/', '', $return); $return = preg_replace('/<\/body><\/html>$/', '', $return); return $return; diff --git a/test/phpunit/SecurityTest.php b/test/phpunit/SecurityTest.php index 16cc8438c91..8e99a03021e 100644 --- a/test/phpunit/SecurityTest.php +++ b/test/phpunit/SecurityTest.php @@ -363,6 +363,7 @@ class SecurityTest extends PHPUnit\Framework\TestCase $_POST["param13"]='n n > < " XSS'; $_POST["param13b"]='n n > < " XSS'; $_POST["param14"]="Text with ' encoded with the numeric html entity converted into text entity ' (like when submited by CKEditor)"; + $_POST["param15"]=" src=>0xbeefed"; //$_POST["param13"]='javascript%26colon%26%23x3B%3Balert(1)'; //$_POST["param14"]='javascripT&javascript#x3a alert(1)'; @@ -480,7 +481,7 @@ class SecurityTest extends PHPUnit\Framework\TestCase // Test with restricthtml we must remove html open/close tag and content but not htmlentities (we can decode html entities for ascii chars like n) $result=GETPOST("param6", 'restricthtml'); - print __METHOD__." result=".$result."\n"; + print __METHOD__." result param6=".$result."\n"; $this->assertEquals('">', $result); $result=GETPOST("param7", 'restricthtml'); @@ -503,6 +504,29 @@ class SecurityTest extends PHPUnit\Framework\TestCase print __METHOD__." result=".$result."\n"; $this->assertEquals("Text with ' encoded with the numeric html entity converted into text entity ' (like when submited by CKEditor)", $result, 'Test 14'); + $result=GETPOST("param15", 'restricthtml'); // src=>0xbeefed + print __METHOD__." result=".$result."\n"; + $this->assertEquals("0xbeefed", $result, 'Test 15a'); // The GETPOST return a harmull string + + // Test with restricthtml + MAIN_RESTRICTHTML_REMOVE_ALSO_BAD_ATTRIBUTES to test disabling of bad atrributes + $conf->global->MAIN_RESTRICTHTML_ONLY_VALID_HTML = 1; + + $result=GETPOST("param15", 'restricthtml'); + print __METHOD__." result=".$result."\n"; + $this->assertEquals('InvalidHTMLString', $result, 'Test 15b'); + + unset($conf->global->MAIN_RESTRICTHTML_ONLY_VALID_HTML); + + // Test with restricthtml + MAIN_RESTRICTHTML_REMOVE_ALSO_BAD_ATTRIBUTES to test disabling of bad atrributes + $conf->global->MAIN_RESTRICTHTML_REMOVE_ALSO_BAD_ATTRIBUTES = 1; + + $result=GETPOST("param15", 'restricthtml'); + print __METHOD__." result=".$result."\n"; + $this->assertEquals('0xbeefed', $result, 'Test 15b'); + + unset($conf->global->MAIN_RESTRICTHTML_REMOVE_ALSO_BAD_ATTRIBUTES); + + // Special test for GETPOST of backtopage, backtolist or backtourl parameter $_POST["backtopage"]='//www.google.com'; From 3dff7e29cc28140db07a67d284e5899b2fac8772 Mon Sep 17 00:00:00 2001 From: Laurent Destailleur Date: Tue, 6 Jul 2021 01:44:05 +0200 Subject: [PATCH 2/3] Fix #yogosha6567 --- htdocs/comm/mailing/card.php | 3 +-- htdocs/comm/mailing/class/mailing.class.php | 12 ++++++++++++ htdocs/core/lib/functions.lib.php | 7 +++++-- test/phpunit/SecurityTest.php | 4 ++++ 4 files changed, 22 insertions(+), 4 deletions(-) diff --git a/htdocs/comm/mailing/card.php b/htdocs/comm/mailing/card.php index d801b4429c7..e03d13c29e8 100644 --- a/htdocs/comm/mailing/card.php +++ b/htdocs/comm/mailing/card.php @@ -577,7 +577,6 @@ if (empty($reshook)) { if (!$isupload) { $mesgs = array(); - $object->sujet = (string) GETPOST("sujet"); $object->body = (string) GETPOST("bodyemail", 'restricthtml'); $object->bgcolor = (string) GETPOST("bgcolor"); @@ -744,7 +743,7 @@ if ($action == 'create') { print '
'; // wysiwyg editor require_once DOL_DOCUMENT_ROOT.'/core/class/doleditor.class.php'; - $doleditor = new DolEditor('bodyemail', GETPOST('bodyemail', 'restricthtml'), '', 600, 'dolibarr_mailings', '', true, true, $conf->global->FCKEDITOR_ENABLE_MAILING, 20, '90%'); + $doleditor = new DolEditor('bodyemail', GETPOST('bodyemail', 'restricthtmlallowunvalid'), '', 600, 'dolibarr_mailings', '', true, true, $conf->global->FCKEDITOR_ENABLE_MAILING, 20, '90%'); $doleditor->Create(); print '
'; diff --git a/htdocs/comm/mailing/class/mailing.class.php b/htdocs/comm/mailing/class/mailing.class.php index 04d2f7c17d8..d281fcb1a3a 100644 --- a/htdocs/comm/mailing/class/mailing.class.php +++ b/htdocs/comm/mailing/class/mailing.class.php @@ -208,6 +208,12 @@ class Mailing extends CommonObject { global $conf, $langs; + // Check properties + if ($this->body === 'InvalidHTMLString') { + $this->error = 'InvalidHTMLString'; + return -1; + } + $this->db->begin(); $this->title = trim($this->title); @@ -257,6 +263,12 @@ class Mailing extends CommonObject */ public function update($user) { + // Check properties + if ($this->body === 'InvalidHTMLString') { + $this->error = 'InvalidHTMLString'; + return -1; + } + $sql = "UPDATE ".MAIN_DB_PREFIX."mailing "; $sql .= " SET titre = '".$this->db->escape($this->title)."'"; $sql .= ", sujet = '".$this->db->escape($this->sujet)."'"; diff --git a/htdocs/core/lib/functions.lib.php b/htdocs/core/lib/functions.lib.php index c622ebf1aba..93a33e5bd5f 100644 --- a/htdocs/core/lib/functions.lib.php +++ b/htdocs/core/lib/functions.lib.php @@ -775,18 +775,21 @@ function checkVal($out = '', $check = 'alphanohtml', $filter = null, $options = } break; case 'restricthtml': // Recommended for most html textarea + case 'restricthtmlallowunvalid': do { $oldstringtoclean = $out; - if (!empty($conf->global->MAIN_RESTRICTHTML_ONLY_VALID_HTML)) { - $dom = new DOMDocument; + if (!empty($conf->global->MAIN_RESTRICTHTML_ONLY_VALID_HTML) && $check != 'restricthtmlallowunvalid') { try { + $dom = new DOMDocument; $dom->loadHTML($out, LIBXML_ERR_NONE|LIBXML_HTML_NOIMPLIED|LIBXML_HTML_NODEFDTD|LIBXML_NONET|LIBXML_NOWARNING|LIBXML_NOXMLDECL); } catch(Exception $e) { + //print $e->getMessage(); return 'InvalidHTMLString'; } $out = $dom->saveHTML(); } + //var_dump($oldstringtoclean);var_dump($out); // Ckeditor use the numeric entitic for apostrophe so we force it to text entity (all other special chars are correctly // encoded using text entities). This is a fix for CKeditor. diff --git a/test/phpunit/SecurityTest.php b/test/phpunit/SecurityTest.php index 8e99a03021e..44b391148f5 100644 --- a/test/phpunit/SecurityTest.php +++ b/test/phpunit/SecurityTest.php @@ -340,6 +340,10 @@ class SecurityTest extends PHPUnit\Framework\TestCase $langs=$this->savlangs; $db=$this->savdb; + // Force default mode + $conf->global->MAIN_RESTRICTHTML_ONLY_VALID_HTML = 0; + $conf->global->MAIN_RESTRICTHTML_REMOVE_ALSO_BAD_ATTRIBUTES = 0; + $_COOKIE["id"]=111; $_GET["param1"]="222"; $_POST["param1"]="333"; From 6e24ee48d4564991dfef8f31ac7ddd877e84af64 Mon Sep 17 00:00:00 2001 From: Laurent Destailleur Date: Tue, 6 Jul 2021 02:07:58 +0200 Subject: [PATCH 3/3] Debug security page --- htdocs/admin/system/security.php | 107 ++++++++++++++++++------------- htdocs/langs/en_US/admin.lang | 3 +- 2 files changed, 66 insertions(+), 44 deletions(-) diff --git a/htdocs/admin/system/security.php b/htdocs/admin/system/security.php index 0b2f2678521..c4140cfe416 100644 --- a/htdocs/admin/system/security.php +++ b/htdocs/admin/system/security.php @@ -251,7 +251,7 @@ print '
'; if (empty($conf->global->SECURITY_DISABLE_TEST_ON_OBFUSCATED_CONF)) { print '$dolibarr_main_db_pass: '; if (!empty($dolibarr_main_db_pass) && empty($dolibarr_main_db_encrypted_pass)) { - print img_picto('', 'warning').' '.$langs->trans("DatabasePasswordNotObfuscated").'   ('.$langs->trans("Recommanded").': '.$langs->trans("SetOptionTo", $langs->transnoentitiesnoconv("MainDbPasswordFileConfEncrypted"), yn(1)).')'; + print img_picto('', 'warning').' '.$langs->trans("DatabasePasswordNotObfuscated").'   ('.$langs->trans("Recommended").': '.$langs->trans("SetOptionTo", $langs->transnoentitiesnoconv("MainDbPasswordFileConfEncrypted"), yn(1)).')'; //print ' ('.$langs->trans("RecommendedValueIs", $langs->transnoentitiesnoconv("IPsOfUsers")).')'; } else { print img_picto('', 'tick').' '.$langs->trans("DatabasePasswordObfuscated"); @@ -267,49 +267,14 @@ if (empty($conf->global->SECURITY_DISABLE_TEST_ON_OBFUSCATED_CONF)) { print '
'; print '
'; print '
'; -print load_fiche_titre($langs->trans("Menu").' '.$langs->trans("SecuritySetup").' + '.$langs->trans("OtherSetup"), '', 'folder'); -//print ''.$langs->trans("PasswordEncryption").': '; -print 'MAIN_SECURITY_HASH_ALGO = '.(empty($conf->global->MAIN_SECURITY_HASH_ALGO) ? ''.$langs->trans("Undefined").'' : $conf->global->MAIN_SECURITY_HASH_ALGO)."   "; -if (empty($conf->global->MAIN_SECURITY_HASH_ALGO)) { - print '     If unset: \'md5\''; -} -if ($conf->global->MAIN_SECURITY_HASH_ALGO != 'password_hash') { - print '
MAIN_SECURITY_SALT = '.(empty($conf->global->MAIN_SECURITY_SALT) ? ''.$langs->trans("Undefined").'' : $conf->global->MAIN_SECURITY_SALT).'
'; -} else { - print '('.$langs->trans("Recommanded").': password_hash)'; - print '
'; -} -if ($conf->global->MAIN_SECURITY_HASH_ALGO != 'password_hash') { - print '
The recommanded value for MAIN_SECURITY_HASH_ALGO is now \'password_hash\' but setting it now will make ALL existing passwords of all users not valid, so update is not possible.
'; - print 'If you really want to switch, you must:
'; - print '- Go on home - setup - other and add constant MAIN_SECURITY_HASH_ALGO to value \'password_hash\'
'; - print '- In same session, WITHOUT LOGGING OUT, go into your admin user record and set a new password
'; - print '- You can now logout and login with this new password. You must now reset password of all other users.
'; - print '

'; -} +print load_fiche_titre($langs->trans("Menu").' '.$langs->trans("SecuritySetup"), '', 'folder'); + + +print ''.$langs->trans("UseCaptchaCode").': '; +print empty($conf->global->MAIN_SECURITY_ENABLECAPTCHA) ? '' : img_picto('', 'tick').' '; +print yn(empty($conf->global->MAIN_SECURITY_ENABLECAPTCHA) ? 0 : 1); print '
'; - - -print 'MAIN_SECURITY_ANTI_SSRF_SERVER_IP = '.(empty($conf->global->MAIN_SECURITY_ANTI_SSRF_SERVER_IP) ? ''.$langs->trans("Undefined").'   ('.$langs->trans("Example").': static-ips-of-server - '.$langs->trans("Note").': common loopback ip like 127.*.*.*, [::1] are already added)' : $conf->global->MAIN_SECURITY_ANTI_SSRF_SERVER_IP)."
"; -print '
'; - -print 'MAIN_ALLOW_SVG_FILES_AS_IMAGES = '.(empty($conf->global->MAIN_ALLOW_SVG_FILES_AS_IMAGES) ? '0   ('.$langs->trans("Recommanded").': 0)' : $conf->global->MAIN_ALLOW_SVG_FILES_AS_IMAGES)."
"; -print '
'; - -print 'MAIN_EXEC_USE_POPEN = '; -if (empty($conf->global->MAIN_EXEC_USE_POPEN)) { - print ''.$langs->trans("Undefined").''; -} else { - print $conf->global->MAIN_EXEC_USE_POPEN; -} -if ($execmethod == 1) { - print '   ("exec" PHP method will be used for shell commands)'; -} -if ($execmethod == 2) { - print '   ("popen" PHP method will be used for shell commands)'; -} -print "
"; print '
'; @@ -354,6 +319,62 @@ if (empty($out)) { } print '
'; +print '
'; +print '
'; +print '
'; + + +print load_fiche_titre($langs->trans("OtherSetup").' ('.$langs->trans("Experimental").')', '', 'folder'); + + +//print ''.$langs->trans("PasswordEncryption").': '; +print 'MAIN_SECURITY_HASH_ALGO = '.(empty($conf->global->MAIN_SECURITY_HASH_ALGO) ? ''.$langs->trans("Undefined").'' : $conf->global->MAIN_SECURITY_HASH_ALGO)."   "; +if (empty($conf->global->MAIN_SECURITY_HASH_ALGO)) { + print '     If unset: \'md5\''; +} +if ($conf->global->MAIN_SECURITY_HASH_ALGO != 'password_hash') { + print '
MAIN_SECURITY_SALT = '.(empty($conf->global->MAIN_SECURITY_SALT) ? ''.$langs->trans("Undefined").'' : $conf->global->MAIN_SECURITY_SALT).'
'; +} else { + print '('.$langs->trans("Recommanded").': password_hash)'; + print '
'; +} +if ($conf->global->MAIN_SECURITY_HASH_ALGO != 'password_hash') { + print '
The recommanded value for MAIN_SECURITY_HASH_ALGO is now \'password_hash\' but setting it now will make ALL existing passwords of all users not valid, so update is not possible.
'; + print 'If you really want to switch, you must:
'; + print '- Go on home - setup - other and add constant MAIN_SECURITY_HASH_ALGO to value \'password_hash\'
'; + print '- In same session, WITHOUT LOGGING OUT, go into your admin user record and set a new password
'; + print '- You can now logout and login with this new password. You must now reset password of all other users.
'; + print '

'; +} +print '
'; + +print 'MAIN_SECURITY_ANTI_SSRF_SERVER_IP = '.(empty($conf->global->MAIN_SECURITY_ANTI_SSRF_SERVER_IP) ? ''.$langs->trans("Undefined").'   ('.$langs->trans("Example").': static-ips-of-server - '.$langs->trans("Note").': common loopback ip like 127.*.*.*, [::1] are already added)' : $conf->global->MAIN_SECURITY_ANTI_SSRF_SERVER_IP)."
"; +print '
'; + +print 'MAIN_ALLOW_SVG_FILES_AS_IMAGES = '.(empty($conf->global->MAIN_ALLOW_SVG_FILES_AS_IMAGES) ? '0   ('.$langs->trans("Recommanded").': 0)' : $conf->global->MAIN_ALLOW_SVG_FILES_AS_IMAGES)."
"; +print '
'; + +print 'MAIN_RESTRICTHTML_ONLY_VALID_HTML = '.(empty($conf->global->MAIN_RESTRICTHTML_ONLY_VALID_HTML) ? ''.$langs->trans("Undefined").'   ('.$langs->trans("Recommanded").': 1)' : '')."
"; +print '
'; + +print 'MAIN_RESTRICTHTML_REMOVE_ALSO_BAD_ATTRIBUTES = '.(empty($conf->global->MAIN_RESTRICTHTML_REMOVE_ALSO_BAD_ATTRIBUTES) ? ''.$langs->trans("Undefined").'   ('.$langs->trans("Recommanded").': 1)' : '')."
"; +print '
'; + +print 'MAIN_EXEC_USE_POPEN = '; +if (empty($conf->global->MAIN_EXEC_USE_POPEN)) { + print ''.$langs->trans("Undefined").''; +} else { + print $conf->global->MAIN_EXEC_USE_POPEN; +} +if ($execmethod == 1) { + print '   ("exec" PHP method will be used for shell commands)'; +} +if ($execmethod == 2) { + print '   ("popen" PHP method will be used for shell commands)'; +} +print "
"; +print '
'; + // Modules/Applications @@ -405,7 +426,7 @@ if (empty($conf->api->enabled) && empty($conf->webservices->enabled)) { print '
'; } if (!empty($conf->api->enabled)) { - print 'API_ENDPOINT_RULES = '.(empty($conf->global->API_ENDPOINT_RULES) ? ''.$langs->trans("Undefined").'' : $conf->global->API_ENDPOINT_RULES)."
\n"; + print 'API_ENDPOINT_RULES = '.(empty($conf->global->API_ENDPOINT_RULES) ? ''.$langs->trans("Undefined").'   ('.$langs->trans("Example").': endpoint1:1,endpoint2:1,...)' : $conf->global->API_ENDPOINT_RULES)."
\n"; print '
'; } } diff --git a/htdocs/langs/en_US/admin.lang b/htdocs/langs/en_US/admin.lang index 0e357e27577..0c15369a0a9 100644 --- a/htdocs/langs/en_US/admin.lang +++ b/htdocs/langs/en_US/admin.lang @@ -2134,7 +2134,8 @@ IfCLINotRequiredYouShouldDisablePHPFunctions=Except if you need to run system co PHPFunctionsRequiredForCLI=For shell purpose (like scheduled job backup or running an anitivurs program), you must keep PHP functions NoWritableFilesFoundIntoRootDir=No writable files or directories of the common programs were found into your root directory (Good) RecommendedValueIs=Recommended: %s -NotRecommended=Not recommanded +Recommended=Recommended +NotRecommended=Not recommended ARestrictedPath=A restricted path CheckForModuleUpdate=Check for external modules updates CheckForModuleUpdateHelp=This action will connect to editors of external modules to check if a new version is available.