Better testSqlAndScriptInject (deal htmlentities encoded signatures)

More phpunits on GETPOST

htdocs/main.inc.php

@@ -51,12 +51,15 @@ if (!empty($_SERVER['MAIN_SHOW_TUNING_INFO']))
 /**
  * Security: SQL Injection and XSS Injection (scripts) protection (Filters on GET, POST, PHP_SELF).
  *
- * @param		string		$val		Value
+ * @param		string		$val		Value brut found int $_GET, $_POST or PHP_SELF
  * @param		string		$type		1=GET, 0=POST, 2=PHP_SELF, 3=GET without sql reserved keywords (the less tolerant test)
  * @return		int						>0 if there is an injection, 0 if none
  */
 function testSqlAndScriptInject($val, $type)
 {
+	$val=html_entity_decode($val, ENT_QUOTES);		// So <svg o&#110;load='console.log(&quot;123&quot;)' become <svg onload='console.log(&quot;123&quot;)'
+	// TODO loop to decode until no more thing to decode ?
+
 	$inj = 0;
 	// For SQL Injection (only GET are used to be included into bad escaped SQL requests)
 	if ($type == 1 || $type == 3)
@@ -80,7 +83,7 @@ function testSqlAndScriptInject($val, $type)
 		$inj += preg_match('/union.+select/i', $val);
 		$inj += preg_match('/(\.\.%2f)+/i', $val);
 	}
-	// For XSS Injection done by closing textarea to exucute content into a textarea field
+	// For XSS Injection done by closing textarea to execute content into a textarea field
 	$inj += preg_match('/<\/textarea/i', $val);
 	// For XSS Injection done by adding javascript with script
 	// This is all cases a browser consider text is javascript:

htdocs/user/card.php

@@ -226,7 +226,7 @@ if (empty($reshook)) {
 			}
 
 			$object->email = preg_replace('/\s+/', '', GETPOST("email", 'alphanohtml'));
-			$object->job = GETPOST("job", 'nohtml');
+			$object->job = GETPOST("job", 'alphanohtml');
 			$object->signature = GETPOST("signature", 'restricthtml');
 			$object->accountancy_code = GETPOST("accountancy_code", 'alphanohtml');
 			$object->note = GETPOST("note", 'restricthtml');
@@ -388,7 +388,7 @@ if (empty($reshook)) {
 					}
 				}
 				$object->email = preg_replace('/\s+/', '', GETPOST("email", 'alphanohtml'));
-				$object->job = GETPOST("job", 'nohtml');
+				$object->job = GETPOST("job", 'alphanohtml');
 				$object->signature = GETPOST("signature", 'restricthtml');
 				$object->accountancy_code = GETPOST("accountancy_code", 'alphanohtml');
 				$object->openid = GETPOST("openid", 'alphanohtml');
@@ -1183,7 +1183,7 @@ if ($action == 'create' || $action == 'adduserldap')
 	// Position/Job
 	print '<tr><td class="titlefieldcreate">'.$langs->trans("PostOrFunction").'</td>';
 	print '<td>';
-	print '<input class="maxwidth200" type="text" name="job" value="'.dol_escape_htmltag(GETPOST('job', 'nohtml')).'">';
+	print '<input class="maxwidth200" type="text" name="job" value="'.dol_escape_htmltag(GETPOST('job', 'alphanohtml')).'">';
 	print '</td></tr>';
 
 	if ((!empty($conf->salaries->enabled) && !empty($user->rights->salaries->read))
@@ -1570,7 +1570,7 @@ if ($action == 'create' || $action == 'adduserldap')
 
 			// Position/Job
 			print '<tr><td>'.$langs->trans("PostOrFunction").'</td>';
-			print '<td>'.$object->job.'</td>';
+			print '<td>'.dol_escape_htmltag($object->job).'</td>';
 			print '</tr>'."\n";
 
 			//$childids = $user->getAllChildIds(1);
@@ -2606,10 +2606,10 @@ if ($action == 'create' || $action == 'adduserldap')
 			print '<td>';
 			if ($caneditfield)
 			{
-				print '<input size="30" type="text" name="job" value="'.$object->job.'">';
+				print '<input size="30" type="text" name="job" value="'.dol_escape_htmltag($object->job).'">';
 			} else {
-				print '<input type="hidden" name="job" value="'.$object->job.'">';
-				print $object->job;
+				print '<input type="hidden" name="job" value="'.dol_escape_htmltag($object->job).'">';
+				print dol_escape_htmltag($object->job);
 			}
 			print '</td></tr>';
 

test/phpunit/SecurityTest.php

@@ -173,11 +173,12 @@ class SecurityTest extends PHPUnit\Framework\TestCase
 		$_GET["param1"]="222";
         $_POST["param1"]="333";
 		$_GET["param2"]='a/b#e(pr)qq-rr\cc';
-        $_GET["param3"]='"a/b#e(pr)qq-rr\cc';    // Same than param2 + "
+        $_GET["param3"]='"na/b#e(pr)qq-rr\cc';    // Same than param2 + " and n
         $_GET["param4"]='../dir';
         $_GET["param5"]="a_1-b";
-        $_POST["param6"]="&quot;&gt;<svg o&#110;load='console.log(&quot;Stored XSS &quot;)'&gt;";
-        $_GET["param7"]='"c:\this is a path~1\aaa" abc<bad>def</bad>';
+        $_POST["param6"]="&quot;&gt;<svg o&#110;load='console.log(&quot;123&quot;)'&gt;";
+        $_GET["param7"]='"c:\this is a path~1\aaa&#110;" abc<bad>def</bad>';
+        $_POST["param8"]="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
 
         // Test int
         $result=GETPOST('id', 'int');              // Must return nothing
@@ -199,7 +200,7 @@ class SecurityTest extends PHPUnit\Framework\TestCase
 
         $result=GETPOST("param3", 'alpha');  // Must return string sanitized from char "
         print __METHOD__." result=".$result."\n";
-        $this->assertEquals($result, 'a/b#e(pr)qq-rr\cc');
+        $this->assertEquals($result, 'na/b#e(pr)qq-rr\cc');
 
         $result=GETPOST("param4", 'alpha');  // Must return string sanitized from ../
         print __METHOD__." result=".$result."\n";
@@ -230,9 +231,15 @@ class SecurityTest extends PHPUnit\Framework\TestCase
         print __METHOD__." result=".$result."\n";
         $this->assertEquals('">', $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" abcdef', $result);
+        $this->assertEquals('"c:\this is a path~1\aaa&#110;" abcdef', $result);
+
+        // With alphanohtml, we must convert the html entities like &#110;
+        $result=GETPOST("param8", 'alphanohtml');
+        print __METHOD__." result=".$result."\n";
+        $this->assertEquals("Hacker<svg onload='console.log(123)'", $result);
 
         return $result;
     }