diff --git a/htdocs/accountancy/class/bookkeeping.class.php b/htdocs/accountancy/class/bookkeeping.class.php index 825664d99e9..69aaaa6242f 100644 --- a/htdocs/accountancy/class/bookkeeping.class.php +++ b/htdocs/accountancy/class/bookkeeping.class.php @@ -927,23 +927,23 @@ class BookKeeping extends CommonObject } elseif ($key == 't.fk_doc' || $key == 't.fk_docdet' || $key == 't.piece_num') { $sqlwhere[] = $this->db->sanitize($key).' = '.((int) $value); } elseif ($key == 't.subledger_account' || $key == 't.numero_compte') { - $sqlwhere[] = $this->db->sanitize($key).' LIKE \''.$this->db->escape($this->db->escapeforlike($value)).'%\''; + $sqlwhere[] = $this->db->sanitize($key)." LIKE '".$this->db->escape($this->db->escapeforlike($value))."%'"; } elseif ($key == 't.date_creation>=') { - $sqlwhere[] = 't.date_creation >= \''.$this->db->idate($value).'\''; + $sqlwhere[] = "t.date_creation >= '".$this->db->idate($value)."'"; } elseif ($key == 't.date_creation<=') { - $sqlwhere[] = 't.date_creation <= \''.$this->db->idate($value).'\''; + $sqlwhere[] = "t.date_creation <= '".$this->db->idate($value)."'"; } elseif ($key == 't.date_export>=') { - $sqlwhere[] = 't.date_export >= \''.$this->db->idate($value).'\''; + $sqlwhere[] = "t.date_export >= '".$this->db->idate($value)."'"; } elseif ($key == 't.date_export<=') { - $sqlwhere[] = 't.date_export <= \''.$this->db->idate($value).'\''; + $sqlwhere[] = "t.date_export <= '".$this->db->idate($value)."'"; } elseif ($key == 't.date_validated>=') { - $sqlwhere[] = 't.date_validated >= \''.$this->db->idate($value).'\''; + $sqlwhere[] = "t.date_validated >= '".$this->db->idate($value)."'"; } elseif ($key == 't.date_validated<=') { - $sqlwhere[] = 't.date_validated <= \''.$this->db->idate($value).'\''; + $sqlwhere[] = "t.date_validated <= '".$this->db->idate($value)."'"; } elseif ($key == 't.date_lim_reglement>=') { - $sqlwhere[] = 't.date_lim_reglement>=\''.$this->db->idate($value).'\''; + $sqlwhere[] = "t.date_lim_reglement>='".$this->db->idate($value)."'"; } elseif ($key == 't.date_lim_reglement<=') { - $sqlwhere[] = 't.date_lim_reglement<=\''.$this->db->idate($value).'\''; + $sqlwhere[] = "t.date_lim_reglement<='".$this->db->idate($value)."'"; } elseif ($key == 't.credit' || $key == 't.debit') { $sqlwhere[] = natural_search($key, $value, 1, 1); } elseif ($key == 't.reconciled_option') { @@ -955,7 +955,7 @@ class BookKeeping extends CommonObject $sqlwhere[] = natural_search("t.code_journal", $value, 3, 1); } } elseif ($key == 't.search_accounting_code_in' && !empty($value)) { - $sqlwhere[] = 't.numero_compte IN ('.$this->db->sanitize($value, 1).')'; + $sqlwhere[] = "t.numero_compte IN (".$this->db->sanitize($value, 1).")"; } else { $sqlwhere[] = natural_search($key, $value, 0, 1); } @@ -1108,7 +1108,7 @@ class BookKeeping extends CommonObject if (count($filter) > 0) { foreach ($filter as $key => $value) { if ($key == 't.doc_date') { - $sqlwhere[] = $this->db->sanitize($key).' = \''.$this->db->idate($value).'\''; + $sqlwhere[] = $this->db->sanitize($key)." = '".$this->db->idate($value)."'"; } elseif ($key == 't.doc_date>=') { $sqlwhere[] = "t.doc_date >= '".$this->db->idate($value)."'"; } elseif ($key == 't.doc_date<=') { @@ -1128,23 +1128,23 @@ class BookKeeping extends CommonObject } elseif ($key == 't.fk_doc' || $key == 't.fk_docdet' || $key == 't.piece_num') { $sqlwhere[] = $this->db->sanitize($key).' = '.((int) $value); } elseif ($key == 't.subledger_account' || $key == 't.numero_compte') { - $sqlwhere[] = $this->db->sanitize($key).' LIKE \''.$this->db->escape($value).'%\''; + $sqlwhere[] = $this->db->sanitize($key)." LIKE '".$this->db->escape($value)."%'"; } elseif ($key == 't.date_creation>=') { - $sqlwhere[] = 't.date_creation >= \''.$this->db->idate($value).'\''; + $sqlwhere[] = "t.date_creation >= '".$this->db->idate($value)."'"; } elseif ($key == 't.date_creation<=') { - $sqlwhere[] = 't.date_creation <= \''.$this->db->idate($value).'\''; + $sqlwhere[] = "t.date_creation <= '".$this->db->idate($value)."'"; } elseif ($key == 't.tms>=') { - $sqlwhere[] = 't.tms >= \''.$this->db->idate($value).'\''; + $sqlwhere[] = "t.tms >= '".$this->db->idate($value)."'"; } elseif ($key == 't.tms<=') { - $sqlwhere[] = 't.tms <= \''.$this->db->idate($value).'\''; + $sqlwhere[] = "t.tms <= '".$this->db->idate($value)."'"; } elseif ($key == 't.date_export>=') { - $sqlwhere[] = 't.date_export >= \''.$this->db->idate($value).'\''; + $sqlwhere[] = "t.date_export >= '".$this->db->idate($value)."'"; } elseif ($key == 't.date_export<=') { - $sqlwhere[] = 't.date_export <= \''.$this->db->idate($value).'\''; + $sqlwhere[] = "t.date_export <= '".$this->db->idate($value)."'"; } elseif ($key == 't.date_validated>=') { - $sqlwhere[] = 't.date_validated >= \''.$this->db->idate($value).'\''; + $sqlwhere[] = "t.date_validated >= '".$this->db->idate($value)."'"; } elseif ($key == 't.date_validated<=') { - $sqlwhere[] = 't.date_validated <= \''.$this->db->idate($value).'\''; + $sqlwhere[] = "t.date_validated <= '".$this->db->idate($value)."'"; } elseif ($key == 't.credit' || $key == 't.debit') { $sqlwhere[] = natural_search($key, $value, 1, 1); } elseif ($key == 't.code_journal' && !empty($value)) { @@ -1159,7 +1159,7 @@ class BookKeeping extends CommonObject } } if (count($sqlwhere) > 0) { - $sql .= ' AND '.implode(" ".$this->db->sanitize($filtermode)." ", $sqlwhere); + $sql .= " AND ".implode(" ".$this->db->sanitize($filtermode)." ", $sqlwhere); } $filter = ''; diff --git a/htdocs/comm/mailing/class/advtargetemailing.class.php b/htdocs/comm/mailing/class/advtargetemailing.class.php index a7eef05e6a5..63f471aaec9 100644 --- a/htdocs/comm/mailing/class/advtargetemailing.class.php +++ b/htdocs/comm/mailing/class/advtargetemailing.class.php @@ -1013,9 +1013,9 @@ class AdvanceTargetingMailing extends CommonObject */ public function transformToSQL($column_to_test, $criteria) { - $return_sql_criteria = '('; + $return_sql_criteria = "("; - //This is a multiple value test + // This is a multiple value test if (preg_match('/;/', $criteria)) { $return_sql_not_like = array(); $return_sql_like = array(); @@ -1023,23 +1023,23 @@ class AdvanceTargetingMailing extends CommonObject $criteria_array = explode(';', $criteria); foreach ($criteria_array as $inter_criteria) { if (preg_match('/!/', $inter_criteria)) { - $return_sql_not_like[] = '('.$column_to_test.' NOT LIKE \''.str_replace('!', '', $inter_criteria).'\')'; + $return_sql_not_like[] = "(".$this->db->sanitize($column_to_test)." NOT LIKE '".$this->db->sanitize(str_replace('!', '', $inter_criteria))."')"; } else { - $return_sql_like[] = '('.$column_to_test.' LIKE \''.$inter_criteria.'\')'; + $return_sql_like[] = "(".$this->db->sanitize($column_to_test)." LIKE '".$this->db->sanitize($inter_criteria)."')"; } } if (count($return_sql_like) > 0) { - $return_sql_criteria .= '('.implode(' OR ', $return_sql_like).')'; + $return_sql_criteria .= "(".implode(" OR ", $return_sql_like).")"; // element in arrays were sanitized previously } if (count($return_sql_not_like) > 0) { - $return_sql_criteria .= ' AND ('.implode(' AND ', $return_sql_not_like).')'; + $return_sql_criteria .= " AND (".implode(" AND ", $return_sql_not_like).")"; // element in arrays were sanitized previously } } else { - $return_sql_criteria .= $column_to_test.' LIKE \''.$this->db->escape($criteria).'\''; + $return_sql_criteria .= $this->db->sanitize($column_to_test)." LIKE '".$this->db->escape($criteria)."'"; } - $return_sql_criteria .= ')'; + $return_sql_criteria .= ")"; return $return_sql_criteria; } diff --git a/htdocs/core/db/pgsql.class.php b/htdocs/core/db/pgsql.class.php index 8c456044410..1a32049ff65 100644 --- a/htdocs/core/db/pgsql.class.php +++ b/htdocs/core/db/pgsql.class.php @@ -324,13 +324,13 @@ class DoliDBPgsql extends DoliDB // To have PostgreSQL case sensitive $count_like = 0; - $line = str_replace(' LIKE \'', ' ILIKE \'', $line, $count_like); + $line = str_replace(" LIKE '", " ILIKE '", $line, $count_like); if (getDolGlobalString('PSQL_USE_UNACCENT') && $count_like > 0) { // @see https://docs.PostgreSQL.fr/11/unaccent.html : 'unaccent()' function must be installed before $line = preg_replace('/\s+(\(+\s*)([a-zA-Z0-9\-\_\.]+) ILIKE /', ' \1unaccent(\2) ILIKE ', $line); } - $line = str_replace(' LIKE BINARY \'', ' LIKE \'', $line); + $line = str_replace(" LIKE BINARY '", " LIKE '", $line); // Replace INSERT IGNORE into INSERT $line = preg_replace('/^INSERT IGNORE/', 'INSERT', $line); diff --git a/htdocs/core/db/sqlite3.class.php b/htdocs/core/db/sqlite3.class.php index 3e5fe6cb908..0cfa2a48a27 100644 --- a/htdocs/core/db/sqlite3.class.php +++ b/htdocs/core/db/sqlite3.class.php @@ -1301,7 +1301,7 @@ class DoliDBSqlite3 extends DoliDB // FIXME: not for SQLite $fullpathofdump = '/pathtomysqldump/mysqldump'; - $resql = $this->query('SHOW VARIABLES LIKE \'basedir\''); + $resql = $this->query("SHOW VARIABLES LIKE 'basedir'"); if ($resql) { $liste = $this->fetch_array($resql); $basedir = $liste['Value']; @@ -1320,7 +1320,7 @@ class DoliDBSqlite3 extends DoliDB // FIXME: not for SQLite $fullpathofimport = '/pathtomysql/mysql'; - $resql = $this->query('SHOW VARIABLES LIKE \'basedir\''); + $resql = $this->query("SHOW VARIABLES LIKE 'basedir'"); if ($resql) { $liste = $this->fetch_array($resql); $basedir = $liste['Value']; @@ -1387,7 +1387,9 @@ class DoliDBSqlite3 extends DoliDB $result = array(); /* $sql='SHOW STATUS'; - if ($filter) $sql.=" LIKE '".$this->escape($filter)."'"; + if ($filter) { + $sql.=" LIKE '".$this->escape($filter)."'"; + } $resql=$this->query($sql); if ($resql) { diff --git a/test/phpunit/CodingPhpTest.php b/test/phpunit/CodingPhpTest.php index af861b03c3b..b2f02ec5341 100644 --- a/test/phpunit/CodingPhpTest.php +++ b/test/phpunit/CodingPhpTest.php @@ -293,6 +293,22 @@ class CodingPhpTest extends CommonClassTest //exit; + // Part to scan code vulnerability on SQL injection + + + // Check sql using ' instead of " + $ok = true; + $matches = array(); + preg_match_all('/LIKE \\\/', $filecontent, $matches, PREG_SET_ORDER); + foreach ($matches as $key => $val) { + var_dump($matches); + $ok = false; + break; + } + //print __METHOD__." Result for checking we don't have non escaped string in sql requests for file ".$file."\n"; + $this->assertTrue($ok, "Found a LIKE \' when we should have LIKE ' - Bad."); + //exit; + // Check sql string DELETE|OR|AND|WHERE|INSERT ... yyy = ".$xxx // with xxx that is not 'thi' (for $this->db->sanitize) and 'db-' (for $db->sanitize). It means we forget a ' if string, or an (int) if int, when forging sql request.