Resolve security/core#25 Escape use of cacheKey to prevent SQLI when …

…populating the prevNextCache Security civicrm#25 Update Redis implementation to match function sig of interface function
eileenmcnaughton · Jan 13, 2019 · 4c1e702 · 4c1e702
1 parent 4ef061f
commit 4c1e702
Show file tree

Hide file tree

Showing 6 changed files with 15 additions and 11 deletions.
diff --git a/CRM/Campaign/Selector/Search.php b/CRM/Campaign/Selector/Search.php
@@ -282,12 +282,12 @@ public function buildPrevNextCache($sort) {
       );
       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
@@ -1041,11 +1041,11 @@ public function fillupPrevNextCache($sort, $cacheKey, $start = 0, $end = self::C
     // 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_replace(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
@@ -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
@@ -61,8 +61,8 @@ public function __construct($settings) {
     $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
@@ -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
@@ -45,11 +45,11 @@ public function testFillSql() {
     $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"
     );