From 7ec003387ab0d68b78d9158e9f14950cce524888 Mon Sep 17 00:00:00 2001 From: ThomasNgr-OpenDSI Date: Mon, 20 Jul 2026 15:29:30 +0200 Subject: [PATCH] 22 report sec (#39235) * Sec: sql injection via api in POST of thirdparties/id/banckaccount - reported by MDnyn * Sec: sql injection via api in POST of thirdparties/id/banckaccount - reported by MDnyn --------- Co-authored-by: Laurent Destailleur --- htdocs/accountancy/admin/productaccount.php | 2 +- htdocs/adherents/class/subscription.class.php | 6 +-- htdocs/admin/external_rss.php | 2 +- htdocs/contrat/class/contratligne.class.php | 2 +- htdocs/core/class/discount.class.php | 2 +- .../class/companybankaccount.class.php | 2 +- htdocs/user/class/userbankaccount.class.php | 2 +- test/phpunit/CodingPhpTest.php | 53 +++++++++++++++---- 8 files changed, 51 insertions(+), 20 deletions(-) diff --git a/htdocs/accountancy/admin/productaccount.php b/htdocs/accountancy/admin/productaccount.php index ecef0d2a796..5ea3e654f63 100644 --- a/htdocs/accountancy/admin/productaccount.php +++ b/htdocs/accountancy/admin/productaccount.php @@ -352,7 +352,7 @@ if (getDolGlobalString('MAIN_PRODUCT_PERENTITY_SHARED')) { $sql .= " LEFT JOIN " . MAIN_DB_PREFIX . "accounting_account as aa ON aa.account_number = p." . $db->sanitize($accountancy_field_name) . " AND aa.fk_pcg_version = '" . $db->escape($pcgvercode) . "'"; } if (!empty($searchCategoryProductList)) { - $sql .= ' LEFT JOIN '.MAIN_DB_PREFIX."categorie_product as cp ON p.rowid = cp.fk_product"; // We'll need this table joined to the select in order to filter by categ + $sql .= " LEFT JOIN ".MAIN_DB_PREFIX."categorie_product as cp ON p.rowid = cp.fk_product"; // We'll need this table joined to the select in order to filter by categ } $sql .= ' WHERE p.entity IN ('.getEntity('product').')'; if (strlen(trim($search_current_account))) { diff --git a/htdocs/adherents/class/subscription.class.php b/htdocs/adherents/class/subscription.class.php index c021e335604..727de08e0b6 100644 --- a/htdocs/adherents/class/subscription.class.php +++ b/htdocs/adherents/class/subscription.class.php @@ -518,9 +518,9 @@ class Subscription extends CommonObject */ public function info($id) { - $sql = 'SELECT c.rowid, c.datec, c.tms as datem, c.fk_user_creat'; - $sql .= ' FROM '.MAIN_DB_PREFIX.'subscription as c'; - $sql .= ' WHERE c.rowid = '.((int) $id); + $sql = "SELECT c.rowid, c.datec, c.tms as datem, c.fk_user_creat"; + $sql .= " FROM ".MAIN_DB_PREFIX."subscription as c"; + $sql .= " WHERE c.rowid = ".((int) $id); $resql = $this->db->query($sql); if ($resql) { diff --git a/htdocs/admin/external_rss.php b/htdocs/admin/external_rss.php index 9a8e79bae72..e16da408523 100644 --- a/htdocs/admin/external_rss.php +++ b/htdocs/admin/external_rss.php @@ -148,7 +148,7 @@ if (GETPOST("delete")) { $obj = $db->fetch_object($resql); $sql = "DELETE FROM ".MAIN_DB_PREFIX."boxes"; - $sql .= " WHERE entity = ".$conf->entity; + $sql .= " WHERE entity = ".((int) $conf->entity); $sql .= " AND box_id = ".((int) $obj->rowid); $resql = $db->query($sql); diff --git a/htdocs/contrat/class/contratligne.class.php b/htdocs/contrat/class/contratligne.class.php index b0d01da8338..7815f701dd6 100644 --- a/htdocs/contrat/class/contratligne.class.php +++ b/htdocs/contrat/class/contratligne.class.php @@ -839,7 +839,7 @@ class ContratLigne extends CommonObjectLine if ($this->date_end > 0) { $sql .= ",date_fin_validite"; } - $sql .= ") VALUES ($this->fk_contrat, '', '".$this->db->escape($this->description)."',"; + $sql .= ") VALUES (".((int) $this->fk_contrat).", '', '".$this->db->escape($this->description)."',"; $sql .= ($this->fk_product > 0 ? $this->fk_product : "null").","; $sql .= " '".$this->db->escape((string) $this->qty)."',"; $sql .= " '".$this->db->escape($this->vat_src_code)."',"; diff --git a/htdocs/core/class/discount.class.php b/htdocs/core/class/discount.class.php index 3058ceb20ca..ef8b4025a6d 100644 --- a/htdocs/core/class/discount.class.php +++ b/htdocs/core/class/discount.class.php @@ -349,7 +349,7 @@ class DiscountAbsolute extends CommonObject $sql .= " multicurrency_amount_ht, multicurrency_amount_tva, multicurrency_amount_ttc,"; $sql .= " fk_facture_source, fk_invoice_supplier_source, multicurrency_code, multicurrency_tx"; $sql .= ")"; - $sql .= " VALUES (".$conf->entity.", '".$this->db->idate($this->datec != '' ? $this->datec : dol_now())."', ".((int) $this->socid).", ".(empty($this->discount_type) ? 0 : intval($this->discount_type)).", ".((int) $userid).", '".$this->db->escape($this->description)."',"; + $sql .= " VALUES (".((int) $conf->entity).", '".$this->db->idate($this->datec != '' ? $this->datec : dol_now())."', ".((int) $this->socid).", ".(empty($this->discount_type) ? 0 : intval($this->discount_type)).", ".((int) $userid).", '".$this->db->escape($this->description)."',"; $sql .= " ".price2num($this->amount_ht).", ".price2num($this->amount_tva).", ".price2num($this->amount_ttc).", ".price2num($this->tva_tx).", '".$this->db->escape($this->vat_src_code)."',"; $sql .= " ".price2num($this->multicurrency_amount_ht).", ".price2num($this->multicurrency_amount_tva).", ".price2num($this->multicurrency_amount_ttc).", "; $sql .= " ".($this->fk_facture_source ? ((int) $this->fk_facture_source) : "null").","; diff --git a/htdocs/societe/class/companybankaccount.class.php b/htdocs/societe/class/companybankaccount.class.php index c6832d92084..2fc761c9f22 100644 --- a/htdocs/societe/class/companybankaccount.class.php +++ b/htdocs/societe/class/companybankaccount.class.php @@ -396,7 +396,7 @@ class CompanyBankAccount extends Account $this->db->begin(); $sql = "INSERT INTO ".MAIN_DB_PREFIX."societe_rib (fk_soc, type, datec, model_pdf)"; - $sql .= " VALUES (".((int) $this->socid).", '".$this->type."', '".$this->db->idate($this->datec)."',"; + $sql .= " VALUES (".((int) $this->socid).", '".$this->db->escape($this->type)."', '".$this->db->idate($this->datec)."',"; $sql .= " '".$this->db->escape(getDolGlobalString("BANKADDON_PDF"))."'"; $sql .= ")"; $resql = $this->db->query($sql); diff --git a/htdocs/user/class/userbankaccount.class.php b/htdocs/user/class/userbankaccount.class.php index 19db5f1514e..be10af47621 100644 --- a/htdocs/user/class/userbankaccount.class.php +++ b/htdocs/user/class/userbankaccount.class.php @@ -96,7 +96,7 @@ class UserBankAccount extends Account $now = dol_now(); $sql = "INSERT INTO ".$this->db->prefix()."user_rib (fk_user, datec)"; - $sql .= " VALUES (".$this->userid.", '".$this->db->idate($now)."')"; + $sql .= " VALUES (".((int) $this->userid).", '".$this->db->idate($now)."')"; $resql = $this->db->query($sql); if ($resql) { if ($this->db->affected_rows($resql)) { diff --git a/test/phpunit/CodingPhpTest.php b/test/phpunit/CodingPhpTest.php index 6e6ff0196e9..afdc98068bf 100644 --- a/test/phpunit/CodingPhpTest.php +++ b/test/phpunit/CodingPhpTest.php @@ -117,7 +117,7 @@ class CodingPhpTest extends CommonClassTest // To process only 1 file, uncomment this /* foreach($returnlist as $key => $val) { - if ($val[0]['name'] != 'societe.class.php') { + if ($val[0]['name'] != 'companybankaccount.class.php') { unset($returnlist[$key]); } } @@ -407,21 +407,52 @@ class CodingPhpTest extends CommonClassTest } $this->assertTrue($ok, 'Found a forged SQL string that mix on same line the use of \' for PHP string and PHP variables in file '.$file['relativename'].' Use " to forge PHP string like this: $sql = "SELECT ".$myvar...'); - // Check sql string VALUES ... , ".$xxx + // Check sql string VALUES ... , ".$xxx or string VALUES ... , '".$xxx // with xxx that is not 'db-' (for $db->escape). It means we forget a ' if string, or an (int) if int, when forging sql request. + // ... = " VALUES (".((int) $this->socid).", '".$this->type."', '".$this->db->idate($this->datec)."',"; + $ok = true; $matches = array(); - preg_match_all('/(VALUES).*,\s*"\s*\.\s*\$(...)/', $filecontent, $matches, PREG_SET_ORDER); + preg_match_all('/(VALUES).*,\s*\'?"\s*\.\s*\$([a-z\-\>]+)/', $filecontent, $matches, PREG_SET_ORDER); foreach ($matches as $key => $val) { - if ($val[1] == 'VALUES' && $val[2] == 'db-') { // exclude $db->escape( - continue; + $matches2 = array(); + preg_match_all('/,\s*\'?"\s*\.\s*\$([a-z\-\>]+)/', $val[0], $matches2, PREG_SET_ORDER); + foreach ($matches2 as $key2 => $val2) { + if ($val2[1] == 'mydb->escape') { // exclude ".$mydb->escape( + continue; + } + if ($val2[1] == 'dbsession->escape') { // exclude ".$dbsession->escape( + continue; + } + if ($val2[1] == 'dbsession->idate') { // exclude ".$dbsession->escape( + continue; + } + if ($val2[1] == 'this->db->encrypt') { // exclude ".$this->db->encrypt( + continue; + } + if ($val2[1] == 'this->db->escape') { // exclude ".$this->db->escape( + continue; + } + if ($val2[1] == 'this->db->idate') { // exclude ".$this->db->idate( + continue; + } + if ($val2[1] == 'db->encrypt') { // exclude ".$db->encrypt( + continue; + } + if ($val2[1] == 'db->escape') { // exclude ".$db->escape( + continue; + } + if ($val2[1] == 'db->idate') { // exclude ".$db->idate( + continue; + } + if ($val2[1] == 'this->escape') { // exclude ".$this->db->encrypt( + continue; + } + + var_dump($matches2); + $ok = false; + break; } - if ($val[1] == 'VALUES' && $val[2] == 'thi' && preg_match('/this->db->encrypt/', $val[0])) { // exclude ".$this->db->encrypt( - continue; - } - 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 non quoted or not casted var in sql request '.$file['relativename'].' - Bad.');