CVE-2021-25955: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting') (CWE-79)
value: In “Dolibarr ERP CRM”, WYSIWYG Editor module, v2.8.1 to v13.0.2 are affected by a stored XSS vulnerability that allows low privileged application users to store malicious scripts in the “Private Note” field at “/adherents/note.php?id=1” endpoint. These scripts are executed in a victim’s browser when they open the page containing the vulnerable field. In the worst case, the victim who inadvertently triggers the attack is a highly privileged administrator. The injected scripts can extract the Session ID, which can lead to full Account takeover of the admin and due to other vulnerability (Improper Access Control on Private notes) a low privileged user can update the private notes which could lead to privilege escalation.
CWE description: The product does not neutralize or incorrectly neutralizes user-controllable input before it is placed in output that is used as a web page that is served to other users.
CVSS v3 base score: 9
Published: 2021-08-15T21:15Z
Repository: https://github.com/Dolibarr/dolibarr
Language: PHP
Files changed: htdocs/core/lib/functions.lib.php, htdocs/main.inc.php, test/phpunit/SecurityTest.php
Commit date: 2021-06-29 18:17:27 +0200
Commit message: Enhance the sanitizing.
Diff: diff --git a/htdocs/core/lib/functions.lib.php b/htdocs/core/lib/functions.lib.php
index e3b288aff8e1..87a4b9660563 100644
--- a/htdocs/core/lib/functions.lib.php
+++ b/htdocs/core/lib/functions.lib.php
@@ -776,12 +776,16 @@ function checkVal($out = '', $check = 'alphanohtml', $filter = null, $options =
 			break;
 		case 'restricthtml':		// Recommended for most html textarea
 			do {
 				$oldstringtoclean = $out;
 
-				// We replace chars encoded with numeric HTML entities with real char (to avoid to have numeric entities used for obfuscation of injections)
-				$out = preg_replace_callback('/&#(x?[0-9][0-9a-f]+);/i', 'realCharForNumericEntities', $out);
+				// We replace chars from a/A to z/Z encoded with numeric HTML entities with the real char so we won't loose the chars at the next step.
+				// No need to use a loop here, this step is not to sanitize (this is done at next step, this is to try to save chars, even if they are
+				// using a non coventionnel way to be encoded, to not have them sanitized just after)
+				$out = preg_replace_callback('/&#(x?[0-9][0-9a-f]+;?)/i', 'realCharForNumericEntities', $out);
+
+				// Now we remove all remaining HTML entities staring with a number. We don't want such entities.
 				$out = preg_replace('/&#x?[0-9]+/i', '', $out);	// For example if we have j&#x61vascript with an entities without the ; to hide the 'a' of 'javascript'.
 
 				$out = dol_string_onlythesehtmltags($out, 0, 1, 1);
 
 				// We should also exclude non expected attributes
diff --git a/htdocs/main.inc.php b/htdocs/main.inc.php
index 933881902750..b66b5f5b211b 100644
--- a/htdocs/main.inc.php
+++ b/htdocs/main.inc.php
@@ -51,29 +51,30 @@ if (!empty($_SERVER['MAIN_SHOW_TUNING_INFO'])) {
 }
 
 
 /**
  * Return the real char for a numeric entities.
- * This function is required by testSqlAndScriptInject().
+ * WARNING: This function is required by testSqlAndScriptInject() and the GETPOST 'restricthtml'. Regex calling must be similar.
  *
  * @param	string		$matches			String of numeric entity
  * @return	string							New value
  */
 function realCharForNumericEntities($matches)
 {
 	$newstringnumentity = $matches[1];
+	//print  '$newstringnumentity='.$newstringnumentity;
 
 	if (preg_match('/^x/i', $newstringnumentity)) {
-		$newstringnumentity = hexdec(preg_replace('/^x/i', '', $newstringnumentity));
+		$newstringnumentity = hexdec(preg_replace('/;$/', '', preg_replace('/^x/i', '', $newstringnumentity)));
 	}
 
-	// The numeric value we don't want as entities
+	// The numeric value we don't want as entities because they encode ascii char, and why using html entities on ascii except for haking ?
 	if (($newstringnumentity >= 65 && $newstringnumentity <= 90) || ($newstringnumentity >= 97 && $newstringnumentity <= 122)) {
 		return chr((int) $newstringnumentity);
 	}
 
-	return '&#'.$matches[1];
+	return '&#'.$matches[1];	// Value will be unchanged because regex was /&#(  )/
 }
 
 /**
  * Security: WAF layer for SQL Injection and XSS Injection (scripts) protection (Filters on GET, POST, PHP_SELF).
  * Warning: Such a protection can't be enough. It is not reliable as it will always be possible to bypass this. Good protection can
diff --git a/test/phpunit/SecurityTest.php b/test/phpunit/SecurityTest.php
index 7e93f7673f22..08d4ec88703a 100644
--- a/test/phpunit/SecurityTest.php
+++ b/test/phpunit/SecurityTest.php
@@ -343,11 +343,11 @@ class SecurityTest extends PHPUnit\Framework\TestCase
 		$_GET["param3"]='"&#110;a/b#e(pr)qq-rr\cc';    // Same than param2 + " and &#110;
 		$_GET["param4"]='../dir';
 		$_GET["param5"]="a_1-b";
 		$_POST["param6"]="&quot;&gt;<svg o&#110;load='console.log(&quot;123&quot;)'&gt;";
 		$_POST["param6b"]='<<<../>../>../svg><<<../>../>../animate =alert(1)>abc';
-		$_GET["param7"]='"c:\this is a path~1\aaa&#110;" abc<bad>def</bad>';
+		$_GET["param7"]='"c:\this is a path~1\aaa&#110; &#x&#x31;&#x31;&#x30;;" abc<bad>def</bad>';
 		$_POST["param8a"]="Hacker<svg o&#110;load='console.log(&quot;123&quot;)'";	// html tag is not closed so it is not detected as html tag but is still harmfull
 		$_POST['param8b']='<img src=x onerror=alert(document.location) t=';		// this is html obfuscated by non closing tag
 		$_POST['param8c']='< with space after is ok';
 		$_POST['param8d']='<abc123 is html to clean';
 		$_POST['param8e']='<123abc is not html to clean';
@@ -477,24 +477,24 @@ class SecurityTest extends PHPUnit\Framework\TestCase
 		$result=GETPOST("param6", 'restricthtml');
 		print __METHOD__." result=".$result."\n";
 		$this->assertEquals('&quot;&gt;', $result);
 
 		$result=GETPOST("param7", 'restricthtml');
-		print __METHOD__." result=".$result."\n";
-		$this->assertEquals('"c:\this is a path~1\aaan" abcdef', $result);
+		print __METHOD__." result param7 = ".$result."\n";
+		$this->assertEquals('"c:\this is a path~1\aaan &#x;;;;" abcdef', $result);
 
 		$result=GETPOST("param12", 'restricthtml');
 		print __METHOD__." result=".$result."\n";
 		$this->assertEquals(trim($_POST["param12"]), $result, 'Test a string with DOCTYPE and restricthtml');
 
 		$result=GETPOST("param13", 'restricthtml');
 		print __METHOD__." result=".$result."\n";
-		$this->assertEquals('n n &gt; &lt; &quot; <a href=\"alert(document.domain)\">XSS</a>', $result, 'Test that HTML entities are decoded with restricthtml, but only for common alpha chars');
+		$this->assertEquals('n n &gt; &lt; &quot; <a href=\"alert(document.domain)\">XSS</a>', $result, 'Test 13 that HTML entities are decoded with restricthtml, but only for common alpha chars');
 
 		$result=GETPOST("param13b", 'restricthtml');
 		print __METHOD__." result=".$result."\n";
-		$this->assertEquals('n n &gt; &lt; &quot; <a href=\"jvascript:alert(document.domain)\">XSS</a>', $result, 'Test that HTML entities are decoded with restricthtml, but only for common alpha chars');
+		$this->assertEquals('n n &gt; &lt; &quot; <a href=\"alert(document.domain)\">XSS</a>', $result, 'Test 13b that HTML entities are decoded with restricthtml, but only for common alpha chars');
 
 		// Special test for GETPOST of backtopage, backtolist or backtourl parameter
 
 		$_POST["backtopage"]='//www.google.com';
 		$result=GETPOST("backtopage");

Vulnerable code: 				// We replace chars encoded with numeric HTML entities with real char (to avoid to have numeric entities used for obfuscation of injections)
				$out = preg_replace_callback('/&#(x?[0-9][0-9a-f]+);/i', 'realCharForNumericEntities', $out);
 * This function is required by testSqlAndScriptInject().
		$newstringnumentity = hexdec(preg_replace('/^x/i', '', $newstringnumentity));
	// The numeric value we don't want as entities
	return '&#'.$matches[1];
		$_GET["param7"]='"c:\this is a path~1\aaa&#110;" abc<bad>def</bad>';
		print __METHOD__." result=".$result."\n";
		$this->assertEquals('"c:\this is a path~1\aaan" abcdef', $result);
		$this->assertEquals('n n &gt; &lt; &quot; <a href=\"alert(document.domain)\">XSS</a>', $result, 'Test that HTML entities are decoded with restricthtml, but only for common alpha chars');
		$this->assertEquals('n n &gt; &lt; &quot; <a href=\"jvascript:alert(document.domain)\">XSS</a>', $result, 'Test that HTML entities are decoded with restricthtml, but only for common alpha chars');
Fixed code: 				// We replace chars from a/A to z/Z encoded with numeric HTML entities with the real char so we won't loose the chars at the next step.
				// No need to use a loop here, this step is not to sanitize (this is done at next step, this is to try to save chars, even if they are
				// using a non coventionnel way to be encoded, to not have them sanitized just after)
				$out = preg_replace_callback('/&#(x?[0-9][0-9a-f]+;?)/i', 'realCharForNumericEntities', $out);

				// Now we remove all remaining HTML entities staring with a number. We don't want such entities.
 * WARNING: This function is required by testSqlAndScriptInject() and the GETPOST 'restricthtml'. Regex calling must be similar.
	//print  '$newstringnumentity='.$newstringnumentity;
		$newstringnumentity = hexdec(preg_replace('/;$/', '', preg_replace('/^x/i', '', $newstringnumentity)));
	// The numeric value we don't want as entities because they encode ascii char, and why using html entities on ascii except for haking ?
	return '&#'.$matches[1];	// Value will be unchanged because regex was /&#(  )/
		$_GET["param7"]='"c:\this is a path~1\aaa&#110; &#x&#x31;&#x31;&#x30;;" abc<bad>def</bad>';
		print __METHOD__." result param7 = ".$result."\n";
		$this->assertEquals('"c:\this is a path~1\aaan &#x;;;;" abcdef', $result);
		$this->assertEquals('n n &gt; &lt; &quot; <a href=\"alert(document.domain)\">XSS</a>', $result, 'Test 13 that HTML entities are decoded with restricthtml, but only for common alpha chars');
		$this->assertEquals('n n &gt; &lt; &quot; <a href=\"alert(document.domain)\">XSS</a>', $result, 'Test 13b that HTML entities are decoded with restricthtml, but only for common alpha chars');