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).

diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php
index 92c47c34bff0c7352c8eeac0ddb31501b660b11a..c1800d1314d1e60f8274fc6992b6c7e0c19f9b35 100644 (file)
--- 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 fc46a7ac9a3d7890469f53ba4f5d15497a796e95..1d9e7061d638f4a314223201ccfe258e5f2affd4 100644 (file)
--- a/tests/phpunit/api/v3/ContactTest.php
+++ b/tests/phpunit/api/v3/ContactTest.php
@@ -3902,4 +3902,69 @@ class api_v3_ContactTest extends CiviUnitTestCase {
     $this->assertTrue($g3Contacts['count'] == 1);
   }
 
+  /**
+   * 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
+    ]);
+  }
+
 }