From a380458d2e9072c1f5affa52ad259b38108e3ca6 Mon Sep 17 00:00:00 2001 From: iglocska Date: Fri, 13 Nov 2015 23:48:29 +0100 Subject: [PATCH 1/4] Fixed a security issue with the site admin file uploader - as discovered and reported by Egidio Romano of Minded Security - The site admin file upload tool allowed for unrestricted file upload that could lead to RCE - Fixed the file uploader to be much more restrictive - removed the interactive terms file upload --- app/Model/Server.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/Model/Server.php b/app/Model/Server.php index b4dac41c2..8438a8fb1 100755 --- a/app/Model/Server.php +++ b/app/Model/Server.php @@ -1408,11 +1408,11 @@ class Server extends AppModel { 'expected' => array(), 'valid_format' => '48x48 pixel .png files', 'path' => APP . 'webroot' . DS . 'img' . DS . 'orgs', - 'regex' => '.*\.(png|PNG)', + 'regex' => '.*\.(png|PNG)$', 'regex_error' => 'Filename must be in the following format: *.png', 'files' => array(), ), - 'terms' => array( + /*'terms' => array( 'name' => 'Terms of Use file', 'description' => 'Terms of use file viewable / downloadable by users. Make sure that it is either in text / html format if served inline.', 'expected' => array('MISP.terms_file' => Configure::read('MISP.terms_file')), @@ -1421,7 +1421,7 @@ class Server extends AppModel { 'regex' => '^(?!empty).*$', 'regex_error' => 'Filename can be any string consisting of characters between a-z, A-Z, 0-9 or one of the following: "_" or "-". The filename can also have an extension.', 'files' => array(), - ), + ),*/ 'img' => array( 'name' => 'Additional image files', 'description' => 'Image files uploaded into this directory can be used for various purposes, such as for the login page logos', @@ -1432,7 +1432,7 @@ class Server extends AppModel { ), 'valid_format' => 'text/html if served inline, anything that conveys the terms of use if served as download', 'path' => APP . 'webroot' . DS . 'img' . DS . 'custom', - 'regex' => '.*\.(png|PNG)', + 'regex' => '.*\.(png|PNG)$', 'regex_error' => 'Filename must be in the following format: *.png', 'files' => array(), ), From afdcc1af0c39c00d60e65c043c8216fb05b243fd Mon Sep 17 00:00:00 2001 From: iglocska Date: Fri, 13 Nov 2015 23:57:03 +0100 Subject: [PATCH 2/4] 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(); Date: Sat, 14 Nov 2015 00:03:10 +0100 Subject: [PATCH 3/4] Added an additional role to the default installation - by default there was no publisher role --- INSTALL/MYSQL.sql | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/INSTALL/MYSQL.sql b/INSTALL/MYSQL.sql index b27434e30..868990b51 100644 --- a/INSTALL/MYSQL.sql +++ b/INSTALL/MYSQL.sql @@ -528,15 +528,18 @@ VALUES ('2', 'Org Admin', NOW() , NOW() , '1', '1', '1' , '1', '1', '1', '1', '0 INSERT INTO `roles` (`id` ,`name` ,`created` ,`modified` ,`perm_add` ,`perm_modify` ,`perm_modify_org` ,`perm_publish` ,`perm_sync` ,`perm_admin` ,`perm_audit` ,`perm_full` ,`perm_auth`, `perm_regexp_access`, `perm_tagger`, `perm_site_admin`) VALUES ('3', 'User', NOW() , NOW() , '1', '1', '1' , '0' , '0' , '0' , '0' , '0' , '0', '0', '0', '0'); +INSERT INTO `roles` (`id` ,`name` ,`created` ,`modified` ,`perm_add` ,`perm_modify` ,`perm_modify_org` ,`perm_publish` ,`perm_sync` ,`perm_admin` ,`perm_audit` ,`perm_full` ,`perm_auth`, `perm_regexp_access`, `perm_tagger`, `perm_site_admin`) +VALUES ('4', 'Publisher', NOW() , NOW() , '1', '1', '1' , '1' , '0' , '0' , '0' , '0' , '0', '0', '0', '0'); + INSERT INTO `roles` (`id`, `name`, `created`, `modified`, `perm_add`, `perm_modify`, `perm_modify_org`, `perm_publish`, `perm_sync`, `perm_admin`, `perm_audit`, `perm_full`, `perm_auth`, `perm_regexp_access`, `perm_tagger`, `perm_site_admin`) -VALUES ('4', 'Sync user', NOW(), NOW(), '1', '1', '1', '1', '1', '0', '0', '0', '1', '0', '0', '0'); +VALUES ('5', 'Sync user', NOW(), NOW(), '1', '1', '1', '1', '1', '0', '0', '0', '1', '0', '0', '0'); INSERT INTO `roles` (`id`, `name`, `created`, `modified`, `perm_add`, `perm_modify`, `perm_modify_org`, `perm_publish`, `perm_sync`, `perm_admin`, `perm_audit`, `perm_full`, `perm_auth`, `perm_regexp_access`, `perm_tagger`, `perm_site_admin`) -VALUES ('5', 'Automation user', NOW(), NOW(), '1', '1', '1', '1', '0', '0', '0', '0', '1', '0', '0', '0'); +VALUES ('6', 'Automation user', NOW(), NOW(), '1', '1', '1', '1', '0', '0', '0', '0', '1', '0', '0', '0'); INSERT INTO `roles` (`id`, `name`, `created`, `modified`, `perm_add`, `perm_modify`, `perm_modify_org`, `perm_publish`, `perm_sync`, `perm_admin`, `perm_audit`, `perm_full`, `perm_auth`, `perm_regexp_access`, `perm_tagger`, `perm_site_admin`) -VALUES ('6', 'Read Only', NOW(), NOW(), '0', '0', '0', '0', '0', '0', '0', '0', '0', '0', '0', '0'); +VALUES ('7', 'Read Only', NOW(), NOW(), '0', '0', '0', '0', '0', '0', '0', '0', '0', '0', '0', '0'); -- -------------------------------------------------------- From 697ff43465f18de8141a54f4ec2aeb80265864e5 Mon Sep 17 00:00:00 2001 From: iglocska Date: Sat, 14 Nov 2015 00:03:41 +0100 Subject: [PATCH 4/4] Version bump --- VERSION.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION.json b/VERSION.json index ef54516ac..1ebf250ec 100644 --- a/VERSION.json +++ b/VERSION.json @@ -1 +1 @@ -{"major":2, "minor":3, "hotfix":157} +{"major":2, "minor":3, "hotfix":158}