From 0cf0a3f39283cac6b7c45a059ffba09621e813e3 Mon Sep 17 00:00:00 2001 From: Patrick Figel Date: Sat, 27 Oct 2018 21:08:32 +0200 Subject: [PATCH] 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). --- CRM/Contact/BAO/Query.php | 50 ++++++++++++++------- tests/phpunit/api/v3/ContactTest.php | 65 ++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 15 deletions(-) diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php index 92c47c34bf..c1800d1314 100644 --- a/CRM/Contact/BAO/Query.php +++ b/CRM/Contact/BAO/Query.php @@ -2972,7 +2972,7 @@ class CRM_Contact_BAO_Query { $smartGroupIDs[] = $id; } else { - $regularGroupIDs[] = $id; + $regularGroupIDs[] = trim($id); } } @@ -3011,7 +3011,10 @@ class CRM_Contact_BAO_Query { 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,13 +3175,16 @@ WHERE $smartGroupClause 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] @@ -3186,8 +3192,8 @@ WHERE $smartGroupClause 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 @@ WHERE $smartGroupClause 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 @@ WHERE $smartGroupClause 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 @@ WHERE $smartGroupClause 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 @@ WHERE $smartGroupClause 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} ) diff --git a/tests/phpunit/api/v3/ContactTest.php b/tests/phpunit/api/v3/ContactTest.php index 3ae1d26020..f2f53c32ec 100644 --- a/tests/phpunit/api/v3/ContactTest.php +++ b/tests/phpunit/api/v3/ContactTest.php @@ -3929,4 +3929,69 @@ class api_v3_ContactTest extends CiviUnitTestCase { $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 + ]); + } + } -- 2.25.1