CVE-2022-0224: Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection') (CWE-89)
value: dolibarr is vulnerable to Improper Neutralization of Special Elements used in an SQL Command
CWE description: The product constructs all or part of an SQL command using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify the intended SQL command when it is sent to a downstream component. Without sufficient removal or quoting of SQL syntax in user-controllable inputs, the generated SQL query can cause those inputs to be interpreted as SQL instead of ordinary user data.
CVSS v3 base score: 9.8
Published: 2022-01-14T18:15Z
Repository: https://github.com/dolibarr/dolibarr
Language: PHP
Files changed: htdocs/compta/sociales/list.php, test/phpunit/CodingPhpTest.php
Commit date: 2022-01-13 18:46:29 +0100
Commit message: Fix #hunterf1d1ce3e-ca92-4c7b-b1b8-934e28eaa486

Conflicts:
	test/phpunit/CodingPhpTest.php

Diff: diff --git a/htdocs/compta/sociales/list.php b/htdocs/compta/sociales/list.php
index ce379774e0a..578c7650846 100644
--- a/htdocs/compta/sociales/list.php
+++ b/htdocs/compta/sociales/list.php
@@ -204,11 +204,11 @@ if (!empty($conf->projet->enabled)) {
 	if ($search_project_ref != '') {
 		$sql .= natural_search("p.ref", $search_project_ref);
 	}
 }
 if (!empty($search_users)) {
-	$sql .= ' AND cs.fk_user IN('.implode(', ', $search_users).')';
+	$sql .= ' AND cs.fk_user IN ('.$db->sanitize(implode(', ', $search_users)).')';
 }
 if (!empty($search_type) && $search_type > 0) {
 	$sql .= ' AND cs.fk_mode_reglement='.$search_type;
 }
 if (!empty($search_account) && $search_account > 0) {
diff --git a/test/phpunit/CodingPhpTest.php b/test/phpunit/CodingPhpTest.php
index 9c00cb300c7..723f3853ad7 100644
--- a/test/phpunit/CodingPhpTest.php
+++ b/test/phpunit/CodingPhpTest.php
@@ -326,26 +326,34 @@ class CodingPhpTest extends PHPUnit\Framework\TestCase
 			}
 			//print __METHOD__." Result for checking we don't have non escaped string in sql requests for file ".$file."\n";
 			$this->assertTrue($ok, 'Found non escaped string in building of a sql request '.$file['relativename'].': '.$val[0].' - Bad.');
 			//exit;
 
-			// Check string 'IN (".xxx' or 'IN (\'.xxx'  with xxx that is not '$this->db->sanitize' and not '$db->sanitize'. It means we forget a db->sanitize when forging sql request.
-			preg_match_all('/ IN \([\'"]\s*\.\s*(.........)/i', $filecontent, $matches, PREG_SET_ORDER);
+			// Checks with IN
+
+			// Check string ' IN (".xxx' or ' IN (\'.xxx'  with xxx that is not '$this->db->sanitize' and not '$db->sanitize'. It means we forget a db->sanitize when forging sql request.
+			$ok=true;
+			$matches=array();
+			preg_match_all('/\s+IN\s*\([\'"]\s*\.\s*(.........)/i', $filecontent, $matches, PREG_SET_ORDER);
 			foreach ($matches as $key => $val) {
+				//var_dump($val);
 				if (!in_array($val[1], array('$db->sani', '$this->db', 'getEntity', 'WON\',\'L', 'self::STA', 'Commande:', 'CommandeF', 'Entrepot:', 'Facture::', 'FactureFo', 'ExpenseRe', 'Societe::', 'Ticket::S'))) {
 					$ok=false;
 					break;
 				}
 				//if ($reg[0] != 'db') $ok=false;
 			}
 			//print __METHOD__." Result for checking we don't have non escaped string in sql requests for file ".$file."\n";
 			$this->assertTrue($ok, 'Found non sanitized string in building of a IN or NOT IN sql request '.$file['relativename'].' - Bad.');
 			//exit;
 
-			// Check string 'IN (\'".xxx'   with xxx that is not '$this->db->sanitize' and not '$db->sanitize'. It means we forget a db->sanitize when forging sql request.
-			preg_match_all('/ IN \(\'"\s*\.\s*(.........)/i', $filecontent, $matches, PREG_SET_ORDER);
+			// Check string ' IN (\'".xxx'   with xxx that is not '$this->db->sanitize' and not '$db->sanitize'. It means we forget a db->sanitize when forging sql request.
+			$ok=true;
+			$matches=array();
+			preg_match_all('/\s+IN\s*\(\'"\s*\.\s*(.........)/i', $filecontent, $matches, PREG_SET_ORDER);
 			foreach ($matches as $key => $val) {
+				//var_dump($val);
 				if (!in_array($val[1], array('$db->sani', '$this->db', 'getEntity', 'WON\',\'L', 'self::STA', 'Commande:', 'CommandeF', 'Entrepot:', 'Facture::', 'FactureFo', 'ExpenseRe', 'Societe::', 'Ticket::S'))) {
 					$ok=false;
 					break;
 				}
 				//if ($reg[0] != 'db') $ok=false;

Vulnerable code: 	$sql .= ' AND cs.fk_user IN('.implode(', ', $search_users).')';
			// Check string 'IN (".xxx' or 'IN (\'.xxx'  with xxx that is not '$this->db->sanitize' and not '$db->sanitize'. It means we forget a db->sanitize when forging sql request.
			preg_match_all('/ IN \([\'"]\s*\.\s*(.........)/i', $filecontent, $matches, PREG_SET_ORDER);
			// Check string 'IN (\'".xxx'   with xxx that is not '$this->db->sanitize' and not '$db->sanitize'. It means we forget a db->sanitize when forging sql request.
			preg_match_all('/ IN \(\'"\s*\.\s*(.........)/i', $filecontent, $matches, PREG_SET_ORDER);
Fixed code: 	$sql .= ' AND cs.fk_user IN ('.$db->sanitize(implode(', ', $search_users)).')';
			// Checks with IN

			// Check string ' IN (".xxx' or ' IN (\'.xxx'  with xxx that is not '$this->db->sanitize' and not '$db->sanitize'. It means we forget a db->sanitize when forging sql request.
			$ok=true;
			$matches=array();
			preg_match_all('/\s+IN\s*\([\'"]\s*\.\s*(.........)/i', $filecontent, $matches, PREG_SET_ORDER);
				//var_dump($val);
			// Check string ' IN (\'".xxx'   with xxx that is not '$this->db->sanitize' and not '$db->sanitize'. It means we forget a db->sanitize when forging sql request.
			$ok=true;
			$matches=array();
			preg_match_all('/\s+IN\s*\(\'"\s*\.\s*(.........)/i', $filecontent, $matches, PREG_SET_ORDER);
				//var_dump($val);