From afdcc1af0c39c00d60e65c043c8216fb05b243fd Mon Sep 17 00:00:00 2001 From: iglocska Date: Fri, 13 Nov 2015 23:57:03 +0100 Subject: [PATCH] Fixed a security issue with the CSRF protection being avoidable using some site admin functionality - as discovered and reported by Egidio Romano of Minded Security - Lacking checks of HTTP methods in some functionality could lead to a site admin uploading and executing malicious scripts - Tightened HTTP method verification across the board for actions that modify data - Turned some administrative tasks to POST only actions --- app/Controller/AppController.php | 8 ++++---- app/Controller/AttributesController.php | 6 +++--- app/Controller/EventsController.php | 19 ++---------------- app/Controller/RegexpController.php | 2 +- app/Controller/ServersController.php | 8 ++++---- app/Controller/ShadowAttributesController.php | 4 ++-- app/Controller/UsersController.php | 20 +++++++++---------- app/View/Elements/side_menu.ctp | 2 +- app/View/Pages/administration.ctp | 16 +++++++-------- 9 files changed, 34 insertions(+), 51 deletions(-) diff --git a/app/Controller/AppController.php b/app/Controller/AppController.php index 73d339c71..4e3f5d79c 100755 --- a/app/Controller/AppController.php +++ b/app/Controller/AppController.php @@ -362,7 +362,7 @@ class AppController extends Controller { } public function generateCount() { - if (!self::_isSiteAdmin()) throw new NotFoundException(); + if (!self::_isSiteAdmin() || !$this->request->is('post')) throw new NotFoundException(); // do one SQL query with the counts // loop over events, update in db $this->loadModel('Attribute'); @@ -382,7 +382,7 @@ class AppController extends Controller { } public function pruneDuplicateUUIDs() { - if (!$this->_isSiteAdmin()) throw new MethodNotAllowedException(); + if (!$this->_isSiteAdmin() || !$this->request->is('post')) throw new MethodNotAllowedException(); $this->LoadModel('Attribute'); $duplicates = $this->Attribute->find('all', array( 'fields' => array('Attribute.uuid', 'count(*) as occurance'), @@ -409,7 +409,7 @@ class AppController extends Controller { } public function removeDuplicateEvents() { - if (!$this->_isSiteAdmin()) throw new MethodNotAllowedException(); + if (!$this->_isSiteAdmin() || !$this->request->is('post')) throw new MethodNotAllowedException(); $this->LoadModel('Event'); $duplicates = $this->Event->find('all', array( 'fields' => array('Event.uuid', 'count(*) as occurance'), @@ -445,7 +445,7 @@ class AppController extends Controller { } public function updateDatabase($command) { - if (!$this->_isSiteAdmin()) throw new MethodNotAllowedException(); + if (!$this->_isSiteAdmin() || !$this->request->is('post')) throw new MethodNotAllowedException(); $this->loadModel('Server'); $this->Server->updateDatabase($command); $this->Session->setFlash('Done.'); diff --git a/app/Controller/AttributesController.php b/app/Controller/AttributesController.php index e5413a24b..c8448a6d1 100755 --- a/app/Controller/AttributesController.php +++ b/app/Controller/AttributesController.php @@ -923,7 +923,7 @@ class AttributesController extends AppController { } public function deleteSelected($id) { - if (!$this->request->is('post') && !$this->request->is('ajax')) { + if (!$this->request->is('post') || !$this->request->is('ajax')) { //if (!$this->request->is('post')) { throw new MethodNotAllowedException(); } @@ -1840,7 +1840,7 @@ class AttributesController extends AppController { } public function generateCorrelation() { - if (!self::_isSiteAdmin()) throw new NotFoundException(); + if (!self::_isSiteAdmin() || !$this->request->is('post')) throw new NotFoundException(); if (!Configure::read('MISP.background_jobs')) { $k = $this->Attribute->generateCorrelation(); $this->Session->setFlash(__('All done. ' . $k . ' attributes processed.')); @@ -2217,7 +2217,7 @@ class AttributesController extends AppController { } public function pruneOrphanedAttributes() { - if (!$this->_isSiteAdmin()) throw new MethodNotAllowedException('You are not authorised to do that.'); + if (!$this->_isSiteAdmin() || !$this->request->is('post')) throw new MethodNotAllowedException('You are not authorised to do that.'); $events = array_keys($this->Attribute->Event->find('list')); $orphans = $this->Attribute->find('list', array('conditions' => array('Attribute.event_id !=' => $events))); if (count($orphans) > 0) $this->Attribute->deleteAll(array('Attribute.event_id !=' => $events), false, true); diff --git a/app/Controller/EventsController.php b/app/Controller/EventsController.php index ada2c8c49..03116b760 100755 --- a/app/Controller/EventsController.php +++ b/app/Controller/EventsController.php @@ -2621,7 +2621,7 @@ class EventsController extends AppController { } public function create_dummy_event() { - if (!$this->_isSiteAdmin()) throw new MethodNotAllowedException('You don\'t have the privileges to access this.'); + if (!$this->_isSiteAdmin() || !$this->request->is('post')) throw new MethodNotAllowedException('You don\'t have the privileges to access this.'); $date = new DateTime(); $data['Event']['info'] = 'Test event showing every category-type combination'; $data['Event']['date'] = '2013-10-09'; @@ -2689,7 +2689,7 @@ class EventsController extends AppController { // for load testing, it's slow, execution time is set at 1 hour maximum public function create_massive_dummy_events() { - if (!$this->_isSiteAdmin()) throw new MethodNotAllowedException('You don\'t have the privileges to access this.'); + if (!$this->_isSiteAdmin() || !$this->request->is('post')) throw new MethodNotAllowedException('You don\'t have the privileges to access this.'); ini_set('max_execution_time', 3600); $this->Event->Behaviors->unload('SysLogLogable.SysLogLogable'); $date = new DateTime(); @@ -2797,21 +2797,6 @@ class EventsController extends AppController { $this->set('count', $count); } - - public function generateLocked() { - if (!self::_isSiteAdmin()) throw new NotFoundException(); - $toBeUpdated = $this->Event->generateLocked(); - $this->Session->setFlash('Events updated, '. $toBeUpdated . ' record(s) altered.'); - $this->redirect(array('controller' => 'pages', 'action' => 'display', 'administration')); - } - - public function generateThreatLevelFromRisk() { - if (!self::_isSiteAdmin()) throw new NotFoundException(); - $updated = $this->Event->generateThreatLevelFromRisk(); - $this->Session->setFlash('Events updated, '. $updated . ' record(s) altered.'); - $this->redirect(array('controller' => 'pages', 'action' => 'display', 'administration')); - } - public function addTag($id = false, $tag_id = false) { if (!$this->request->is('post')) { return new CakeResponse(array('body'=> json_encode(array('saved' => false, 'errors' => 'You don\'t have permission to do that.')), 'status'=>200)); diff --git a/app/Controller/RegexpController.php b/app/Controller/RegexpController.php index 3b1f74814..75fd65200 100755 --- a/app/Controller/RegexpController.php +++ b/app/Controller/RegexpController.php @@ -194,7 +194,7 @@ class RegexpController extends AppController { * */ public function admin_clean() { - if(!$this->_isSiteAdmin()) $this->redirect(array('controller' => 'regexp', 'action' => 'index', 'admin' => false)); + if(!$this->_isSiteAdmin() || !$this->request->is('post')) throw new MethodNotAllowedException('This action is only accessible via a POST request.'); $allRegexp = $this->Regexp->find('all'); $deletable = array(); $modifications = 0; diff --git a/app/Controller/ServersController.php b/app/Controller/ServersController.php index 9c5656bb2..6f6fe7a61 100755 --- a/app/Controller/ServersController.php +++ b/app/Controller/ServersController.php @@ -260,7 +260,7 @@ class ServersController extends AppController { } } - public function __saveCert($server, $id) { + private function __saveCert($server, $id) { $ext = ''; App::uses('File', 'Utility'); App::uses('Folder', 'Utility'); @@ -424,7 +424,7 @@ class ServersController extends AppController { } public function startWorker($type) { - if (!$this->_isSiteAdmin() || !$this->request->is('Post')) throw new MethodNotAllowedException(); + if (!$this->_isSiteAdmin() || !$this->request->is('post')) throw new MethodNotAllowedException(); $validTypes = array('default', 'email', 'scheduler', 'cache'); if (!in_array($type, $validTypes)) throw new MethodNotAllowedException('Invalid worker type.'); if ($type != 'scheduler') shell_exec(APP . 'Console' . DS . 'cake ' . DS . 'CakeResque.CakeResque start --interval 5 --queue ' . $type .' > /dev/null 2>&1 &'); @@ -433,7 +433,7 @@ class ServersController extends AppController { } public function stopWorker($pid) { - if (!$this->_isSiteAdmin() || !$this->request->is('Post')) throw new MethodNotAllowedException(); + if (!$this->_isSiteAdmin() || !$this->request->is('post')) throw new MethodNotAllowedException(); $this->Server->killWorker($pid, $this->Auth->user()); $this->redirect('/servers/serverSettings/workers'); } @@ -597,7 +597,7 @@ class ServersController extends AppController { } public function uploadFile($type) { - if (!$this->_isSiteAdmin()) throw new MethodNotAllowedException(); + if (!$this->_isSiteAdmin() || !$this->request->is('post')) throw new MethodNotAllowedException(); $validItems = $this->Server->getFileRules(); // Check if there were problems with the file upload diff --git a/app/Controller/ShadowAttributesController.php b/app/Controller/ShadowAttributesController.php index 61a6bf098..cb60b2f8b 100644 --- a/app/Controller/ShadowAttributesController.php +++ b/app/Controller/ShadowAttributesController.php @@ -1099,7 +1099,7 @@ class ShadowAttributesController extends AppController { } public function discardSelected($id) { - if (!$this->request->is('post') && !$this->request->is('ajax')) throw new MethodNotAllowedException(); + if (!$this->request->is('post') || !$this->request->is('ajax')) throw new MethodNotAllowedException(); // get a json object with a list of proposal IDs to be discarded // check each of them and return a json object with the successful discards and the failed ones. @@ -1135,7 +1135,7 @@ class ShadowAttributesController extends AppController { } public function acceptSelected($id) { - if (!$this->request->is('post') && !$this->request->is('ajax')) throw new MethodNotAllowedException(); + if (!$this->request->is('post') || !$this->request->is('ajax')) throw new MethodNotAllowedException(); // get a json object with a list of proposal IDs to be accepted // check each of them and return a json object with the successful accepts and the failed ones. diff --git a/app/Controller/UsersController.php b/app/Controller/UsersController.php index 8ed921932..f4e5e7eed 100755 --- a/app/Controller/UsersController.php +++ b/app/Controller/UsersController.php @@ -116,7 +116,7 @@ class UsersController extends AppController { $this->request->data = $this->User->data; } // XXX ACL roles - $this->extraLog("change_pw"); + $this->__extralog("change_pw"); $roles = $this->User->Role->find('list'); $this->set(compact('roles')); } @@ -406,13 +406,13 @@ class UsersController extends AppController { foreach (array_keys($this->request->data['User']) as $field) { if($field != 'password') array_push($fields, $field); } - // TODO Audit, extraLog, fields get orig + // TODO Audit, __extralog, fields get orig $fieldsOldValues = array(); foreach ($fields as $field) { if($field != 'confirm_password') array_push($fieldsOldValues, $this->User->field($field)); else array_push($fieldsOldValues, $this->User->field('password')); } - // TODO Audit, extraLog, fields get orig END + // TODO Audit, __extralog, fields get orig END if ("" != $this->request->data['User']['password']) $fields[] = 'password'; $fields[] = 'role_id'; @@ -425,7 +425,7 @@ class UsersController extends AppController { } } if ($this->User->save($this->request->data, true, $fields)) { - // TODO Audit, extraLog, fields compare + // TODO Audit, __extralog, fields compare // newValues to array $fieldsNewValues = array(); foreach ($fields as $field) { @@ -455,8 +455,8 @@ class UsersController extends AppController { $c++; } $fieldsResultStr = substr($fieldsResultStr, 2); - $this->extraLog("edit", "user", $fieldsResultStr); // TODO Audit, check: modify User - // TODO Audit, extraLog, fields compare END + $this->__extralog("edit", "user", $fieldsResultStr); // TODO Audit, check: modify User + // TODO Audit, __extralog, fields compare END $this->Session->setFlash(__('The user has been saved')); $this->_refreshAuth(); // in case we modify ourselves $this->redirect(array('action' => 'index')); @@ -495,7 +495,7 @@ class UsersController extends AppController { throw new NotFoundException(__('Invalid user')); } if ($this->User->delete()) { - $this->extraLog("delete", $fieldsDescrStr, ''); // TODO Audit, check: modify User + $this->__extralog("delete", $fieldsDescrStr, ''); // TODO Audit, check: modify User $this->Session->setFlash(__('User deleted')); $this->redirect(array('action' => 'index')); } @@ -505,7 +505,7 @@ class UsersController extends AppController { public function login() { if ($this->Auth->login()) { - $this->extraLog("login"); // TODO Audit, extraLog, check: customLog i.s.o. extraLog, no auth user?: $this->User->customLog('login', $this->Auth->user('id'), array('title' => '','user_id' => $this->Auth->user('id'),'email' => $this->Auth->user('email'),'org' => 'IN2')); + $this->__extralog("login"); // TODO Audit, __extralog, check: customLog i.s.o. __extralog, no auth user?: $this->User->customLog('login', $this->Auth->user('id'), array('title' => '','user_id' => $this->Auth->user('id'),'email' => $this->Auth->user('email'),'org' => 'IN2')); // TODO removed the auto redirect for now, due to security concerns - will look more into this // $this->redirect($this->Auth->redirectUrl()); $this->redirect(array('controller' => 'events', 'action' => 'index')); @@ -570,7 +570,7 @@ class UsersController extends AppController { public function logout() { if ($this->Session->check('Auth.User')) { // TODO session, user is logged in, so .. - $this->extraLog("logout"); // TODO Audit, extraLog, check: customLog i.s.o. extraLog, $this->User->customLog('logout', $this->Auth->user('id'), array()); + $this->__extralog("logout"); // TODO Audit, __extralog, check: customLog i.s.o. __extralog, $this->User->customLog('logout', $this->Auth->user('id'), array()); } $this->Session->setFlash(__('Good-Bye')); $this->redirect($this->Auth->logout()); @@ -730,7 +730,7 @@ class UsersController extends AppController { return $this->response; } - public function extraLog($action = null, $description = null, $fieldsResult = null) { // TODO move audit to AuditsController? + private function __extralog($action = null, $description = null, $fieldsResult = null) { // TODO move audit to AuditsController? // new data $userId = $this->Auth->user('id'); $model = 'User'; diff --git a/app/View/Elements/side_menu.ctp b/app/View/Elements/side_menu.ctp index 2167564e5..3b193167f 100755 --- a/app/View/Elements/side_menu.ctp +++ b/app/View/Elements/side_menu.ctp @@ -79,7 +79,7 @@
  • >Html->link('List Regexp', array('admin' => $isSiteAdmin, 'action' => 'index'));?>
  • >Html->link('New Regexp', array('admin' => true, 'action' => 'add'));?>
    • -
  • Html->link('Perform on existing', array('admin' => true, 'action' => 'clean'));?>
    • +
  • Form->postLink('Perform on existing', array('admin' => true, 'action' => 'clean'));?>
  • diff --git a/app/View/Pages/administration.ctp b/app/View/Pages/administration.ctp index 3e23c0773..d1d90674f 100644 --- a/app/View/Pages/administration.ctp +++ b/app/View/Pages/administration.ctp @@ -11,16 +11,14 @@ if (!$isSiteAdmin) exit();