Resolve security/core#25 Escape use of cacheKey to prevent SQLI when populating the...

Security #25 Update Redis implementation to match function sig of interface function

diff --git a/CRM/Campaign/Selector/Search.php b/CRM/Campaign/Selector/Search.php
index d7ed429ba5184c57ac38646687ebf7e2870b3431..8d71c4cd1e1e5f9ecf4722219a4a4e6c99a92ee0 100644 (file)
--- a/CRM/Campaign/Selector/Search.php
+++ b/CRM/Campaign/Selector/Search.php
@@ -282,12 +282,12 @@ class CRM_Campaign_Selector_Search extends CRM_Core_Selector_Base implements CRM
       );
       list($select, $from) = explode(' FROM ', $sql);
       $selectSQL = "
-      SELECT '$cacheKey', contact_a.id, contact_a.display_name
+      SELECT %1, contact_a.id, contact_a.display_name
 FROM {$from}
 ";
 
       try {
-        Civi::service('prevnext')->fillWithSql($cacheKey, $selectSQL);
+        Civi::service('prevnext')->fillWithSql($cacheKey, $selectSQL, [1 => [$cacheKey, 'String']]);
       }
       catch (CRM_Core_Exception $e) {
         // Heavy handed, no? Seems like this merits an explanation.

diff --git a/CRM/Contact/Selector.php b/CRM/Contact/Selector.php
index bd85b81808bc8750a911cf2cb5ddfb58abc929f1..b11e19e6e37bdd1378c176e461cc829dd64fc053 100644 (file)
--- a/CRM/Contact/Selector.php
+++ b/CRM/Contact/Selector.php
@@ -1041,11 +1041,11 @@ class CRM_Contact_Selector extends CRM_Core_Selector_Base implements CRM_Core_Se
     // the other alternative of running the FULL query will just be incredibly inefficient
     // and slow things down way too much on large data sets / complex queries
 
-    $selectSQL = "SELECT DISTINCT '$cacheKey', contact_a.id, contact_a.sort_name";
+    $selectSQL = "SELECT DISTINCT %1, contact_a.id, contact_a.sort_name";
 
     $sql = str_ireplace(array("SELECT contact_a.id as contact_id", "SELECT contact_a.id as id"), $selectSQL, $sql);
     try {
-      Civi::service('prevnext')->fillWithSql($cacheKey, $sql);
+      Civi::service('prevnext')->fillWithSql($cacheKey, $sql, [1 => [$cacheKey, 'String']]);
     }
     catch (CRM_Core_Exception $e) {
       if ($coreSearch) {

diff --git a/CRM/Core/PrevNextCache/Interface.php b/CRM/Core/PrevNextCache/Interface.php
index 33ce6dab7538595a1048b6168b6ba451a93b1c22..6b72908318c0f6215ba78e926603a2e02bf4be2a 100644 (file)
--- a/CRM/Core/PrevNextCache/Interface.php
+++ b/CRM/Core/PrevNextCache/Interface.php
@@ -40,9 +40,11 @@ interface CRM_Core_PrevNextCache_Interface {
    * @param string $sql
    *   A SQL query. The query *MUST* be a SELECT statement which yields
    *   the following columns (in order): cacheKey, entity_id1, data
+   * @param array $sqlParams
+   *   An array of SQLParams to be used with the $sql
    * @return bool
    */
-  public function fillWithSql($cacheKey, $sql);
+  public function fillWithSql($cacheKey, $sql, $sqlParams);
 
   /**
    * Store the contents of an array in the cache.

diff --git a/CRM/Core/PrevNextCache/Redis.php b/CRM/Core/PrevNextCache/Redis.php
index ff4a0f3d6d686afce38a457036e48999946d1e43..99986f4d714423fdf6ebaa545833479697953bab 100644 (file)
--- a/CRM/Core/PrevNextCache/Redis.php
+++ b/CRM/Core/PrevNextCache/Redis.php
@@ -61,8 +61,8 @@ class CRM_Core_PrevNextCache_Redis implements CRM_Core_PrevNextCache_Interface {
     $this->prefix .= \CRM_Utils_Cache::DELIMITER . 'prevnext' . \CRM_Utils_Cache::DELIMITER;
   }
 
-  public function fillWithSql($cacheKey, $sql) {
-    $dao = CRM_Core_DAO::executeQuery($sql, [], FALSE, NULL, FALSE, TRUE, TRUE);
+  public function fillWithSql($cacheKey, $sql, $sqlParams = []) {
+    $dao = CRM_Core_DAO::executeQuery($sql, $sqlParams, FALSE, NULL, FALSE, TRUE, TRUE);
     if (is_a($dao, 'DB_Error')) {
       throw new CRM_Core_Exception($dao->message);
     }

diff --git a/CRM/Core/PrevNextCache/Sql.php b/CRM/Core/PrevNextCache/Sql.php
index 953dee024303dfb4462795b2c6ea9d1dd842f225..38cd490f1b09e10bb642f8bd034854764df5072a 100644 (file)
--- a/CRM/Core/PrevNextCache/Sql.php
+++ b/CRM/Core/PrevNextCache/Sql.php
@@ -38,14 +38,16 @@ class CRM_Core_PrevNextCache_Sql implements CRM_Core_PrevNextCache_Interface {
    * @param string $sql
    *   A SQL query. The query *MUST* be a SELECT statement which yields
    *   the following columns (in order): cacheKey, entity_id1, data
+   * @param array $sqlParams
+   *   An array of Parameters to be used on the Insert Query
    * @return bool
    * @throws CRM_Core_Exception
    */
-  public function fillWithSql($cacheKey, $sql) {
+  public function fillWithSql($cacheKey, $sql, $sqlParams = []) {
     $insertSQL = "
 INSERT INTO civicrm_prevnext_cache (cacheKey, entity_id1, data)
 ";
-    $result = CRM_Core_DAO::executeQuery($insertSQL . $sql, [], FALSE, NULL, FALSE, TRUE, TRUE);
+    $result = CRM_Core_DAO::executeQuery($insertSQL . $sql, $sqlParams, FALSE, NULL, FALSE, TRUE, TRUE);
     if (is_a($result, 'DB_Error')) {
       throw new CRM_Core_Exception($result->message);
     }

diff --git a/tests/phpunit/E2E/Core/PrevNextTest.php b/tests/phpunit/E2E/Core/PrevNextTest.php
index d416e564b8141e49b34e9c21991cb0990b1f2f7d..ed1fdebae63b1241a4d5c7b60026066860880cb2 100644 (file)
--- a/tests/phpunit/E2E/Core/PrevNextTest.php
+++ b/tests/phpunit/E2E/Core/PrevNextTest.php
@@ -45,11 +45,11 @@ class PrevNextTest extends \CiviEndToEndTestCase {
     $query = new \CRM_Contact_BAO_Query(array(), NULL, NULL, FALSE, FALSE, 1, FALSE, TRUE, FALSE, NULL, 'AND');
     $sql = $query->searchQuery($start, $prefillLimit, $sort, FALSE, $query->_includeContactIds,
       FALSE, TRUE, TRUE);
-    $selectSQL = "SELECT DISTINCT '$this->cacheKey', contact_a.id, contact_a.sort_name";
+    $selectSQL = "SELECT DISTINCT %1, contact_a.id, contact_a.sort_name";
     $sql = str_replace(array("SELECT contact_a.id as contact_id", "SELECT contact_a.id as id"), $selectSQL, $sql);
 
     $this->assertTrue(
-      $this->prevNext->fillWithSql($this->cacheKey, $sql),
+      $this->prevNext->fillWithSql($this->cacheKey, $sql, [1 => [$this->cacheKey, 'String']]),
       "fillWithSql should return TRUE on success"
     );