In Model use placeholders / bind parameters rather than writing the Id Sites in the SQL query - Models should be implemented in a safe way and not assume that the callee will have validated all against SQLI

2 files changed, 15 insertions, 9 deletions

diff --git a/core/DataAccess/ArchiveSelector.php b/core/DataAccess/ArchiveSelector.php
index 56dede02b7..a89638f964 100644
--- a/core/DataAccess/ArchiveSelector.php
+++ b/core/DataAccess/ArchiveSelector.php
@@ -154,9 +154,9 @@ class ArchiveSelector
 
         $getArchiveIdsSql = "SELECT idsite, name, date1, date2, MAX(idarchive) as idarchive
                                FROM %s
-                              WHERE %s
+                              WHERE idsite IN (" . Common::getSqlStringFieldsArray($siteIds) . ")
                                 AND " . self::getNameCondition($plugins, $segment, $isSkipAggregationOfSubTables) . "
-                                AND idsite IN (" . implode(',', $siteIds) . ")
+                                AND %s
                            GROUP BY idsite, date1, date2";
 
         $monthToPeriods = array();
@@ -171,7 +171,7 @@ class ArchiveSelector
         foreach ($monthToPeriods as $table => $periods) {
             $firstPeriod = reset($periods);
 
-            $bind = array();
+            $bind = $siteIds;
 
             if ($firstPeriod instanceof Range) {
                 $dateCondition = "period = ? AND date1 = ? AND date2 = ?";
diff --git a/core/DataAccess/Model.php b/core/DataAccess/Model.php
index dd020af083..6e072bf1bc 100644
--- a/core/DataAccess/Model.php
+++ b/core/DataAccess/Model.php
@@ -37,6 +37,8 @@ class Model
         // prevent error 'The SELECT would examine more than MAX_JOIN_SIZE rows'
         Db::get()->query('SET SQL_BIG_SELECTS=1');
 
+        $idSitesString = Common::getSqlStringFieldsArray($idSites);
+
         $query = 'SELECT t1.idarchive FROM `' . $archiveTable . '` t1
                   INNER JOIN `' . $archiveTable . '` t2
                       ON t1.name    = t2.name
@@ -45,13 +47,13 @@ class Model
                       AND t1.date2  = t2.date2
                       AND t1.period = t2.period
                   WHERE t1.value = ' . ArchiveWriter::DONE_INVALIDATED . '
-                  AND t1.idsite IN (' . implode(",", $idSites) . ')
+                  AND t1.idsite IN (' . $idSitesString . ')
                   AND t2.value IN(' . ArchiveWriter::DONE_OK . ', ' . ArchiveWriter::DONE_OK_TEMPORARY . ')
                   AND t1.ts_archived < t2.ts_archived
                   AND t1.name LIKE \'done%\'
         ';
 
-        $result = Db::fetchAll($query);
+        $result = Db::fetchAll($query, $idSites);
 
         $archiveIds = array_map(
             function ($elm) {
@@ -80,6 +82,10 @@ class Model
         }
         $sql = implode(" OR ", $sql);
 
+
+        $sqlSites = " AND idsite IN (" . Common::getSqlStringFieldsArray($idSites) . ")";
+        $bind = array_merge($bind, $idSites);
+
         $sqlPeriod = "";
         if ($periodId) {
             $sqlPeriod = " AND period = ? ";
@@ -89,7 +95,7 @@ class Model
         $query = "UPDATE $archiveTable " .
             " SET value = " . ArchiveWriter::DONE_INVALIDATED .
             " WHERE ( $sql ) " .
-            " AND idsite IN (" . implode(",", $idSites) . ")" .
+            $sqlSites .
             $sqlPeriod;
         Db::query($query, $bind);
     }
@@ -122,12 +128,12 @@ class Model
 
     public function deleteArchiveIds($numericTable, $blobTable, $idsToDelete)
     {
-        $query = "DELETE FROM %s WHERE idarchive IN (" . implode(',', $idsToDelete) . ")";
+        $query = "DELETE FROM %s WHERE idarchive IN (" . Common::getSqlStringFieldsArray($idsToDelete) . ")";
 
-        Db::query(sprintf($query, $numericTable));
+        Db::query(sprintf($query, $numericTable), $idsToDelete);
 
         try {
-            Db::query(sprintf($query, $blobTable));
+            Db::query(sprintf($query, $blobTable), $idsToDelete);
         } catch (Exception $e) {
             // Individual blob tables could be missing
         }