From 50ed845ae6444c344f0bd7734012be521b8c928e Mon Sep 17 00:00:00 2001
From: Seamus Lee <seamuslee001@gmail.com>
Date: Sat, 30 Mar 2019 16:19:58 +1100
Subject: [PATCH] security/core#49 Ensure that only intergers are passed to the
 IN build options in address

Fix Rule checking and add a unit test

Add in unit test on building country_id options too

Add in a unit test for building county options with a state_province_id filter
---
 CRM/Core/BAO/Address.php             |  9 ++++++
 CRM/Utils/Rule.php                   |  2 ++
 tests/phpunit/api/v3/AddressTest.php | 43 ++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+)

diff --git a/CRM/Core/BAO/Address.php b/CRM/Core/BAO/Address.php
index b5ec97d748..d94ecf2800 100644
--- a/CRM/Core/BAO/Address.php
+++ b/CRM/Core/BAO/Address.php
@@ -1319,6 +1319,9 @@ SELECT is_primary,
           }
         }
         if (!empty($props['country_id'])) {
+          if (!CRM_Utils_Rule::commaSeparatedIntegers(implode(',', (array) $props['country_id']))) {
+            throw new CRM_Core_Exception(ts('Province limit or default country setting is incorrect'));
+          }
           $params['condition'] = 'country_id IN (' . implode(',', (array) $props['country_id']) . ')';
         }
         break;
@@ -1331,6 +1334,9 @@ SELECT is_primary,
         if ($context != 'get' && $context != 'validate') {
           $config = CRM_Core_Config::singleton();
           if (!empty($config->countryLimit) && is_array($config->countryLimit)) {
+            if (!CRM_Utils_Rule::commaSeparatedIntegers(implode(',', $config->countryLimit))) {
+              throw new CRM_Core_Exception(ts('Available Country setting is incorrect'));
+            }
             $params['condition'] = 'id IN (' . implode(',', $config->countryLimit) . ')';
           }
         }
@@ -1339,6 +1345,9 @@ SELECT is_primary,
       // Filter county list based on chosen state
       case 'county_id':
         if (!empty($props['state_province_id'])) {
+          if (!CRM_Utils_Rule::commaSeparatedIntegers(implode(',', (array) $props['state_province_id']))) {
+            throw new CRM_Core_Exception(ts('Can only accept Integers for state_province_id filtering'));
+          }
           $params['condition'] = 'state_province_id IN (' . implode(',', (array) $props['state_province_id']) . ')';
         }
         break;
diff --git a/CRM/Utils/Rule.php b/CRM/Utils/Rule.php
index 50d72d3835..940128d715 100644
--- a/CRM/Utils/Rule.php
+++ b/CRM/Utils/Rule.php
@@ -488,6 +488,8 @@ class CRM_Utils_Rule {
    */
   public static function commaSeparatedIntegers($value) {
     foreach (explode(',', $value) as $val) {
+      // Remove any Whitespace around the key.
+      $val = trim($val);
       if (!self::positiveInteger($val)) {
         return FALSE;
       }
diff --git a/tests/phpunit/api/v3/AddressTest.php b/tests/phpunit/api/v3/AddressTest.php
index bbbc21cf9a..308b012d60 100644
--- a/tests/phpunit/api/v3/AddressTest.php
+++ b/tests/phpunit/api/v3/AddressTest.php
@@ -479,4 +479,47 @@ class api_v3_AddressTest extends CiviUnitTestCase {
     $this->assertEquals($expectState, $created['state_province_id']);
   }
 
+  public function testBuildStateProvinceOptionsWithDodgyProvinceLimit() {
+    $provinceLimit = [1228, "abcd;ef"];
+    $this->callAPISuccess('setting', 'create', [
+     'provinceLimit' => $provinceLimit,
+    ]);
+    $result = $this->callAPIFailure('address', 'getoptions', ['field' => 'state_province_id']);
+    // confirm that we hit our error not a SQLI.
+    $this->assertEquals('Province limit or default country setting is incorrect', $result['error_message']);
+    $this->callAPISuccess('setting', 'create', [
+     'provinceLimit' => [1228],
+    ]);
+    // Now confirm with a correct province setting it works fine
+    $this->callAPISuccess('address', 'getoptions', ['field' => 'state_province_id']);
+  }
+
+  public function testBuildCountryWithDodgyCountryLimitSetting() {
+    $countryLimit = [1228, "abcd;ef"];
+    $this->callAPISuccess('setting', 'create', [
+     'countryLimit' => $countryLimit,
+    ]);
+    $result = $this->callAPIFailure('address', 'getoptions', ['field' => 'country_id']);
+    // confirm that we hit our error not a SQLI.
+    $this->assertEquals('Available Country setting is incorrect', $result['error_message']);
+    $this->callAPISuccess('setting', 'create', [
+     'countryLimit' => [1228],
+    ]);
+    // Now confirm with a correct province setting it works fine
+    $this->callAPISuccess('address', 'getoptions', ['field' => 'country_id']);
+  }
+
+  public function testBuildCountyWithDodgeStateProvinceFiltering() {
+    $result = $this->callAPIFailure('Address', 'getoptions', [
+      'field' => 'county_id',
+      'state_province_id' => "abcd;ef",
+    ]);
+    $this->assertEquals('Can only accept Integers for state_province_id filtering', $result['error_message']);
+    $goodResult = $this->callAPISuccess('Address', 'getoptions', [
+      'field' => 'county_id',
+      'state_province_id' => 1004,
+    ]);
+    $this->assertEquals('San Francisco', $goodResult['values'][4]);
+  }
+
 }
-- 
2.25.1