Skip to content
Permalink
Browse files

security/core#28 - CRM_Contact - Fix SQL injection in group/tag search

This fixes various SQL injections in CRM_Contact_BAO_Query in the group
and tag search code. CRM_Contact_BAO_Query is used by the API and some
other core features such as the advanced contact search.

For CRM_Contact_BAO_Query::tag, the lack of input validation meant that
API syntax that would typically not work for other parameters works for
tag search, so the fix attempts to not break backwards-compatibility
for API calls like Contact.get tag="1, 2" (i.e. using a comma-separated
list with spaces).
  • Loading branch information...
pfigel authored and seamuslee001 committed Oct 27, 2018
1 parent 5078188 commit 0cf0a3f39283cac6b7c45a059ffba09621e813e3
Showing with 100 additions and 15 deletions.
  1. +35 −15 CRM/Contact/BAO/Query.php
  2. +65 −0 tests/phpunit/api/v3/ContactTest.php
@@ -2972,7 +2972,7 @@ public function group($values) {
$smartGroupIDs[] = $id;
}
else {
$regularGroupIDs[] = $id;
$regularGroupIDs[] = trim($id);
}
}
@@ -3011,7 +3011,10 @@ public function group($values) {
if (count($regularGroupIDs) > 1) {
$op = strpos($op, 'IN') ? $op : ($op == '!=') ? 'NOT IN' : 'IN';
}
$groupIds = implode(',', (array) $regularGroupIDs);
$groupIds = CRM_Utils_Type::validate(
implode(',', (array) $regularGroupIDs),
'CommaSeparatedIntegers'
);
$gcTable = "`civicrm_group_contact-" . uniqid() . "`";
$joinClause = array("contact_a.id = {$gcTable}.contact_id");
@@ -3172,22 +3175,25 @@ public function tagSearch(&$values) {
list($name, $op, $value, $grouping, $wildcard) = $values;
$op = "LIKE";
// security/core#28: hashed value serves as a unique, SQLi-safe table alias
$alias = hash('sha256', $value);
$value = "%{$value}%";
$escapedValue = CRM_Utils_Type::escape("%{$value}%", 'String');
$useAllTagTypes = $this->getWhereValues('all_tag_types', $grouping);
$tagTypesText = $this->getWhereValues('tag_types_text', $grouping);
$etTable = "`civicrm_entity_tag-" . $value . "`";
$tTable = "`civicrm_tag-" . $value . "`";
$etTable = "`civicrm_entity_tag-" . $alias . "`";
$tTable = "`civicrm_tag-" . $alias . "`";
if ($useAllTagTypes[2]) {
$this->_tables[$etTable] = $this->_whereTables[$etTable]
= " LEFT JOIN civicrm_entity_tag {$etTable} ON ( {$etTable}.entity_id = contact_a.id)
LEFT JOIN civicrm_tag {$tTable} ON ( {$etTable}.tag_id = {$tTable}.id )";
// search tag in cases
$etCaseTable = "`civicrm_entity_case_tag-" . $value . "`";
$tCaseTable = "`civicrm_case_tag-" . $value . "`";
$etCaseTable = "`civicrm_entity_case_tag-" . $alias . "`";
$tCaseTable = "`civicrm_case_tag-" . $alias . "`";
$this->_tables[$etCaseTable] = $this->_whereTables[$etCaseTable]
= " LEFT JOIN civicrm_case_contact ON civicrm_case_contact.contact_id = contact_a.id
LEFT JOIN civicrm_case
@@ -3196,8 +3202,8 @@ public function tagSearch(&$values) {
LEFT JOIN civicrm_entity_tag {$etCaseTable} ON ( {$etCaseTable}.entity_table = 'civicrm_case' AND {$etCaseTable}.entity_id = civicrm_case.id )
LEFT JOIN civicrm_tag {$tCaseTable} ON ( {$etCaseTable}.tag_id = {$tCaseTable}.id )";
// search tag in activities
$etActTable = "`civicrm_entity_act_tag-" . $value . "`";
$tActTable = "`civicrm_act_tag-" . $value . "`";
$etActTable = "`civicrm_entity_act_tag-" . $alias . "`";
$tActTable = "`civicrm_act_tag-" . $alias . "`";
$activityContacts = CRM_Activity_BAO_ActivityContact::buildOptions('record_type_id', 'validate');
$targetID = CRM_Utils_Array::key('Activity Targets', $activityContacts);
@@ -3210,12 +3216,12 @@ public function tagSearch(&$values) {
LEFT JOIN civicrm_entity_tag as {$etActTable} ON ( {$etActTable}.entity_table = 'civicrm_activity' AND {$etActTable}.entity_id = civicrm_activity.id )
LEFT JOIN civicrm_tag {$tActTable} ON ( {$etActTable}.tag_id = {$tActTable}.id )";
$this->_where[$grouping][] = "({$tTable}.name $op '" . $value . "' OR {$tCaseTable}.name $op '" . $value . "' OR {$tActTable}.name $op '" . $value . "')";
$this->_where[$grouping][] = "({$tTable}.name $op '" . $escapedValue . "' OR {$tCaseTable}.name $op '" . $escapedValue . "' OR {$tActTable}.name $op '" . $escapedValue . "')";
$this->_qill[$grouping][] = ts('Tag %1 %2', array(1 => $tagTypesText[2], 2 => $op)) . ' ' . $value;
}
else {
$etTable = "`civicrm_entity_tag-" . $value . "`";
$tTable = "`civicrm_tag-" . $value . "`";
$etTable = "`civicrm_entity_tag-" . $alias . "`";
$tTable = "`civicrm_tag-" . $alias . "`";
$this->_tables[$etTable] = $this->_whereTables[$etTable] = " LEFT JOIN civicrm_entity_tag {$etTable} ON ( {$etTable}.entity_id = contact_a.id AND
{$etTable}.entity_table = 'civicrm_contact' )
LEFT JOIN civicrm_tag {$tTable} ON ( {$etTable}.tag_id = {$tTable}.id ) ";
@@ -3243,20 +3249,31 @@ public function tag(&$values) {
if (count($value) > 1) {
$this->_useDistinct = TRUE;
}
$value = implode(',', (array) $value);
}
// implode array, then remove all spaces and validate CommaSeparatedIntegers
$value = CRM_Utils_Type::validate(
str_replace(' ', '', implode(',', (array) $value)),
'CommaSeparatedIntegers'
);
$useAllTagTypes = $this->getWhereValues('all_tag_types', $grouping);
$tagTypesText = $this->getWhereValues('tag_types_text', $grouping);
$etTable = "`civicrm_entity_tag-" . $value . "`";
$etTable = CRM_Utils_Type::escape(
str_replace(',', '-', "`civicrm_entity_tag-" . $value . "`"),
'MysqlColumnNameOrAlias'
);
if ($useAllTagTypes[2]) {
$this->_tables[$etTable] = $this->_whereTables[$etTable]
= " LEFT JOIN civicrm_entity_tag {$etTable} ON ( {$etTable}.entity_id = contact_a.id AND {$etTable}.entity_table = 'civicrm_contact') ";
// search tag in cases
$etCaseTable = "`civicrm_entity_case_tag-" . $value . "`";
$etCaseTable = CRM_Utils_Type::escape(
str_replace(',', '-', "`civicrm_entity_case_tag-" . $value . "`"),
'MysqlColumnNameOrAlias'
);
$activityContacts = CRM_Activity_BAO_ActivityContact::buildOptions('record_type_id', 'validate');
$targetID = CRM_Utils_Array::key('Activity Targets', $activityContacts);
@@ -3267,7 +3284,10 @@ public function tag(&$values) {
AND civicrm_case.is_deleted = 0 )
LEFT JOIN civicrm_entity_tag {$etCaseTable} ON ( {$etCaseTable}.entity_table = 'civicrm_case' AND {$etCaseTable}.entity_id = civicrm_case.id ) ";
// search tag in activities
$etActTable = "`civicrm_entity_act_tag-" . $value . "`";
$etActTable = CRM_Utils_Type::escape(
str_replace(',', '-', "`civicrm_entity_act_tag-" . $value . "`"),
'MysqlColumnNameOrAlias'
);
$this->_tables[$etActTable] = $this->_whereTables[$etActTable]
= " LEFT JOIN civicrm_activity_contact
ON ( civicrm_activity_contact.contact_id = contact_a.id AND civicrm_activity_contact.record_type_id = {$targetID} )
@@ -3929,4 +3929,69 @@ public function testCreateNoteinCreateArrayFormat() {
$this->assertEquals($note['values'][$note['id']]['note'], "Test note created by API Call as array");
}
/**
* Verify that passing tag IDs to Contact.get works
*
* Tests the following formats
* - Contact.get tag='id1'
* - Contact.get tag='id1,id2'
* - Contact.get tag='id1, id2'
*/
public function testContactGetWithTag() {
$contact = $this->callApiSuccess('Contact', 'create', [
'contact_type' => 'Individual',
'first_name' => 'Test',
'last_name' => 'Tagged',
'email' => 'test@example.org',
]);
$tags = [];
foreach (['Tag A', 'Tag B'] as $name) {
$tags[] = $this->callApiSuccess('Tag', 'create', [
'name' => $name
]);
}
// assign contact to "Tag B"
$this->callApiSuccess('EntityTag', 'create', [
'entity_table' => 'civicrm_contact',
'entity_id' => $contact['id'],
'tag_id' => $tags[1]['id'],
]);
// test format Contact.get tag='id1'
$contact_get = $this->callAPISuccess('Contact', 'get', [
'tag' => $tags[1]['id'],
'return' => 'tag',
]);
$this->assertEquals(1, $contact_get['count']);
$this->assertEquals($contact['id'], $contact_get['id']);
$this->assertEquals('Tag B', $contact_get['values'][$contact['id']]['tags']);
// test format Contact.get tag='id1,id2'
$contact_get = $this->callAPISuccess('Contact', 'get', [
'tag' => $tags[0]['id'] . ',' . $tags[1]['id'],
'return' => 'tag',
]);
$this->assertEquals(1, $contact_get['count']);
$this->assertEquals($contact['id'], $contact_get['id']);
$this->assertEquals('Tag B', $contact_get['values'][$contact['id']]['tags']);
// test format Contact.get tag='id1, id2'
$contact_get = $this->callAPISuccess('Contact', 'get', [
'tag' => $tags[0]['id'] . ', ' . $tags[1]['id'],
'return' => 'tag',
]);
$this->assertEquals(1, $contact_get['count']);
$this->assertEquals($contact['id'], $contact_get['id']);
$this->assertEquals('Tag B', $contact_get['values'][$contact['id']]['tags']);
foreach ($tags as $tag) {
$this->callAPISuccess('Tag', 'delete', ['id' => $tag['id']]);
}
$this->callAPISuccess('Contact', 'delete', [
'id' => $contact['id'],
'skip_undelete' => TRUE
]);
}
}

0 comments on commit 0cf0a3f

Please sign in to comment.
You can’t perform that action at this time.