From 77f99f7c3ee3299339a5ee285064a4208eeeb22a Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Fri, 2 Jul 2021 11:16:18 +0200 Subject: [PATCH 1/2] new: [test] Security test for publishing events --- tests/testlive_security.py | 113 +++++++++++++++++++++++++++++++------ 1 file changed, 96 insertions(+), 17 deletions(-) diff --git a/tests/testlive_security.py b/tests/testlive_security.py index 254e9fe6c..8237c87dc 100644 --- a/tests/testlive_security.py +++ b/tests/testlive_security.py @@ -5,6 +5,7 @@ import time import json import datetime import unittest +from unittest.util import safe_repr from typing import Union, List, Optional import urllib3 # type: ignore import logging @@ -39,6 +40,7 @@ class ROLE(Enum): ADMIN = 1 ORG_ADMIN = 2 USER = 3 + PUBLISHER = 4 SYNC_USER = 5 @@ -387,7 +389,7 @@ class TestSecurity(unittest.TestCase): self.__delete_advanced_authkey(auth_key["id"]) - self.__assertErrorResponse(logged_in.get_user()) + self.assertErrorResponse(logged_in.get_user()) def test_advanced_authkeys_deleted_keep_session(self): with self.__setting({ @@ -453,7 +455,7 @@ class TestSecurity(unittest.TestCase): # Reset auth key for different user new_auth_key = send(logged_in, "POST", "users/resetauthkey/1", check_errors=False) - self.__assertErrorResponse(new_auth_key) + self.assertErrorResponse(new_auth_key) # Try to login again logged_in = PyMISP(url, auth_key["authkey_raw"]) @@ -805,7 +807,7 @@ class TestSecurity(unittest.TestCase): user.org_id = self.test_org.id user.role_id = 3 created_user = self.org_admin_misp_connector.add_user(user) - self.__assertErrorResponse(created_user) + self.assertErrorResponse(created_user) def test_change_user_org_by_org_admin_different_org(self): updated_user = self.org_admin_misp_connector.update_user({'org_id': 1}, self.test_usr) @@ -1168,6 +1170,68 @@ class TestSecurity(unittest.TestCase): self.assertIn("X-Username", response.headers) self.assertEqual(f"{self.test_usr.email}/API/{auth_key['id']}", response.headers["X-Username"]) + def test_event_publish_no_perm(self): + test_usr = self.__login(self.test_usr) + + created_event = test_usr.add_event(self.__generate_event()) + self.assertSuccessfulResponse(created_event, "User should be able to create event") + + access_event = test_usr.get_event(created_event) + self.assertSuccessfulResponse(access_event, "User should be able to access that event") + + published = test_usr.publish(access_event) + self.assertErrorResponse(published, "User should not be able to publish that event without perm_publish permission") + + published = test_usr.publish(access_event, alert=True) + self.assertErrorResponse(published, "User should not be able to publish that event without perm_publish permission") + + self.assertSuccessfulResponse(test_usr.delete_event(access_event), "User should be able to delete his event") + + def test_event_publish_with_perm(self): + publisher_user = self.__create_user(self.test_org.id, ROLE.PUBLISHER) + logged_in = self.__login(publisher_user) + + created_event = logged_in.add_event(self.__generate_event()) + self.assertSuccessfulResponse(created_event, "User should be able to create event") + + access_event = logged_in.get_event(created_event) + self.assertSuccessfulResponse(access_event, "User should be able to access that event") + + published = logged_in.publish(access_event) + self.assertSuccessfulResponse(published, "User should be able to publish that event without perm_publish permission") + + published = logged_in.publish(access_event, alert=True) + self.assertSuccessfulResponse(published, "User should be able to publish (alert) that event without perm_publish permission") + + self.assertSuccessfulResponse(logged_in.delete_event(access_event), "User should be able to delete his event") + + # Cleanup + self.admin_misp_connector.delete_user(publisher_user) + + def test_event_publish_different_org(self): + different_org = self.__create_org() + publisher_user = self.__create_user(different_org.id, ROLE.PUBLISHER) + logged_in = self.__login(publisher_user) + + test_usr = self.__login(self.test_usr) + + created_event = test_usr.add_event(self.__generate_event()) + self.assertSuccessfulResponse(created_event, "User should be able to create event") + + access_event = test_usr.get_event(created_event) + self.assertSuccessfulResponse(access_event, "User should be able to access that event") + + published = logged_in.publish(created_event) + self.assertErrorResponse(published, "User from different org should not be able to publish that event") + + published = logged_in.publish(created_event, alert=True) + self.assertErrorResponse(published, "User from different org should not be able to publish that event") + + # Cleanup + test_usr.delete_event(created_event) + self.admin_misp_connector.delete_user(publisher_user) + self.admin_misp_connector.delete_organisation(different_org) + def test_sg_index_user_cannot_see(self): org = self.__create_org() hidden_sg = self.__create_sharing_group() @@ -1221,9 +1285,9 @@ class TestSecurity(unittest.TestCase): with self.assertRaises(Exception): send(logged_in, "POST", f"/sharingGroups/edit/{hidden_sg.uuid}", {"name": "New name2"}) - self.__assertErrorResponse(logged_in.add_org_to_sharing_group(hidden_sg, self.test_org.uuid)) - self.__assertErrorResponse(logged_in.remove_org_from_sharing_group(hidden_sg, org.uuid)) - self.__assertErrorResponse(logged_in.delete_sharing_group(hidden_sg)) + self.assertErrorResponse(logged_in.add_org_to_sharing_group(hidden_sg, self.test_org.uuid)) + self.assertErrorResponse(logged_in.remove_org_from_sharing_group(hidden_sg, org.uuid)) + self.assertErrorResponse(logged_in.delete_sharing_group(hidden_sg)) self.admin_misp_connector.delete_sharing_group(hidden_sg) self.admin_misp_connector.delete_organisation(org) @@ -1260,9 +1324,9 @@ class TestSecurity(unittest.TestCase): send(logged_in, "POST", f"/sharingGroups/edit/{sg.uuid}", {"name": "New name2"}) self.assertEqual(sg.name, send(logged_in, "GET", f"/sharingGroups/view/{sg.id}")["SharingGroup"]["name"]) - self.__assertErrorResponse(logged_in.add_org_to_sharing_group(sg, self.test_org.uuid)) - self.__assertErrorResponse(logged_in.remove_org_from_sharing_group(sg, org.uuid)) - self.__assertErrorResponse(logged_in.delete_sharing_group(sg)) + self.assertErrorResponse(logged_in.add_org_to_sharing_group(sg, self.test_org.uuid)) + self.assertErrorResponse(logged_in.remove_org_from_sharing_group(sg, org.uuid)) + self.assertErrorResponse(logged_in.delete_sharing_group(sg)) self.admin_misp_connector.delete_sharing_group(sg) self.admin_misp_connector.delete_user(sync_user) @@ -1283,7 +1347,7 @@ class TestSecurity(unittest.TestCase): after_edit = send(logged_in, "POST", f"/sharingGroups/edit/{sg.uuid}", {"name": "New name2"}) self.assertEqual("New name2", after_edit["SharingGroup"]["name"]) - self.__assertErrorResponse(logged_in.delete_sharing_group(sg)) + self.assertErrorResponse(logged_in.delete_sharing_group(sg)) self.admin_misp_connector.delete_sharing_group(sg) self.admin_misp_connector.delete_user(sync_user) @@ -1305,8 +1369,8 @@ class TestSecurity(unittest.TestCase): org = self.__create_org() with self.__setting("Security.hide_organisation_index_from_users", True): logged_in = PyMISP(url, self.test_usr.authkey) - self.__assertErrorResponse(logged_in.get_organisation(org.id)) - self.__assertErrorResponse(logged_in.get_organisation(org.uuid)) + self.assertErrorResponse(logged_in.get_organisation(org.id)) + self.assertErrorResponse(logged_in.get_organisation(org.uuid)) self.admin_misp_connector.delete_organisation(org) @@ -1335,7 +1399,7 @@ class TestSecurity(unittest.TestCase): with self.__setting("Security.hide_organisation_index_from_users", True): logged_in = PyMISP(url, self.test_usr.authkey) for key in (org.id, org.uuid, org.name): - self.__assertErrorResponse(logged_in.get_organisation(key)) + self.assertErrorResponse(logged_in.get_organisation(key)) self.admin_misp_connector.delete_event(event) self.admin_misp_connector.delete_user(user) @@ -1511,6 +1575,13 @@ class TestSecurity(unittest.TestCase): assert user_id == auth_key["user_id"], "Key was created for different user" return auth_key + def __login(self, user: MISPUser) -> PyMISP: + logged_in = PyMISP(url, user.authkey) + self.assertEqual(logged_in._current_user.id, user.id, "Logged in by different user") + if int(user.role_id) == ROLE.PUBLISHER.value: + self.assertTrue(logged_in._current_role.perm_publish, "Publisher user should have permission to publish events") + return logged_in + def __login_by_advanced_authkey(self, auth_key: dict) -> PyMISP: logged_in = PyMISP(url, auth_key["authkey_raw"]) self.assertEqual(logged_in._current_user.id, auth_key["user_id"], "Logged in by different user") @@ -1524,6 +1595,18 @@ class TestSecurity(unittest.TestCase): check_response(response) return response + def assertSuccessfulResponse(self, response, msg=None): + self.assertIsInstance(response, dict) + if "errors" in response: + msg = self._formatMessage(msg, safe_repr(response["errors"])) + self.fail(msg) + + def assertErrorResponse(self, response, msg=None): + self.assertIsInstance(response, dict) + if "errors" not in response: + msg = self._formatMessage(msg, safe_repr(response)) + self.fail(msg) + def __setting(self, key, value=None) -> MISPSetting: if not isinstance(key, dict): new_setting = {key: value} @@ -1531,10 +1614,6 @@ class TestSecurity(unittest.TestCase): new_setting = key return MISPSetting(self.admin_misp_connector, new_setting) - def __assertErrorResponse(self, response): - if "errors" not in response: - self.fail(response) - def __default_shibb_config(self) -> dict: return { "ApacheShibbAuth": { From 2a7ca1844245ce9bb2f9cc7162d6f593e6c70ccf Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Wed, 7 Jul 2021 15:25:33 +0200 Subject: [PATCH 2/2] chg: [API] Refactor event publishing --- .../Component/RestResponseComponent.php | 5 +- app/Controller/EventsController.php | 134 ++++++++---------- 2 files changed, 64 insertions(+), 75 deletions(-) diff --git a/app/Controller/Component/RestResponseComponent.php b/app/Controller/Component/RestResponseComponent.php index 60b90e92e..d11626d67 100644 --- a/app/Controller/Component/RestResponseComponent.php +++ b/app/Controller/Component/RestResponseComponent.php @@ -470,7 +470,6 @@ class RestResponseComponent extends Component public function saveFailResponse($controller, $action, $id = false, $validationErrors, $format = false, $data = null) { - $this->autoRender = false; $response = array(); $action = $this->__dissectAdminRouting($action); $stringifiedAction = $action['action']; @@ -480,7 +479,7 @@ class RestResponseComponent extends Component $response['saved'] = false; $response['name'] = 'Could not ' . $stringifiedAction . ' ' . Inflector::singularize($controller); $response['message'] = $response['name']; - if (!is_null($data)) { + if ($data !== null) { $response['data'] = $data; } $response['url'] = $this->__generateURL($action, $controller, $id); @@ -498,7 +497,7 @@ class RestResponseComponent extends Component $response['success'] = true; $response['name'] = $message; $response['message'] = $response['name']; - if (!is_null($data)) { + if ($data !== null) { $response['data'] = $data; } $response['url'] = $this->__generateURL($action, $controller, $id); diff --git a/app/Controller/EventsController.php b/app/Controller/EventsController.php index 26d389dc2..b55981684 100644 --- a/app/Controller/EventsController.php +++ b/app/Controller/EventsController.php @@ -2733,65 +2733,43 @@ class EventsController extends AppController // Publishes the event without sending an alert email public function publish($id = null) { - $id = $this->Toolbox->findIdByUuid($this->Event, $id); - $this->Event->id = $id; - // update the event and set the from field to the current instance's organisation from the bootstrap. We also need to save id and info for the logs. - $this->Event->recursive = -1; - $event = $this->Event->read(null, $id); - if (!$this->_isSiteAdmin()) { - if (!$this->userRole['perm_publish'] || $this->Auth->user('org_id') !== $this->Event->data['Event']['orgc_id']) { - throw new MethodNotAllowedException(__('You do not have the permission to do that.')); - } - } - $this->Event->insertLock($this->Auth->user(), $id); - $success = true; - $message = ''; - $errors = array(); + $event = $this->__prepareForPublish($id); + // only allow form submit CSRF protection. if ($this->request->is('post') || $this->request->is('put')) { - if (!$this->_isRest()) { - $publishable = $this->Event->checkIfPublishable($id); - if ($publishable !== true) { - $this->Flash->error(__('Could not publish event - no tag for required taxonomies missing: %s', implode(', ', $publishable))); - $this->redirect(array('action' => 'view', $id)); - } - } + $errors = array(); // Performs all the actions required to publish an event - $result = $this->Event->publishRouter($id, null, $this->Auth->user()); + $result = $this->Event->publishRouter($event['Event']['id'], null, $this->Auth->user()); if (!Configure::read('MISP.background_jobs')) { if (!is_array($result)) { // redirect to the view event page - $message = 'Event published without alerts'; + $message = __('Event published without alerts'); } else { $lastResult = array_pop($result); $resultString = (count($result) > 0) ? implode(', ', $result) . ' and ' . $lastResult : $lastResult; $errors['failed_servers'] = $result; - $message = sprintf('Event published but not pushed to %s, re-try later. If the issue persists, make sure that the correct sync user credentials are used for the server link and that the sync user on the remote server has authentication privileges.', $resultString); + $message = __('Event published but not pushed to %s, re-try later. If the issue persists, make sure that the correct sync user credentials are used for the server link and that the sync user on the remote server has authentication privileges.', $resultString); } } else { // update the DB to set the published flag // for background jobs, this should be done already - $fieldList = array('published', 'id', 'info', 'publish_timestamp'); $event['Event']['published'] = 1; $event['Event']['publish_timestamp'] = time(); - $this->Event->save($event, array('fieldList' => $fieldList)); + $this->Event->save($event, true, ['id', 'published', 'publish_timestamp', 'info']); // info field is required because of SysLogLogableBehavior $message = 'Job queued'; } if ($this->_isRest()) { - $this->set('name', 'Publish'); - $this->set('message', $message); if (!empty($errors)) { - $this->set('errors', $errors); + return $this->RestResponse->saveFailResponse('Events', 'publish', $event['Event']['id'], $errors); + } else { + return $this->RestResponse->saveSuccessResponse('Events', 'publish', $event['Event']['id'], false, $message); } - $this->set('url', $this->baseurl . '/events/alert/' . $id); - $this->set('id', $id); - $this->set('_serialize', array('name', 'message', 'url', 'id', 'errors')); } else { $this->Flash->success($message); - $this->redirect(array('action' => 'view', $id)); + $this->redirect(array('action' => 'view', $event['Event']['id'])); } } else { - $this->set('id', $id); + $this->set('id', $event['Event']['id']); $this->set('type', 'publish'); $this->render('ajax/eventPublishConfirmationForm'); } @@ -2801,34 +2779,16 @@ class EventsController extends AppController // Users with a GnuPG key will get the mail encrypted, other users will get the mail unencrypted public function alert($id = null) { - $id = $this->Toolbox->findIdByUuid($this->Event, $id); - $this->Event->id = $id; - $this->Event->recursive = 0; - if (!$this->Event->exists()) { - throw new NotFoundException(__('Invalid event')); - } - $this->Event->recursive = -1; - $this->Event->read(null, $id); - if (!$this->_isSiteAdmin()) { - if (!$this->userRole['perm_publish'] || $this->Auth->user('org_id') !== $this->Event->data['Event']['orgc_id']) { - throw new MethodNotAllowedException(__('You do not have the permission to do that.')); - } - } - $errors = array(); + $event = $this->__prepareForPublish($id); + // only allow form submit CSRF protection if ($this->request->is('post') || $this->request->is('put')) { - if (!$this->_isRest()) { - $publishable = $this->Event->checkIfPublishable($id); - if ($publishable !== true) { - $this->Flash->error(__('Could not publish event - no tag for required taxonomies missing: %s', implode(', ', $publishable))); - $this->redirect(array('action' => 'view', $id)); - } - } + $errors = array(); // send out the email - $emailResult = $this->Event->sendAlertEmailRouter($id, $this->Auth->user(), $this->Event->data['Event']['publish_timestamp']); + $emailResult = $this->Event->sendAlertEmailRouter($event['Event']['id'], $this->Auth->user(), $event['Event']['publish_timestamp']); if (is_bool($emailResult) && $emailResult == true) { // Performs all the actions required to publish an event - $result = $this->Event->publishRouter($id, null, $this->Auth->user()); + $result = $this->Event->publishRouter($event['Event']['id'], null, $this->Auth->user()); if (!is_array($result)) { // redirect to the view event page if (Configure::read('MISP.background_jobs')) { @@ -2840,52 +2800,82 @@ class EventsController extends AppController $lastResult = array_pop($result); $resultString = (count($result) > 0) ? implode(', ', $result) . ' and ' . $lastResult : $lastResult; $errors['failed_servers'] = $result; - $failed = 1; - $message = sprintf('Not published given no connection to %s but email sent to all participants.', $resultString); + $message = __('Not published given no connection to %s but email sent to all participants.', $resultString); } } elseif (!is_bool($emailResult)) { // Performs all the actions required to publish an event - $result = $this->Event->publishRouter($id, null, $this->Auth->user()); + $result = $this->Event->publishRouter($event['Event']['id'], null, $this->Auth->user()); if (!is_array($result)) { // redirect to the view event page - $message = 'Published but no email sent given GnuPG is not configured.'; + $message = __('Published but no email sent given GnuPG is not configured.'); $errors['GnuPG'] = 'GnuPG not set up.'; } else { $lastResult = array_pop($result); $resultString = (count($result) > 0) ? implode(', ', $result) . ' and ' . $lastResult : $lastResult; $errors['failed_servers'] = $result; $errors['GnuPG'] = 'GnuPG not set up.'; - $failed = 1; - $message = sprintf('Not published given no connection to %s but no email sent given GnuPG is not configured.', $resultString); + $message = __('Not published given no connection to %s but no email sent given GnuPG is not configured.', $resultString); } } else { $message = 'Sending of email failed'; $errors['email'] = 'The sending of emails failed.'; } if ($this->_isRest()) { - $this->set('name', 'Alert'); - $this->set('message', $message); if (!empty($errors)) { - $this->set('errors', $errors); + return $this->RestResponse->saveFailResponse('Events', 'alert', $event['Event']['id'], $errors); + } else { + return $this->RestResponse->saveSuccessResponse('Events', 'alert', $event['Event']['id'], false, $message); } - $this->set('url', $this->baseurl . '/events/alert/' . $id); - $this->set('id', $id); - $this->set('_serialize', array('name', 'message', 'url', 'id', 'errors')); } else { - if (!empty($failed)) { + if (isset($errors['failed_servers'])) { $this->Flash->error($message); } else { $this->Flash->success($message); } - $this->redirect(array('action' => 'view', $id)); + $this->redirect(array('action' => 'view', $event['Event']['id'])); } } else { - $this->set('id', $id); + $this->set('id', $event['Event']['id']); $this->set('type', 'alert'); $this->render('ajax/eventPublishConfirmationForm'); } } + /** + * @param int|string $id Event ID or UUID + * @return array + */ + private function __prepareForPublish($id) + { + if (empty($id)) { + throw new NotFoundException(__('Invalid event.')); + } + $event = $this->Event->find('first', [ + 'conditions' => Validation::uuid($id) ? ['Event.uuid' => $id] : ['Event.id' => $id], + 'recursive' => -1, + 'fields' => ['id', 'info', 'publish_timestamp', 'orgc_id'], + ]); + if (empty($event)) { + throw new NotFoundException(__('Invalid event.')); + } + if (!$this->_isSiteAdmin() && $this->Auth->user('org_id') !== $event['Event']['orgc_id']) { + throw new MethodNotAllowedException(__('You do not have the permission to do that.')); + } + if (!$this->_isRest()) { + $this->Event->insertLock($this->Auth->user(), $event['Event']['id']); + + if ($this->request->is('post') || $this->request->is('put')) { + $publishable = $this->Event->checkIfPublishable($event['Event']['id']); + if ($publishable !== true) { + $this->Flash->error(__('Could not publish event - no tag for required taxonomies missing: %s', implode(', ', $publishable))); + $this->redirect(['action' => 'view', $event['Event']['id']]); + } + } + } + + return $event; + } + // Send out an contact email to the person who posted the event. // Users with a GnuPG key will get the mail encrypted, other users will get the mail unencrypted public function contact($id = null)