vignesh/dolibarr
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

QUAL: Add SqlInjectionPlugin to detect unsafe SQL variable usage (fixes, notices) (#38722)

Browse source 
* QUAL: Add SqlInjectionPlugin to detect unsafe SQL variable usage

# QUAL: Add SqlInjectionPlugin to detect unsafe SQL variable usage

This commit adds a new SqlInjectionPlugin to the Phan configuration, which helps detect unsafe SQL variable usage in the Dolibarr codebase. The plugin checks for variables used in sql expressions that are not properly escaped, cast, or protected by safe methods.

* Qual: Add SqlInjection exceptions to Phan baseline

* Qual: Add SqlInjection exceptions to Phan baseline

* Qual: Update baseline.txt to remove SqlInjection suppressions

The SqlInjection suppressions have been removed from the baseline.txt file.
This reenables the notification of "unfixed" cases.

* Qual: SQL injection detection for missing quotes on escaped strings

# Qual: SQL injection detection for missing quotes on escaped strings

In summary: report lines with missing quotes as in `" WHERE ".$db->escape($value)." OR "`.

* Qual: Fix code for SqlInjection analysis

# Qual: Fix code for SqlInjection analysis

Rewrite some escape calls, change escape() in sanitize(), rename variable, ignore SqlInjection notices.

* Qual: Update baseline

Allow some SqlInjection* notices, to be fixed later or already fixed in develop.
This commit is contained in:
MDW 2026-08-08 11:19:04 +02:00 committed by GitHub
parent 33df9eac1b
commit a0b820564f
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
12 changed files with 1801 additions and 30 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

19
dev/tools/phan/baseline.txt
View file

@ -9,25 +9,27 @@
*/
return [
// # Issue statistics:
// PhanUndeclaredProperty : 370+ occurrences
// PhanUndeclaredProperty : 340+ occurrences
// PhanTypeMismatchArgument : 25+ occurrences
// PhanUndeclaredGlobalVariable : 20+ occurrences
// PhanTypeMismatchProperty : 15+ occurrences
// PhanTypeInvalidDimOffset : 10+ occurrences
// PhanTypeMismatchDimFetch : 10+ occurrences
// SqlInjection : 7 occurrences
// SqlInjectionUnquotedEscape : 6 occurrences
// PhanTypeArraySuspiciousNull : 5 occurrences
// PhanTypeExpectedObjectPropAccess : 5 occurrences
// PhanUndeclaredMethod : 5 occurrences
// PhanPluginDuplicateArrayKey : 4 occurrences
// PhanTypeMismatchArgumentNullable : 3 occurrences
// PhanTypeExpectedObjectPropAccess : 4 occurrences
// PhanPluginUndeclaredVariableIsset : 2 occurrences
// PhanTypeMismatchArgumentProbablyReal : 2 occurrences
// PhanParamTooMany : 1 occurrence
// PhanPluginSuspiciousParamPosition : 1 occurrence
// PhanTypeExpectedObjectPropAccessButGotNull : 1 occurrence
// PhanTypeMismatchReturn : 1 occurrence
// Currently, file_suppressions and directory_suppressions are the only supported suppressions
'file_suppressions' => [
'htdocs/blockedlog/admin/filecheck_diff.php' => ['SqlInjection'],
'htdocs/comm/action/index.php' => ['PhanTypeMismatchProperty'],
'htdocs/comm/action/pertype.php' => ['PhanTypeExpectedObjectPropAccess'],
'htdocs/comm/action/peruser.php' => ['PhanTypeMismatchArgument'],
@ -46,6 +48,7 @@ return [
'htdocs/compta/sociales/card.php' => ['PhanUndeclaredProperty'],
'htdocs/compta/tva/class/paymentvat.class.php' => ['PhanUndeclaredProperty'],
'htdocs/compta/tva/clients.php' => ['PhanTypeArraySuspiciousNull', 'PhanTypeInvalidDimOffset'],
'htdocs/contact/list.php' => ['SqlInjectionUnquotedEscape'],
'htdocs/core/actions_addupdatedelete.inc.php' => ['PhanUndeclaredProperty'],
'htdocs/core/actions_massactions.inc.php' => ['PhanUndeclaredProperty'],
'htdocs/core/actions_sendmails.inc.php' => ['PhanUndeclaredProperty'],
@ -59,9 +62,9 @@ return [
'htdocs/core/class/ctyperesource.class.php' => ['PhanUndeclaredProperty'],
'htdocs/core/class/dolgraph.class.php' => ['PhanUndeclaredProperty'],
'htdocs/core/class/emailsenderprofile.class.php' => ['PhanUndeclaredProperty'],
'htdocs/core/class/html.form.class.php' => ['PhanParamTooMany', 'PhanPluginSuspiciousParamPosition', 'PhanTypeMismatchArgument'],
'htdocs/core/class/html.formcompany.class.php' => ['PhanUndeclaredProperty'],
'htdocs/core/class/html.formmail.class.php' => ['PhanUndeclaredProperty'],
'htdocs/core/class/openid.class.php' => ['PhanTypeMismatchArgument'],
'htdocs/core/class/timespent.class.php' => ['PhanUndeclaredMethod', 'PhanUndeclaredProperty'],
'htdocs/core/lib/admin.lib.php' => ['PhanUndeclaredProperty'],
'htdocs/core/lib/files.lib.php' => ['PhanUndeclaredProperty'],
@ -82,6 +85,7 @@ return [
'htdocs/core/modules/facture/doc/pdf_octopus.modules.php' => ['PhanTypeMismatchArgument', 'PhanTypeMismatchArgumentNullable', 'PhanUndeclaredProperty'],
'htdocs/core/modules/holiday/mod_holiday_immaculate.php' => ['PhanTypeMismatchArgument'],
'htdocs/core/modules/hrm/mod_evaluation_advanced.php' => ['PhanUndeclaredProperty'],
'htdocs/core/modules/import/modules_import.class.php' => ['SqlInjection'],
'htdocs/core/modules/member/modules_cards.php' => ['PhanTypeMismatchArgument'],
'htdocs/core/modules/mrp/doc/pdf_vinci.modules.php' => ['PhanUndeclaredProperty'],
'htdocs/core/modules/mrp/mod_mo_advanced.php' => ['PhanUndeclaredProperty'],
@ -96,7 +100,6 @@ return [
'htdocs/core/modules/supplier_order/doc/pdf_muscadet.modules.php' => ['PhanTypeMismatchDimFetch', 'PhanTypeMismatchProperty', 'PhanUndeclaredProperty'],
'htdocs/core/modules/supplier_proposal/doc/pdf_aurore.modules.php' => ['PhanTypeMismatchDimFetch', 'PhanTypeMismatchProperty', 'PhanUndeclaredProperty'],
'htdocs/core/modules/supplier_proposal/doc/pdf_zenith.modules.php' => ['PhanTypeMismatchDimFetch', 'PhanTypeMismatchProperty', 'PhanUndeclaredProperty'],
'htdocs/core/modules/syslog/mod_syslog_file.php' => ['PhanPluginDuplicateArrayKey'],
'htdocs/core/modules/workstation/mod_workstation_advanced.php' => ['PhanUndeclaredProperty'],
'htdocs/core/multicompany_page.php' => ['PhanTypeMismatchArgument'],
'htdocs/core/tpl/massactions_pre.tpl.php' => ['PhanTypeMismatchArgumentNullable', 'PhanUndeclaredProperty'],
@ -137,13 +140,12 @@ return [
'htdocs/loan/note.php' => ['PhanUndeclaredProperty'],
'htdocs/loan/payment/payment.php' => ['PhanUndeclaredProperty'],
'htdocs/mrp/tpl/linkedobjectblock.tpl.php' => ['PhanUndeclaredProperty'],
'htdocs/multicurrency/class/multicurrency.class.php' => ['PhanTypeExpectedObjectPropAccess'],
'htdocs/partnership/class/partnership.class.php' => ['PhanUndeclaredProperty'],
'htdocs/partnership/core/modules/partnership/mod_partnership_advanced.php' => ['PhanUndeclaredProperty'],
'htdocs/partnership/partnership_card.php' => ['PhanUndeclaredGlobalVariable'],
'htdocs/partnership/partnership_list.php' => ['PhanUndeclaredProperty'],
'htdocs/product/class/api_products.class.php' => ['PhanTypeMismatchArgument', 'PhanTypeMismatchReturn', 'PhanUndeclaredProperty'],
'htdocs/product/class/html.formproduct.class.php' => ['PhanUndeclaredProperty'],
'htdocs/product/class/html.formproduct.class.php' => ['PhanUndeclaredProperty', 'SqlInjection'],
'htdocs/product/class/productfournisseurprice.class.php' => ['PhanUndeclaredMethod', 'PhanUndeclaredProperty'],
'htdocs/product/inventory/class/inventory.class.php' => ['PhanUndeclaredProperty'],
'htdocs/product/price.php' => ['PhanUndeclaredProperty'],
@ -202,6 +204,7 @@ return [
'htdocs/variants/tpl/productattributevalueline_view.tpl.php' => ['PhanUndeclaredProperty'],
'htdocs/webhook/class/target.class.php' => ['PhanUndeclaredMethod'],
'htdocs/webhook/target_card.php' => ['PhanUndeclaredGlobalVariable'],
'htdocs/webportal/class/html.formwebportal.class.php' => ['SqlInjection'],
'htdocs/webservices/server_project.php' => ['PhanUndeclaredProperty'],
],
// 'directory_suppressions' => ['src/directory_name' => ['PhanIssueName1', 'PhanIssueName2']] can be manually added if needed.

3
dev/tools/phan/config.php
View file

@ -274,6 +274,7 @@ return [
'conffiletoshowshort' => 'string',
'dateSelector' => 'int<0,1>',
'db' => '\DoliDB',
'dbsession' => '\DoliDB',
'disableedit' => 'int<0,1>',
'disablemove' => 'int<0,1>',
'disableremove' => 'int<0,1>',
@ -406,9 +407,11 @@ return [
'/^GETPOSTFLOAT$/' => [1, '{^(?:|M[UTS]|C[UT]|\d+)$}',"InvalidGetPostFloatRounding"],
'/^price2num$/' => [1, '{^(?:|M[UTS]|C[UT]|\d+)$}',"InvalidPrice2NumRounding"],
],
'SqlInjectionPlugin' => ['debug' => false],
'plugins' => [
__DIR__.'/plugins/NoVarDumpPlugin.php',
__DIR__.'/plugins/ParamMatchRegexPlugin.php',
__DIR__.'/plugins/SqlInjectionPlugin.php',
// checks if a function, closure or method unconditionally returns.
// can also be written as 'vendor/phan/phan/.phan/plugins/AlwaysReturnPlugin.php'
'DeprecateAliasPlugin',

3
dev/tools/phan/config_extended.php
View file

@ -1,5 +1,5 @@
<?php
/* Copyright (C) 2024-2025 MDW <mdeweerd@users.noreply.github.com>
/* Copyright (C) 2024-2026 MDW <mdeweerd@users.noreply.github.com>
* Copyright (C) 2024 Frédéric France <frederic.france@free.fr>
*
* This is the phan config file used by dev/tools/apstats.php
@ -10,6 +10,7 @@
$config = include __DIR__.DIRECTORY_SEPARATOR."config.php";
$config['plugins'] = [
__DIR__.'/plugins/SqlInjectionPlugin.php',
__DIR__.'/plugins/NoVarDumpPlugin.php',
__DIR__.'/plugins/ParamMatchRegexPlugin.php',
'DeprecateAliasPlugin',

1763
dev/tools/phan/plugins/SqlInjectionPlugin.php Normal file
View file

File diff suppressed because it is too large Load diff

4
htdocs/compta/bank/graph.php
View file

@ -580,8 +580,8 @@ if ($result < 0) {
$sql .= ", ".MAIN_DB_PREFIX."bank_account as ba";
$sql .= " WHERE b.fk_account = ba.rowid";
$sql .= " AND ba.entity IN (".getEntity('bank_account').")";
$sql .= " AND b.datev >= '".$db->escape($year)."-".$db->escape($month)."-01 00:00:00'";
$sql .= " AND b.datev < '".$db->escape($yearnext)."-".$db->escape($monthnext)."-01 00:00:00'";
$sql .= " AND b.datev >= '".$db->escape($year."-".$month."-01 00:00:00")."'";
$sql .= " AND b.datev < '".$db->escape($yearnext."-".$monthnext."-01 00:00:00")."'";
$sql .= " AND b.amount > 0";
if ($account && GETPOST("option") != 'all') {
$sql .= " AND b.fk_account IN (".$db->sanitize($account).")";

9
htdocs/core/class/commonobject.class.php
View file

@ -10628,10 +10628,11 @@ abstract class CommonObject
/**
* Function to return the array of data key-value from the ->fields and all the ->properties of an object.
*
* Note: $this->${field} are set by the page that make the createCommon() or the updateCommon().
* $this->${field} should be a clean and string value (so date are formatted for SQL insert).
* Note:
* - $this->${field} are set by the page that makes the createCommon() or the updateCommon().
* - $this->${field} should be a clean and string value (so date are formatted for SQL insert).
*
* @return array<string,null|int|float|string> Array with all values of each property to update
* @return array<string,null|int|float|string> Array with all values of each property to update - caller is responsible for escaping
*/
protected function setSaveQuery()
{
@ -10681,7 +10682,7 @@ abstract class CommonObject
} else {
// Note: If $this->{$field} is not defined, it means there is a bug into definition of ->fields or a missing declaration of property
// We should keep the warning generated by this because it is a bug somewhere else in code, not here.
$queryarray[$field] = $this->{$field};
$queryarray[$field] = $this->{$field}; // @phan-suppress-current-line SqlInjection
}
if (array_key_exists('type', $info) && $info['type'] == 'timestamp' && empty($queryarray[$field])) {

4
htdocs/core/class/html.formother.class.php
View file

@ -551,7 +551,7 @@ class FormOther
//Add hook to filter on user (for example on usergroup define in custom modules)
if (!empty($reshook)) {
$sql_usr .= $hookmanager->resArray[0];
$sql_usr .= $hookmanager->resArray[0]; // Trust the hook: @phan-suppress-current-line SqlInjection
}
// Add existing sales representatives of thirdparty of external user
@ -574,7 +574,7 @@ class FormOther
//Add hook to filter on user (for example on usergroup define in custom modules)
if (!empty($reshook)) {
$sql_usr .= $hookmanager->resArray[1];
$sql_usr .= $hookmanager->resArray[1]; // Trust the hook: @phan-suppress-current-line SqlInjection
}
}

8
htdocs/core/db/mysqli.class.php
View file

@ -727,14 +727,14 @@ class DoliDBMysqli extends DoliDB
}
// ALTER DATABASE dolibarr_db DEFAULT CHARACTER SET latin DEFAULT COLLATE latin1_swedish_ci
$sql = "CREATE DATABASE `".$this->escape($database)."`";
$sql .= " DEFAULT CHARACTER SET `".$this->escape($charset)."` DEFAULT COLLATE `".$this->escape($collation)."`";
$sql = "CREATE DATABASE `".$this->sanitize($database)."`";
$sql .= " DEFAULT CHARACTER SET `".$this->sanitize($charset)."` DEFAULT COLLATE `".$this->sanitize($collation)."`";
dol_syslog($sql, LOG_DEBUG);
$ret = $this->query($sql);
if (!$ret) {
// We try again for compatibility with Mysql < 4.1.1
$sql = "CREATE DATABASE `".$this->escape($database)."`";
$sql = "CREATE DATABASE `".$this->sanitize($database)."`";
dol_syslog($sql, LOG_DEBUG);
$ret = $this->query($sql);
}
@ -1130,7 +1130,7 @@ class DoliDBMysqli extends DoliDB
$sql = "CREATE USER '".$this->escape($dolibarr_main_db_user)."'@'localhost' IDENTIFIED BY '".$this->escape($dolibarr_main_db_pass)."'";
$resql = $this->query($sql);
$sql = "GRANT ALL PRIVILEGES ON ".$this->escape($dolibarr_main_db_name).".* TO '".$this->escape($dolibarr_main_db_user)."'@'".$this->escape($dolibarr_main_db_host)."'";
$sql = "GRANT ALL PRIVILEGES ON `".$this->sanitize($dolibarr_main_db_name)."`.* TO '".$this->escape($dolibarr_main_db_user)."'@'".$this->escape($dolibarr_main_db_host)."'";
dol_syslog(get_class($this)."::DDLCreateUser", LOG_DEBUG); // No sql to avoid password in log
$resql = $this->query($sql);
if (!$resql) {

6
htdocs/core/db/pgsql.class.php
View file

@ -965,7 +965,7 @@ class DoliDBPgsql extends DoliDB
//print $charset.' '.setlocale(LC_CTYPE,'0'); exit;
// NOTE: Do not use ' around the database name
$sql = "CREATE DATABASE ".$this->escape($database)." OWNER '".$this->escape($owner)."' ENCODING '".$this->escape((string) $charset)."'";
$sql = "CREATE DATABASE ".$this->sanitize($database)." OWNER '".$this->escape($owner)."' ENCODING '".$this->escape((string) $charset)."'";
dol_syslog($sql, LOG_DEBUG);
$ret = $this->query($sql);
@ -1287,10 +1287,10 @@ class DoliDBPgsql extends DoliDB
if (isset($field_desc['null']) && ($field_desc['null'] == 'not null' || $field_desc['null'] == 'NOT NULL')) {
// We will try to change format of column to NOT NULL. To be sure the ALTER works, we try to update fields that are NULL
if ($field_desc['type'] == 'varchar' || $field_desc['type'] == 'text') {
$sqlbis = "UPDATE ".$this->sanitize($table)." SET ".$this->escape($field_name)." = '".$this->escape(isset($field_desc['default']) ? $field_desc['default'] : '')."' WHERE ".$this->escape($field_name)." IS NULL";
$sqlbis = "UPDATE ".$this->sanitize($table)." SET ".$this->sanitize($field_name)." = '".$this->escape(isset($field_desc['default']) ? $field_desc['default'] : '')."' WHERE ".$this->sanitize($field_name)." IS NULL";
$this->query($sqlbis);
} elseif (in_array($field_desc['type'], array('tinyint', 'smallint', 'int', 'double'))) {
$sqlbis = "UPDATE ".$this->sanitize($table)." SET ".$this->escape($field_name)." = ".((float) $this->escape(isset($field_desc['default']) ? $field_desc['default'] : 0))." WHERE ".$this->escape($field_name)." IS NULL";
$sqlbis = "UPDATE ".$this->sanitize($table)." SET ".$this->sanitize($field_name)." = ".((float) $this->escape(isset($field_desc['default']) ? $field_desc['default'] : 0))." WHERE ".$this->sanitize($field_name)." IS NULL";
$this->query($sqlbis);
}
}

4
htdocs/core/db/sqlite3.class.php
View file

@ -859,8 +859,8 @@ class DoliDBSqlite3 extends DoliDB
}
// ALTER DATABASE dolibarr_db DEFAULT CHARACTER SET latin DEFAULT COLLATE latin1_swedish_ci
$sql = "CREATE DATABASE ".$this->escape($database);
$sql .= " DEFAULT CHARACTER SET ".$this->escape($charset)." DEFAULT COLLATE ".$this->escape($collation);
$sql = "CREATE DATABASE ".$this->sanitize($database);
$sql .= " DEFAULT CHARACTER SET ".$this->sanitize($charset)." DEFAULT COLLATE ".$this->sanitize($collation);
dol_syslog($sql, LOG_DEBUG);
$ret = $this->query($sql);

2
htdocs/core/modules/import/modules_import.class.php
View file

@ -1720,7 +1720,7 @@ class ModeleImports
$keyfield = 'rowid';
}
$sqlend = " WHERE ".$this->db->escape($keyfield)." = ".((int) $lastinsertid);
$sqlend = " WHERE ".$this->db->sanitize($keyfield)." = ".((int) $lastinsertid);
if ($is_table_category_link && !empty($where)) {
'@phan-var-force string[] $where';

6
htdocs/product/class/html.formproduct.class.php
View file

@ -2,7 +2,7 @@
/* Copyright (C) 2008-2009 Laurent Destailleur <eldy@users.sourceforge.net>
* Copyright (C) 2015-2017 Francis Appels <francis.appels@yahoo.com>
* Copyright (C) 2024-2025 Frédéric France <frederic.france@free.fr>
* Copyright (C) 2024-2025 MDW <mdeweerd@users.noreply.github.com>
* Copyright (C) 2024-2026 MDW <mdeweerd@users.noreply.github.com>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@ -167,8 +167,8 @@ class FormProduct
}
}
$sortfield = implode(',', $arraysortfield);
$sortorder = implode(',', $arraysortorder);
$sql .= $this->db->order($sortfield, $sortorder);
$sortorder_unsanitized = implode(',', $arraysortorder); // $db->order sanitizes
$sql .= $this->db->order($sortfield, $sortorder_unsanitized);
dol_syslog(get_class($this).'::loadWarehouses', LOG_DEBUG);
$resql = $this->db->query($sql);