From b41b0dd712c5b9cddaaf5d2103566c56e17ce2ad Mon Sep 17 00:00:00 2001 From: iglocska Date: Sat, 19 Feb 2022 01:02:49 +0100 Subject: [PATCH 01/12] fix: [security] privilege escalation via user edit fixed - org admins could circumvent the role restrictions and elevate themselves to a site admin - as reported by Dawid Czarnecki from Zigrin Security --- src/Controller/UsersController.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/Controller/UsersController.php b/src/Controller/UsersController.php index 3d684a0..5f4e8eb 100644 --- a/src/Controller/UsersController.php +++ b/src/Controller/UsersController.php @@ -166,6 +166,12 @@ class UsersController extends AppController } return $data; }; + $params['beforeSave'] = function ($data) use ($currentUser, $validRoles) { + if (!in_array($data['role_id'], array_keys($validRoles))) { + throw new MethodNotAllowedException(__('You cannot assign the chosen role to a user.')); + } + return $data; + }; } } $this->CRUD->edit($id, $params); From 6e67a5b2392f7f1af06a3c7f14d75f696831d86f Mon Sep 17 00:00:00 2001 From: iglocska Date: Sat, 19 Feb 2022 01:21:29 +0100 Subject: [PATCH 02/12] fix: [security] Sharing group creation on behalf of other organisation fixed - org admin could create sharing groups on behalf of other organisations - can lead to misleading sharing groups being created - as reported by Dawid Czarnecki of Zigrin Security --- src/Controller/SharingGroupsController.php | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/Controller/SharingGroupsController.php b/src/Controller/SharingGroupsController.php index aa96cb4..e03aee5 100644 --- a/src/Controller/SharingGroupsController.php +++ b/src/Controller/SharingGroupsController.php @@ -37,10 +37,17 @@ class SharingGroupsController extends AppController public function add() { + $currentUser = $this->ACL->getUser(); $this->CRUD->add([ 'override' => [ 'user_id' => $this->ACL->getUser()['id'] - ] + ], + 'beforeSave' => function($data) use ($currentUser) { + if (!$currentUser['role']['perm_admin']) { + $data['organisation_id'] = $currentUser['organisation_id']; + } + return $data; + } ]); $dropdownData = [ 'organisation' => $this->getAvailableOrgForSg($this->ACL->getUser()) From 283299bf36e81acc2c582035a6135992c49ab496 Mon Sep 17 00:00:00 2001 From: iglocska Date: Sat, 19 Feb 2022 01:34:07 +0100 Subject: [PATCH 03/12] fix: [security] flood protection control enabled by default - as reported by Dawid Czarnecki from Zigrin Security --- src/Controller/UsersController.php | 2 +- src/Model/Table/SettingProviders/CerebrateSettingsProvider.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Controller/UsersController.php b/src/Controller/UsersController.php index 5f4e8eb..272b1ba 100644 --- a/src/Controller/UsersController.php +++ b/src/Controller/UsersController.php @@ -317,7 +317,7 @@ class UsersController extends AppController if (empty(Configure::read('security.registration.self-registration'))) { throw new UnauthorizedException(__('User self-registration is not open.')); } - if (!empty(Configure::read('security.registration.floodProtection'))) { + if (!Configure::check('security.registration.floodProtection') || Configure::read('security.registration.floodProtection')) { $this->FloodProtection->check('register'); } if ($this->request->is('post')) { diff --git a/src/Model/Table/SettingProviders/CerebrateSettingsProvider.php b/src/Model/Table/SettingProviders/CerebrateSettingsProvider.php index 330e589..a2102e5 100644 --- a/src/Model/Table/SettingProviders/CerebrateSettingsProvider.php +++ b/src/Model/Table/SettingProviders/CerebrateSettingsProvider.php @@ -301,7 +301,7 @@ class CerebrateSettingsProvider extends BaseSettingsProvider 'name' => __('Enable registration flood-protection'), 'type' => 'boolean', 'description' => __('Enabling this setting will only allow 5 registrations / IP address every 15 minutes (rolling time-frame).'), - 'default' => false, + 'default' => true, ], ] ], From 37457391585f59010b0936b9233a25bd32d79d31 Mon Sep 17 00:00:00 2001 From: iglocska Date: Sat, 19 Feb 2022 01:42:24 +0100 Subject: [PATCH 04/12] chg: [flood protection] Changed the description of the setting based on the used IP source - added a warning about the IP source setting affecting the efficacy of the flood protection in regards to an attacker being potentially able to spoof their IP - Warn the admin to make sure that the reverse proxy used (the main reason to use the alternate headers in the first place) needs to be configured to correctly overwrite the header - as reported by Dawid Czarnecki of Zigrin Security --- .../Table/SettingProviders/CerebrateSettingsProvider.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Model/Table/SettingProviders/CerebrateSettingsProvider.php b/src/Model/Table/SettingProviders/CerebrateSettingsProvider.php index a2102e5..908d9aa 100644 --- a/src/Model/Table/SettingProviders/CerebrateSettingsProvider.php +++ b/src/Model/Table/SettingProviders/CerebrateSettingsProvider.php @@ -8,6 +8,7 @@ require_once(APP . 'Model' . DS . 'Table' . DS . 'SettingProviders' . DS . 'Base use App\Settings\SettingsProvider\BaseSettingsProvider; use App\Settings\SettingsProvider\SettingValidator; +use Cake\Core\Configure; class CerebrateSettingsProvider extends BaseSettingsProvider { @@ -300,7 +301,9 @@ class CerebrateSettingsProvider extends BaseSettingsProvider 'security.registration.floodProtection' => [ 'name' => __('Enable registration flood-protection'), 'type' => 'boolean', - 'description' => __('Enabling this setting will only allow 5 registrations / IP address every 15 minutes (rolling time-frame).'), + 'description' => (Configure::check('security.logging.ip_source') && Configure::read('security.logging.ip_source') !== 'REMOTE_ADDR') ? + __('Enabling this setting will only allow 5 registrations / IP address every 15 minutes (rolling time-frame). WARNING: Be aware that you are not using REMOTE_ADDR (as configured via security.logging.ip_source) - this could lead to an attacker being able to spoof their IP and circumvent the flood protection. Only rely on the client IP if your reverse proxy in front of Cerebrate is properly setting this header.'): + __('Enabling this setting will only allow 5 registrations / IP address every 15 minutes (rolling time-frame).'), 'default' => true, ], ] From b046990153170612bbef1c86b7e2e4785d8ba3a6 Mon Sep 17 00:00:00 2001 From: iglocska Date: Sun, 20 Feb 2022 11:49:57 +0100 Subject: [PATCH 05/12] fix: [flood protection] default to REMOTE_ADDR if the selected default logging IP source header is not populated --- src/Controller/Component/FloodProtectionComponent.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/Controller/Component/FloodProtectionComponent.php b/src/Controller/Component/FloodProtectionComponent.php index 6fbd0ec..ff45590 100644 --- a/src/Controller/Component/FloodProtectionComponent.php +++ b/src/Controller/Component/FloodProtectionComponent.php @@ -17,6 +17,9 @@ class FloodProtectionComponent extends Component public function initialize(array $config): void { $ip_source = Configure::check('security.logging.ip_source') ? Configure::read('security.logging.ip_source') : 'REMOTE_ADDR'; + if (!isset($_SERVER[$ip_source])) { + $ip_source = 'REMOTE_ADDR'; + } $this->remote_ip = $_SERVER[$ip_source]; $temp = explode(PHP_EOL, $_SERVER[$ip_source]); if (count($temp) > 1) { From 495c4ee93ce0ef1c74fea6731ae08962c4e2b384 Mon Sep 17 00:00:00 2001 From: iglocska Date: Sun, 20 Feb 2022 12:07:06 +0100 Subject: [PATCH 06/12] fix: [security] XSS in the generic action template - a previously assumed internal url can have user input appended via the MISP local tool connector - requires a compromised connected MISP instance where a malicious administrator modifies the UUIDs of cerebrate relevant objects to JS payloads - as reported by Dawid Czarcnecki of Zigrin Security --- templates/element/genericElements/IndexTable/Fields/actions.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/element/genericElements/IndexTable/Fields/actions.php b/templates/element/genericElements/IndexTable/Fields/actions.php index c280aad..8a215c4 100644 --- a/templates/element/genericElements/IndexTable/Fields/actions.php +++ b/templates/element/genericElements/IndexTable/Fields/actions.php @@ -98,7 +98,7 @@ ); } $reload_url = !empty($action['reload_url']) ? $action['reload_url'] : $this->Url->build(['action' => 'index']); - $action['onclick'] = sprintf('UI.submissionModalForIndex(\'%s\', \'%s\', \'%s\')', $modal_url, $reload_url, $tableRandomValue); + $action['onclick'] = sprintf('UI.submissionModalForIndex(\'%s\', \'%s\', \'%s\')', h($modal_url), h($reload_url), h($tableRandomValue)); } echo sprintf( ' ', From 2ef2dbbe628c88669a1e6400477ef50b4c102306 Mon Sep 17 00:00:00 2001 From: iglocska Date: Sun, 20 Feb 2022 14:48:29 +0100 Subject: [PATCH 07/12] fix: [tests] changed assertion for authkey failure on insufficient privilege from 404 to 405 --- tests/TestCase/Api/AuthKeys/AddAuthKeyApiTest.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/TestCase/Api/AuthKeys/AddAuthKeyApiTest.php b/tests/TestCase/Api/AuthKeys/AddAuthKeyApiTest.php index 2ede468..afb836f 100644 --- a/tests/TestCase/Api/AuthKeys/AddAuthKeyApiTest.php +++ b/tests/TestCase/Api/AuthKeys/AddAuthKeyApiTest.php @@ -65,8 +65,7 @@ class AddAuthKeyApiTest extends TestCase ] ); - $this->assertResponseCode(404); - $this->addWarning('Should return 405 Method Not Allowed instead of 404 Not Found'); + $this->assertResponseCode(405); $this->assertDbRecordNotExists('AuthKeys', ['uuid' => $uuid]); } } From c005cb7f6665cbf55c93df8a9fb267846588d2c6 Mon Sep 17 00:00:00 2001 From: iglocska Date: Sun, 20 Feb 2022 14:56:21 +0100 Subject: [PATCH 08/12] fix: [error code] adding an authkey for a user you are not authorised to modify resulted in a 404 instead of a 405 --- src/Controller/AuthKeysController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Controller/AuthKeysController.php b/src/Controller/AuthKeysController.php index 9ed43c3..978810c 100644 --- a/src/Controller/AuthKeysController.php +++ b/src/Controller/AuthKeysController.php @@ -84,7 +84,7 @@ class AuthKeysController extends AppController 'displayOnSuccess' => 'authkey_display', 'beforeSave' => function($data) use ($users) { if (!in_array($data['user_id'], array_keys($users))) { - return false; + throw new MethodNotAllowedException(__('You are not authorised to do that.')); } return $data; } From e2bb58d3c7cff4e5ebad7591e78c9c21d3950c55 Mon Sep 17 00:00:00 2001 From: iglocska Date: Sun, 20 Feb 2022 15:00:15 +0100 Subject: [PATCH 09/12] fix: [flood protection] default to 127.0.0.1 if no remote_addr is set as we're dealing with a local CLI script --- src/Controller/Component/FloodProtectionComponent.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Controller/Component/FloodProtectionComponent.php b/src/Controller/Component/FloodProtectionComponent.php index ff45590..a52c964 100644 --- a/src/Controller/Component/FloodProtectionComponent.php +++ b/src/Controller/Component/FloodProtectionComponent.php @@ -20,6 +20,11 @@ class FloodProtectionComponent extends Component if (!isset($_SERVER[$ip_source])) { $ip_source = 'REMOTE_ADDR'; } + if (isset($_SERVER[$ip_source])) { + $this->remote_ip = $_SERVER[$ip_source]; + } else { + $this->remote_ip = '127.0.0.1'; + } $this->remote_ip = $_SERVER[$ip_source]; $temp = explode(PHP_EOL, $_SERVER[$ip_source]); if (count($temp) > 1) { From 3af0b0afc5c8ec767b3c666936907289b5552bff Mon Sep 17 00:00:00 2001 From: iglocska Date: Sun, 20 Feb 2022 15:02:07 +0100 Subject: [PATCH 10/12] fix: [misp connector] validations with notEmpty() deprecated, replaced with notEmptyString() --- src/Lib/default/local_tool_connectors/MispConnector.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Lib/default/local_tool_connectors/MispConnector.php b/src/Lib/default/local_tool_connectors/MispConnector.php index c872675..66f5024 100644 --- a/src/Lib/default/local_tool_connectors/MispConnector.php +++ b/src/Lib/default/local_tool_connectors/MispConnector.php @@ -132,9 +132,9 @@ class MispConnector extends CommonConnectorTools { return $validator ->requirePresence('url') - ->notEmpty('url', __('An URL must be provided')) + ->notEmptyString('url', __('An URL must be provided')) ->requirePresence('authkey') - ->notEmpty('authkey', __('An Authkey must be provided')) + ->notEmptyString('authkey', __('An Authkey must be provided')) ->lengthBetween('authkey', [40, 40], __('The authkey must be 40 character long')) ->boolean('skip_ssl'); } From 9245b2d720fa236bf417916a7e7ebcf63ee4fd8f Mon Sep 17 00:00:00 2001 From: iglocska Date: Sun, 20 Feb 2022 15:05:03 +0100 Subject: [PATCH 11/12] fix: [genericTemplates] delete template can be invoked without an ID --- templates/genericTemplates/delete.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/templates/genericTemplates/delete.php b/templates/genericTemplates/delete.php index f2b7449..e838a6b 100644 --- a/templates/genericTemplates/delete.php +++ b/templates/genericTemplates/delete.php @@ -18,7 +18,11 @@ $form = $this->element('genericElements/Form/genericForm', [ ]); $formHTML = sprintf('
%s
', $form); -$bodyMessage = !empty($deletionText) ? __($deletionText) : __('Are you sure you want to delete {0} #{1}?', h(Cake\Utility\Inflector::singularize($this->request->getParam('controller'))), h($id)); +if (!empty($id)) { + $bodyMessage = !empty($deletionText) ? __($deletionText) : __('Are you sure you want to delete {0} #{1}?', h(Cake\Utility\Inflector::singularize($this->request->getParam('controller'))), h($id)); +} else { + $bodyMessage = !empty($deletionText) ? __($deletionText) : __('Are you sure you want to delete the given {0}?', h(Cake\Utility\Inflector::singularize($this->request->getParam('controller')))); +} $bodyHTML = sprintf('%s%s', $formHTML, $bodyMessage); echo $this->Bootstrap->modal([ From b67c2214760b1b3609e45e1b4736e5bf88127df8 Mon Sep 17 00:00:00 2001 From: iglocska Date: Sun, 20 Feb 2022 15:07:58 +0100 Subject: [PATCH 12/12] fix: [copy pasta fail] left previous assignment in that is now superseeded by the if branch above --- src/Controller/Component/FloodProtectionComponent.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Controller/Component/FloodProtectionComponent.php b/src/Controller/Component/FloodProtectionComponent.php index a52c964..91668b6 100644 --- a/src/Controller/Component/FloodProtectionComponent.php +++ b/src/Controller/Component/FloodProtectionComponent.php @@ -25,7 +25,6 @@ class FloodProtectionComponent extends Component } else { $this->remote_ip = '127.0.0.1'; } - $this->remote_ip = $_SERVER[$ip_source]; $temp = explode(PHP_EOL, $_SERVER[$ip_source]); if (count($temp) > 1) { $this->remote_ip = $temp[0];