FIX huntr CWE-79

htdocs/core/lib/functions.lib.php

@@ -634,17 +634,17 @@ function GETPOST($paramname, $check = 'alphanohtml', $method = 0, $filter = null
 		$out = checkVal($out, $check, $filter, $options);
 	}
 
-	// Sanitizing for special parameters. There is no reason to allow the backtopage, backtolist or backtourl parameter to contains an external URL.
+	// Sanitizing for special parameters.
+	// Note: There is no reason to allow the backtopage, backtolist or backtourl parameter to contains an external URL.
 	if ($paramname == 'backtopage' || $paramname == 'backtolist' || $paramname == 'backtourl') {
-		$out = str_replace('\\', '/', $out);
+		$out = str_replace('\\', '/', $out);					// Can be before the loop because only 1 char is replaced. No risk to get it after other replacements.
-		$out = str_replace(array(':', ';', '@'), '', $out);
+		$out = str_replace(array(':', ';', '@'), '', $out);		// Can be before the loop because only 1 char is replaced. No risk to get it after other replacements.
-
 		do {
 			$oldstringtoclean = $out;
 			$out = str_ireplace(array('javascript', 'vbscript', '&colon', '&#'), '', $out);
 		} while ($oldstringtoclean != $out);
 
-		$out = preg_replace(array('/^[a-z]*\/\/+/i'), '', $out);
+		$out = preg_replace(array('/^[a-z]*\/\/+/i'), '', $out);	// We remove schema*// to remove external URL
 	}
 
 	// Code for search criteria persistence.
@@ -684,7 +684,7 @@ function GETPOSTINT($paramname, $method = 0, $filter = null, $options = null, $n
 }
 
 /**
- *  Return a value after checking on a rule.
+ *  Return a value after checking on a rule. A sanitization may also have been done.
  *
  *  @param  string  $out	     Value to check/clear.
  *  @param  string  $check	     Type of check/sanitizing
@@ -777,6 +777,11 @@ function checkVal($out = '', $check = 'alphanohtml', $filter = null, $options =
 		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);
+				$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
@@ -797,7 +802,6 @@ function checkVal($out = '', $check = 'alphanohtml', $filter = null, $options =
 }
 
 
-
 if (!function_exists('dol_getprefix')) {
 	/**
 	 *  Return a prefix to use for this Dolibarr instance, for session/cookie names or email id.

htdocs/main.inc.php

@@ -50,9 +50,33 @@ if (!empty($_SERVER['MAIN_SHOW_TUNING_INFO'])) {
 	}
 }
 
+
+/**
+ * Return the real char for a numeric entities.
+ * This function is required by testSqlAndScriptInject().
+ *
+ * @param	string		$matches			String of numeric entity
+ * @return	string							New value
+ */
+function realCharForNumericEntities($matches)
+{
+	$newstringnumentity = $matches[1];
+
+	if (preg_match('/^x/i', $newstringnumentity)) {
+		$newstringnumentity = hexdec(preg_replace('/^x/i', '', $newstringnumentity));
+	}
+
+	// The numeric value we don't want as entities
+	if (($newstringnumentity >= 65 && $newstringnumentity <= 90) || ($newstringnumentity >= 97 && $newstringnumentity <= 122)) {
+		return chr((int) $newstringnumentity);
+	}
+
+	return '&#'.$matches[1];
+}
+
 /**
  * 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 alwyas be possible to bypass this. Good protection can
+ * Warning: Such a protection can't be enough. It is not reliable as it will always be possible to bypass this. Good protection can
  * only be guaranted by escaping data during output.
  *
  * @param		string		$val		Value brut found int $_GET, $_POST or PHP_SELF
@@ -61,7 +85,7 @@ if (!empty($_SERVER['MAIN_SHOW_TUNING_INFO'])) {
  */
 function testSqlAndScriptInject($val, $type)
 {
-	// Decode string first bcause a lot of things are obfuscated by encoding or multiple encoding.
+	// Decode string first because a lot of things are obfuscated by encoding or multiple encoding.
 	// So <svg o&#110;load='console.log(&quot;123&quot;)' become <svg onload='console.log(&quot;123&quot;)'
 	// So "&colon;&apos;" become ":'" (due to ENT_HTML5)
 	// Loop to decode until no more thing to decode.
@@ -69,6 +93,7 @@ function testSqlAndScriptInject($val, $type)
 	do {
 		$oldval = $val;
 		$val = html_entity_decode($val, ENT_QUOTES | ENT_HTML5);
+		$val = preg_replace_callback('/&#(x?[0-9][0-9a-f]+)/i', 'realCharForNumericEntities', $val);	// Sometimes we have entities without the ; at end so html_entity_decode does not work but entities is still interpreted by browser.
 	} while ($oldval != $val);
 	//print "after  decoding $val\n";
 

test/phpunit/SecurityTest.php

@@ -198,20 +198,29 @@ class SecurityTest extends PHPUnit\Framework\TestCase
 		$result=testSqlAndScriptInject($test, 0);
 		$this->assertEquals($expectedresult, $result, 'Error on testSqlAndScriptInject expected 0b');
 
-		// Should detect XSS
+
+		// Should detect attack
 		$expectedresult=1;
 
 		$_SERVER["PHP_SELF"]='/DIR WITH SPACE/htdocs/admin/index.php/<svg>';
 		$result=testSqlAndScriptInject($_SERVER["PHP_SELF"], 2);
 		$this->assertGreaterThanOrEqual($expectedresult, $result, 'Error on testSqlAndScriptInject for PHP_SELF that should detect XSS');
 
+		$test = 'j&#x61;vascript:';
+		$result=testSqlAndScriptInject($test, 0);
+		$this->assertEquals($expectedresult, $result, 'Error on testSqlAndScriptInject for javascript1. Should find an attack and did not.');
+
+		$test = 'j&#x61vascript:';
+		$result=testSqlAndScriptInject($test, 0);
+		$this->assertEquals($expectedresult, $result, 'Error on testSqlAndScriptInject for javascript2. Should find an attack and did not.');
+
 		$test = 'javascript&colon&#x3B;alert(1)';
 		$result=testSqlAndScriptInject($test, 0);
-		$this->assertEquals($expectedresult, $result, 'Error on testSqlAndScriptInject expected 1a');
+		$this->assertEquals($expectedresult, $result, 'Error on testSqlAndScriptInject for javascript2');
 
 		$test="<img src='1.jpg' onerror =javascript:alert('XSS')>";
 		$result=testSqlAndScriptInject($test, 0);
-		$this->assertGreaterThanOrEqual($expectedresult, $result, 'Error on testSqlAndScriptInject aaa');
+		$this->assertGreaterThanOrEqual($expectedresult, $result, 'Error on testSqlAndScriptInject aaa1');
 
 		$test="<img src='1.jpg' onerror =javascript:alert('XSS')>";
 		$result=testSqlAndScriptInject($test, 2);
@@ -328,9 +337,12 @@ class SecurityTest extends PHPUnit\Framework\TestCase
 		$_POST["param10"]='is_object($object) ? ($object->id < 10 ? round($object->id / 2, 2) : (2 * $user->id) * (int) substr($mysoc->zip, 1, 2)) : \'<abc>objnotdefined\'';
 		$_POST["param11"]=' Name <email@email.com> ';
 		$_POST["param12"]='<!DOCTYPE html><html>aaa</html>';
+		$_POST["param13"]='&#110; &#x6E; &gt; &lt; &quot; <a href=\"j&#x61;vascript:alert(document.domain)\">XSS</a>';
+		$_POST["param13b"]='&#110; &#x6E; &gt; &lt; &quot; <a href=\"j&#x61vascript:alert(document.domain)\">XSS</a>';
 		//$_POST["param13"]='javascript%26colon%26%23x3B%3Balert(1)';
 		//$_POST["param14"]='javascripT&javascript#x3a alert(1)';
 
+
 		$result=GETPOST('id', 'int');              // Must return nothing
 		print __METHOD__." result=".$result."\n";
 		$this->assertEquals($result, '');
@@ -343,7 +355,7 @@ class SecurityTest extends PHPUnit\Framework\TestCase
 		print __METHOD__." result=".$result."\n";
 		$this->assertEquals($result, 333, 'Test on param1 with 3rd param = 2');
 
-		// Test alpha
+		// Test with alpha
 
 		$result=GETPOST("param2", 'alpha');
 		print __METHOD__." result=".$result."\n";
@@ -357,7 +369,7 @@ class SecurityTest extends PHPUnit\Framework\TestCase
 		print __METHOD__." result=".$result."\n";
 		$this->assertEquals($result, 'dir');
 
-		// Test aZ09
+		// Test with aZ09
 
 		$result=GETPOST("param1", 'aZ09');
 		print __METHOD__." result=".$result."\n";
@@ -379,25 +391,22 @@ class SecurityTest extends PHPUnit\Framework\TestCase
 		print __METHOD__." result=".$result."\n";
 		$this->assertEquals($_GET["param5"], $result);
 
-		$result=GETPOST("param6", 'alpha');
+		// Test with nohtml
-		print __METHOD__." result=".$result."\n";
-		$this->assertEquals('>', $result);
 
 		$result=GETPOST("param6", 'nohtml');
 		print __METHOD__." result=".$result."\n";
 		$this->assertEquals('">', $result);
 
-		$result=GETPOST("param6b");
+		// Test with alpha = alphanohtml. We must convert the html entities like &#110; and disable all entities
+
+		$result=GETPOST("param6", 'alphanohtml');
+		print __METHOD__." result=".$result."\n";
+		$this->assertEquals('>', $result);
+
+		$result=GETPOST("param6b", 'alphanohtml');
 		print __METHOD__." result=".$result."\n";
 		$this->assertEquals('abc', $result);
 
-		// With restricthtml we must remove html open/close tag and content but not htmlentities like &#110;
-
-		$result=GETPOST("param7", 'restricthtml');
-		print __METHOD__." result=".$result."\n";
-		$this->assertEquals('"c:\this is a path~1\aaa&#110;" abcdef', $result);
-
-		// With alphanohtml, we must convert the html entities like &#110; and disable all entities
 		$result=GETPOST("param8a", 'alphanohtml');
 		print __METHOD__." result=".$result."\n";
 		$this->assertEquals("Hackersvg onload='console.log(123)'", $result);
@@ -434,24 +443,39 @@ class SecurityTest extends PHPUnit\Framework\TestCase
 		print __METHOD__." result=".$result."\n";
 		$this->assertEquals("Name", $result, 'Test an email string with alphanohtml');
 
+		$result=GETPOST("param13", 'alphanohtml');
+		print __METHOD__." result=".$result."\n";
+		$this->assertEquals('n n > <  XSS', $result, 'Test that html entities are decoded with alpha');
+
+		// Test with alphawithlgt
+
 		$result=GETPOST("param11", 'alphawithlgt');
 		print __METHOD__." result=".$result."\n";
 		$this->assertEquals(trim($_POST["param11"]), $result, 'Test an email string with alphawithlgt');
 
+		// Test with restricthtml we must remove html open/close tag and content but not htmlentities (we can decode html entities for ascii chars like &#110;)
+
+		$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);
+
 		$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", 'alphanohtml');
+		$result=GETPOST("param13", 'restricthtml');
 		print __METHOD__." result=".$result."\n";
-		$this->assertEquals(trim($_POST["param13"]), $result, 'Test a string and alphanohtml');
+		$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');
 
-		$result=GETPOST("param14", 'alphanohtml');
+		$result=GETPOST("param13b", 'restricthtml');
 		print __METHOD__." result=".$result."\n";
-		$this->assertEquals(trim($_POST["param14"]), $result, 'Test a string and alphanohtml');
+		$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');
-		*/
 
-		// Special test for GETPOST of backtopage or backtolist parameter
+		// Special test for GETPOST of backtopage, backtolist or backtourl parameter
 
 		$_POST["backtopage"]='//www.google.com';
 		$result=GETPOST("backtopage");