Commit 2c8b42d5 authored by Seamus Lee's avatar Seamus Lee

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
parent 5fb64d51
......@@ -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.
......
......@@ -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) {
......
......@@ -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.
......
......@@ -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);
}
......
......@@ -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);
}
......
......@@ -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"
);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment