From 45f66fa167e54ff3d849d2d6641a477d300caf91 Mon Sep 17 00:00:00 2001 From: Seamus Lee Date: Sun, 28 Oct 2018 08:44:08 +1100 Subject: [PATCH] Resolve security/core#25 Escape use of cacheKey to prevent SQLI when populating the prevNextCache Security #25 Update Redis implementation to match function sig of interface function --- CRM/Campaign/Selector/Search.php | 4 ++-- CRM/Contact/Selector.php | 4 ++-- CRM/Core/PrevNextCache/Interface.php | 4 +++- CRM/Core/PrevNextCache/Redis.php | 4 ++-- CRM/Core/PrevNextCache/Sql.php | 6 ++++-- tests/phpunit/E2E/Core/PrevNextTest.php | 4 ++-- 6 files changed, 15 insertions(+), 11 deletions(-) diff --git a/CRM/Campaign/Selector/Search.php b/CRM/Campaign/Selector/Search.php index 94a5cb5731..3332f86b1d 100644 --- 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 12f73b41d1..26e15333f7 100644 --- 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 33ce6dab75..6b72908318 100644 --- 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 ff4a0f3d6d..99986f4d71 100644 --- 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 953dee0243..38cd490f1b 100644 --- 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 d416e564b8..ed1fdebae6 100644 --- 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" ); -- 2.25.1