From 7f0d06ae4d148c540f69e4d73faeaaaeac388259 Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Thu, 12 Nov 2020 19:01:42 +0100 Subject: [PATCH 01/42] chg: [internal] Move user checks to one place --- app/Controller/AppController.php | 238 +++++++++++++++++++---------- app/Controller/NewsController.php | 17 +-- app/Controller/UsersController.php | 34 ++--- app/Model/User.php | 6 +- 4 files changed, 175 insertions(+), 120 deletions(-) diff --git a/app/Controller/AppController.php b/app/Controller/AppController.php index 8e1fdd3c9..cf26c5d78 100755 --- a/app/Controller/AppController.php +++ b/app/Controller/AppController.php @@ -333,33 +333,11 @@ class AppController extends Controller if ($this->Auth->user('Role')['perm_site_admin'] || (Configure::read('MISP.live') && !$this->_isRest())) { $this->{$this->modelClass}->runUpdates(); } - $user = $this->Auth->user(); - if (!isset($user['force_logout']) || $user['force_logout']) { - $this->loadModel('User'); - $this->User->id = $this->Auth->user('id'); - $this->User->saveField('force_logout', false); - } - if ($this->Auth->user('disabled')) { - $this->Log = ClassRegistry::init('Log'); - $this->Log->create(); - $log = array( - 'org' => $this->Auth->user('Organisation')['name'], - 'model' => 'User', - 'model_id' => $this->Auth->user('id'), - 'email' => $this->Auth->user('email'), - 'action' => 'auth_fail', - 'title' => 'Login attempt by disabled user.', - 'change' => null, - ); - $this->Log->save($log); - $this->Auth->logout(); - if ($this->_isRest()) { - throw new ForbiddenException('Authentication failed. Your user account has been disabled.'); - } else { - $this->Flash->error('Your user account has been disabled.', array('key' => 'error')); - $this->_redirectToLogin(); - } + + if (!$this->__verifyUser($this->User, $this->Auth->user())) { + $this->_stop(); // just for sure } + $this->set('default_memory_limit', ini_get('memory_limit')); if (isset($this->Auth->user('Role')['memory_limit'])) { if ($this->Auth->user('Role')['memory_limit'] !== '') { @@ -385,62 +363,6 @@ class AppController extends Controller } } - // check if MISP is live - if ($this->Auth->user() && !Configure::read('MISP.live')) { - $role = $this->getActions(); - if (!$role['perm_site_admin']) { - $message = Configure::read('MISP.maintenance_message'); - if (empty($message)) { - $this->loadModel('Server'); - $message = $this->Server->serverSettings['MISP']['maintenance_message']['value']; - } - if (strpos($message, '$email') && Configure::read('MISP.email')) { - $email = Configure::read('MISP.email'); - $message = str_replace('$email', $email, $message); - } - $this->Flash->info($message); - $this->Auth->logout(); - throw new MethodNotAllowedException($message);//todo this should pb be removed? - } else { - $this->Flash->error(__('Warning: MISP is currently disabled for all users. Enable it in Server Settings (Administration -> Server Settings -> MISP tab -> live). An update might also be in progress, you can see the progress in ') , array('params' => array('url' => $this->baseurl . '/servers/updateProgress/', 'urlName' => __('Update Progress')), 'clear' => 1)); - } - } - if ($this->Session->check(AuthComponent::$sessionKey)) { - if ($this->action !== 'checkIfLoggedIn' || $this->request->params['controller'] !== 'users') { - $this->User->id = $this->Auth->user('id'); - if (!$this->User->exists()) { - $message = __('Something went wrong. Your user account that you are authenticated with doesn\'t exist anymore.'); - if ($this->_isRest) { - echo $this->RestResponse->throwException( - 401, - $message - ); - } else { - $this->Flash->info($message); - } - $this->Auth->logout(); - $this->_redirectToLogin(); - } - if (!empty(Configure::read('MISP.terms_file')) && !$this->Auth->user('termsaccepted') && (!in_array($this->request->here, array($base_dir.'/users/terms', $base_dir.'/users/logout', $base_dir.'/users/login', $base_dir.'/users/downloadTerms')))) { - //if ($this->_isRest()) throw new MethodNotAllowedException('You have not accepted the terms of use yet, please log in via the web interface and accept them.'); - if (!$this->_isRest()) { - $this->redirect(array('controller' => 'users', 'action' => 'terms', 'admin' => false)); - } - } elseif ($this->Auth->user('change_pw') && (!in_array($this->request->here, array($base_dir.'/users/terms', $base_dir.'/users/change_pw', $base_dir.'/users/logout', $base_dir.'/users/login')))) { - //if ($this->_isRest()) throw new MethodNotAllowedException('Your user account is expecting a password change, please log in via the web interface and change it before proceeding.'); - if (!$this->_isRest()) { - $this->redirect(array('controller' => 'users', 'action' => 'change_pw', 'admin' => false)); - } - } elseif (!$this->_isRest() && !($this->params['controller'] == 'news' && $this->params['action'] == 'index') && (!in_array($this->request->here, array($base_dir.'/users/terms', $base_dir.'/users/change_pw', $base_dir.'/users/logout', $base_dir.'/users/login')))) { - $newsread = $this->User->field('newsread', array('User.id' => $this->Auth->user('id'))); - $this->loadModel('News'); - $latest_news = $this->News->field('date_created', array(), 'date_created DESC'); - if ($latest_news && $newsread < $latest_news) { - $this->redirect(array('controller' => 'news', 'action' => 'index', 'admin' => false)); - } - } - } - } unset($base_dir); // We don't want to run these role checks before the user is logged in, but we want them available for every view once the user is logged on // instead of using checkAction(), like we normally do from controllers when trying to find out about a permission flag, we can use getActions() @@ -571,6 +493,143 @@ class AppController extends Controller } } + /** + * Check if: + * - user exists in database + * - is not disabled + * - need to force logout + * - accepted terms and conditions + * - must change password + * - reads latest news + * + * @param User $userModel + * @param array $user + * @return bool + */ + private function __verifyUser(User $userModel, array $user) + { + // Skip these checks for 'checkIfLoggedIn' action to make that call fast + if ($this->_isControllerAction(['users' => ['checkIfLoggedIn']])) { + return true; + } + + // Load last user profile modification from database + $userFromDb = $userModel->find('first', [ + 'conditions' => ['id' => $user['id']], + 'recursive' => -1, + 'fields' => ['date_modified'], + ]); + + // Check if user with given ID exists + if (!$userFromDb) { + $message = __('Something went wrong. Your user account that you are authenticated with doesn\'t exist anymore.'); + if ($this->_isRest()) { + // TODO: Why not exception? + $response = $this->RestResponse->throwException(401, $message); + $response->send(); + $this->_stop(); + } else { + $this->Flash->info($message); + $this->Auth->logout(); + $this->_redirectToLogin(); + } + return false; + } + + // Check if session data contain latest changes from db + if ((int)$user['date_modified'] < (int)$userFromDb['User']['date_modified']) { + $user = $this->_refreshAuth(); // session data are old, reload from database + } + + // Check if MISP access is enabled + if (!Configure::read('MISP.live')) { + if (!$user['Role']['perm_site_admin']) { + $message = Configure::read('MISP.maintenance_message'); + if (empty($message)) { + $this->loadModel('Server'); + $message = $this->Server->serverSettings['MISP']['maintenance_message']['value']; + } + if (strpos($message, '$email') && Configure::read('MISP.email')) { + $email = Configure::read('MISP.email'); + $message = str_replace('$email', $email, $message); + } + $this->Flash->info($message); + $this->Auth->logout(); + throw new MethodNotAllowedException($message);//todo this should pb be removed? + } else { + $this->Flash->error(__('Warning: MISP is currently disabled for all users. Enable it in Server Settings (Administration -> Server Settings -> MISP tab -> live). An update might also be in progress, you can see the progress in ') , array('params' => array('url' => $this->baseurl . '/servers/updateProgress/', 'urlName' => __('Update Progress')), 'clear' => 1)); + } + } + + // Force logout doesn't make sense for API key authentication + if (!$this->isApiAuthed && $user['force_logout']) { + $userModel->id = $user['id']; + $userModel->saveField('force_logout', false); + $this->Auth->logout(); + $this->_redirectToLogin(); + return false; + } + + if ($user['disabled']) { + $this->Log = ClassRegistry::init('Log'); + $this->Log->createLogEntry($user, 'auth_fail', 'User', $user['id'], 'Login attempt by disabled user.'); + + $this->Auth->logout(); + if ($this->_isRest()) { + throw new ForbiddenException('Authentication failed. Your user account has been disabled.'); + } else { + $this->Flash->error(__('Your user account has been disabled.')); + $this->_redirectToLogin(); + } + return false; + } + + $isUserRequest = !$this->_isRest() && !$this->request->is('ajax'); + // Next checks makes sense just for user direct HTTP request, so skip REST and AJAX calls + if (!$isUserRequest) { + return true; + } + + // Check if user accepted terms and conditions + if (!$user['termsaccepted'] && !empty(Configure::read('MISP.terms_file')) && !$this->_isControllerAction(['users' => ['terms', 'logout', 'login', 'downloadTerms']])) { + //if ($this->_isRest()) throw new MethodNotAllowedException('You have not accepted the terms of use yet, please log in via the web interface and accept them.'); + $this->redirect(array('controller' => 'users', 'action' => 'terms', 'admin' => false)); + return false; + } + + // Check if user must change password + if ($user['change_pw'] && !$this->_isControllerAction(['users' => ['terms', 'change_pw', 'logout', 'login']])) { + //if ($this->_isRest()) throw new MethodNotAllowedException('Your user account is expecting a password change, please log in via the web interface and change it before proceeding.'); + $this->redirect(array('controller' => 'users', 'action' => 'change_pw', 'admin' => false)); + return false; + } + + // Check if user must read news + if (!$this->_isControllerAction(['news' => ['index'], 'users' => ['terms', 'change_pw', 'login', 'logout']])) { + $this->loadModel('News'); + $latestNewsCreated = $this->News->field('date_created', array(), 'date_created DESC'); + if ($latestNewsCreated && $user['newsread'] < $latestNewsCreated) { + $this->redirect(array('controller' => 'news', 'action' => 'index', 'admin' => false)); + return false; + } + } + + return true; + } + + /** + * @param array $actionsToCheck + * @return bool + */ + private function _isControllerAction($actionsToCheck = []) + { + $controller = Inflector::variable($this->request->params['controller']); + if (!isset($actionsToCheck[$controller])) { + return false; + } + return in_array($this->action, $actionsToCheck[$controller]); + } + private function __rateLimitCheck() { $info = array(); @@ -1331,4 +1390,19 @@ class AppController extends Controller } return false; } + + /** + * Refresh user data in session. + * @return array User data in auth format + */ + protected function _refreshAuth() + { + $userId = $this->Auth->user('id'); + $user = $this->User->getAuthUser($userId); + if (!$user){ + throw new RuntimeException("User with ID $userId not exists."); + } + $this->Auth->login($user); + return $user; + } } diff --git a/app/Controller/NewsController.php b/app/Controller/NewsController.php index 6a6ad0f7f..2601a3edd 100755 --- a/app/Controller/NewsController.php +++ b/app/Controller/NewsController.php @@ -17,24 +17,19 @@ class NewsController extends AppController { $this->paginate['contain'] = array('User' => array('fields' => array('User.email'))); $newsItems = $this->paginate(); - $this->loadModel('User'); - $currentUser = $this->User->find('first', array( - 'recursive' => -1, - 'conditions' => array('User.id' => $this->Auth->user('id')), - 'fields' => array('User.newsread') - )); + + $newsread = $this->Auth->user('newsread'); foreach ($newsItems as $key => $item) { - if ($item['News']['date_created'] > $currentUser['User']['newsread']) { + if ($item['News']['date_created'] > $newsread) { $newsItems[$key]['News']['new'] = true; } else { $newsItems[$key]['News']['new'] = false; } } - $this->User->id = $this->Auth->user('id'); - //if ($this->User->exists()) { - $this->User->saveField('newsread', time()); $this->set('newsItems', $newsItems); - //} + + $this->loadModel('User'); + $this->User->updateField($this->Auth->user(), 'newsread', time()); } public function add() diff --git a/app/Controller/UsersController.php b/app/Controller/UsersController.php index bd5c0f3c3..62bb96576 100644 --- a/app/Controller/UsersController.php +++ b/app/Controller/UsersController.php @@ -182,7 +182,7 @@ class UsersController extends AppController } if (!$abortPost) { // What fields should be saved (allowed to be saved) - $fieldList = array('autoalert', 'gpgkey', 'certif_public', 'nids_sid', 'contactalert', 'disabled'); + $fieldList = array('autoalert', 'gpgkey', 'certif_public', 'nids_sid', 'contactalert', 'disabled', 'date_modified'); if ($this->__canChangeLogin()) { $fieldList[] = 'email'; } @@ -217,7 +217,6 @@ class UsersController extends AppController return $this->RestResponse->viewData($this->__massageUserObject($user), $this->response->type()); } else { $this->Flash->success(__('The profile has been updated')); - $this->_refreshAuth(); $this->redirect(array('action' => 'view', $id)); } } else { @@ -305,7 +304,6 @@ class UsersController extends AppController return $this->RestResponse->saveSuccessResponse('User', 'change_pw', false, $this->response->type(), $message); } $this->Flash->success($message); - $this->_refreshAuth(); $this->redirect(array('action' => 'view', $id)); } else { $message = __('The password could not be updated. Make sure you meet the minimum password length / complexity requirements.'); @@ -915,8 +913,8 @@ class UsersController extends AppController if (isset($this->request->data['User']['role_id']) && !array_key_exists($this->request->data['User']['role_id'], $syncRoles)) { $this->request->data['User']['server_id'] = 0; } - $fields = array(); - $blockedFields = array('id', 'invited_by'); + $fields = []; + $blockedFields = array('id', 'invited_by', 'date_modified'); if (!$this->_isSiteAdmin()) { $blockedFields[] = 'org_id'; } @@ -967,11 +965,15 @@ class UsersController extends AppController throw new Exception('You are not authorised to assign that role to a user.'); } } - if (!empty($fields) && $this->User->save($this->request->data, true, $fields)) { + $fields[] = 'date_modified'; // time will be inserted in `beforeSave` action + if ($this->User->save($this->request->data, true, $fields)) { // newValues to array $fieldsNewValues = array(); foreach ($fields as $field) { - if ($field != 'confirm_password') { + if ($field === 'date_modified') { + continue; + } + if ($field !== 'confirm_password') { $newValue = $this->data['User'][$field]; if (gettype($newValue) == 'array') { $newValueStr = ''; @@ -1014,7 +1016,6 @@ class UsersController extends AppController return $this->RestResponse->viewData($user, $this->response->type()); } else { $this->Flash->success(__('The user has been saved')); - $this->_refreshAuth(); // in case we modify ourselves $this->redirect(array('action' => 'index')); } } else { @@ -1309,7 +1310,6 @@ class UsersController extends AppController } if (!$this->_isRest()) { $this->Flash->success(__('New authkey generated.', true)); - $this->_refreshAuth(); $this->redirect($this->referer()); } else { return $this->RestResponse->saveSuccessResponse('User', 'resetauthkey', $id, $this->response->type(), 'Authkey updated: ' . $newkey); @@ -1436,9 +1436,7 @@ class UsersController extends AppController public function terms() { if ($this->request->is('post') || $this->request->is('put')) { - $this->User->id = $this->Auth->user('id'); - $this->User->saveField('termsaccepted', true); - $this->_refreshAuth(); // refresh auth info + $this->User->updateField($this->Auth->user(), 'termsaccepted', true); $this->Flash->success(__('You accepted the Terms and Conditions.')); $this->redirect(array('action' => 'routeafterlogin')); } @@ -2278,18 +2276,6 @@ class UsersController extends AppController $this->set('users', $user_results); } - // Refreshes the Auth session with new/updated data - protected function _refreshAuth() - { - $oldUser = $this->Auth->user(); - $newUser = $this->User->find('first', array('conditions' => array('User.id' => $oldUser['id']), 'recursive' => -1,'contain' => array('Organisation', 'Role'))); - // Rearrange it a bit to match the Auth object created during the login - $newUser['User']['Role'] = $newUser['Role']; - $newUser['User']['Organisation'] = $newUser['Organisation']; - unset($newUser['Organisation'], $newUser['Role']); - $this->Auth->login($newUser['User']); - } - public function searchGpgKey($email = false) { if (!$email) { diff --git a/app/Model/User.php b/app/Model/User.php index eb888ebcb..1959c714e 100644 --- a/app/Model/User.php +++ b/app/Model/User.php @@ -967,7 +967,7 @@ class User extends AppModel if ($result) { $this->id = $user['User']['id']; $this->saveField('password', $password); - $this->saveField('change_pw', '1'); + $this->updateField($user['User'], 'change_pw', 1); if ($simpleReturn) { return true; } else { @@ -1142,7 +1142,7 @@ class User extends AppModel if (empty(Configure::read('Security.advanced_authkeys'))) { $oldKey = $this->data['User']['authkey']; $newkey = $this->generateAuthKey(); - $this->saveField('authkey', $newkey); + $this->updateField($updatedUser['User'], 'authkey', $newkey); $this->extralog( $user, 'reset_auth_key', @@ -1403,7 +1403,7 @@ class User extends AppModel $name => $value, ], true, ['id', $name, 'date_modified']); if (!$success) { - throw new RuntimeException("Could not save field `$name` with value `$value` for user `{$user['id']}`."); + throw new RuntimeException("Could not save setting $name for user {$user['id']}."); } } From 18402c04899b076625242e9c13b7effbfa5ac708 Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Fri, 13 Nov 2020 21:34:57 +0100 Subject: [PATCH 02/42] chg: [internal] Load user role info from session data --- app/Controller/AppController.php | 25 +---------------------- app/Controller/Component/ACLComponent.php | 2 -- 2 files changed, 1 insertion(+), 26 deletions(-) diff --git a/app/Controller/AppController.php b/app/Controller/AppController.php index cf26c5d78..76c321957 100755 --- a/app/Controller/AppController.php +++ b/app/Controller/AppController.php @@ -365,12 +365,10 @@ class AppController extends Controller unset($base_dir); // We don't want to run these role checks before the user is logged in, but we want them available for every view once the user is logged on - // instead of using checkAction(), like we normally do from controllers when trying to find out about a permission flag, we can use getActions() - // getActions returns all the flags in a single SQL query if ($this->Auth->user()) { $this->set('mispVersion', implode('.', array($versionArray['major'], $versionArray['minor'], 0))); $this->set('mispVersionFull', $this->mispVersion); - $role = $this->getActions(); + $role = $this->Auth->user('Role'); $this->set('me', $this->Auth->user()); $this->set('isAdmin', $role['perm_admin']); $this->set('isSiteAdmin', $role['perm_site_admin']); @@ -888,27 +886,6 @@ class AppController extends Controller return $data; } - // pass an action to this method for it to check the active user's access to the action - public function checkAction($action = 'perm_sync') - { - $this->loadModel('Role'); - $this->Role->recursive = -1; - $role = $this->Role->findById($this->Auth->user('role_id')); - if ($role['Role'][$action]) { - return true; - } - return false; - } - - // returns the role of the currently authenticated user as an array, used to set the permission variables for views in the AppController's beforeFilter() method - public function getActions() - { - $this->loadModel('Role'); - $this->Role->recursive = -1; - $role = $this->Role->findById($this->Auth->user('role_id')); - return $role['Role']; - } - public function checkAuthUser($authkey) { if (Configure::read('Security.advanced_authkeys')) { diff --git a/app/Controller/Component/ACLComponent.php b/app/Controller/Component/ACLComponent.php index 2fc74076c..8f0753898 100644 --- a/app/Controller/Component/ACLComponent.php +++ b/app/Controller/Component/ACLComponent.php @@ -15,13 +15,11 @@ class ACLComponent extends Component private $__aclList = array( '*' => array( 'blackhole' => array(), - 'checkAction' => array(), 'checkAuthUser' => array(), 'checkExternalAuthUser' => array(), 'cleanModelCaches' => array(), 'debugACL' => array(), 'generateCount' => array(), - 'getActions' => array(), 'pruneDuplicateUUIDs' => array(), 'queryACL' => array(), 'removeDuplicateEvents' => array(), From 49b85ed33cc5ebea3739d3ebf5686be4c514fd9b Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Fri, 13 Nov 2020 21:35:16 +0100 Subject: [PATCH 03/42] chg: [internal] Load just necessary info when loading homepage info --- app/Controller/AppController.php | 11 ++----- app/Controller/UsersController.php | 11 ++----- app/Model/UserSetting.php | 47 +++++++++++++++--------------- 3 files changed, 27 insertions(+), 42 deletions(-) diff --git a/app/Controller/AppController.php b/app/Controller/AppController.php index 76c321957..b2427daa8 100755 --- a/app/Controller/AppController.php +++ b/app/Controller/AppController.php @@ -477,16 +477,9 @@ class AppController extends Controller $this->set('notifications', $notifications); $this->loadModel('UserSetting'); - $homepage = $this->UserSetting->find('first', array( - 'recursive' => -1, - 'conditions' => array( - 'UserSetting.user_id' => $this->Auth->user('id'), - 'UserSetting.setting' => 'homepage' - ), - 'contain' => array('User.id', 'User.org_id') - )); + $homepage = $this->UserSetting->getValueForUser($this->Auth->user('id'), 'homepage'); if (!empty($homepage)) { - $this->set('homepage', $homepage['UserSetting']['value']); + $this->set('homepage', $homepage); } } } diff --git a/app/Controller/UsersController.php b/app/Controller/UsersController.php index 62bb96576..a558282bc 100644 --- a/app/Controller/UsersController.php +++ b/app/Controller/UsersController.php @@ -1253,16 +1253,9 @@ class UsersController extends AppController // Events list $url = $this->Session->consume('pre_login_requested_url'); if (empty($url)) { - $homepage = $this->User->UserSetting->find('first', array( - 'recursive' => -1, - 'conditions' => array( - 'UserSetting.user_id' => $this->Auth->user('id'), - 'UserSetting.setting' => 'homepage' - ), - 'contain' => array('User.id', 'User.org_id') - )); + $homepage = $this->User->UserSetting->getValueForUser($this->Auth->user('id'), 'homepage'); if (!empty($homepage)) { - $url = $homepage['UserSetting']['value']['path']; + $url = $homepage['path']; } else { $url = array('controller' => 'events', 'action' => 'index'); } diff --git a/app/Model/UserSetting.php b/app/Model/UserSetting.php index 8dc179d84..35592faef 100644 --- a/app/Model/UserSetting.php +++ b/app/Model/UserSetting.php @@ -201,34 +201,33 @@ class UserSetting extends AppModel public function getDefaulRestSearchParameters($user) { - $setting = $this->find('first', array( - 'recursive' => -1, - 'conditions' => array( - 'UserSetting.user_id' => $user['id'], - 'UserSetting.setting' => 'default_restsearch_parameters' - ) - )); - $parameters = array(); - if (!empty($setting)) { - $parameters = $setting['UserSetting']['value']; - } - return $parameters; + return $this->getValueForUser($user['id'], 'default_restsearch_parameters') ?: []; } public function getTagNumericalValueOverride($userId) { - $setting = $this->find('first', array( - 'recursive' => -1, - 'conditions' => array( - 'UserSetting.user_id' => $userId, - 'UserSetting.setting' => 'tag_numerical_value_override' - ) - )); - $parameters = array(); - if (!empty($setting)) { - $parameters = $setting['UserSetting']['value']; - } - return $parameters; + return $this->getValueForUser($userId, 'tag_numerical_value_override') ?: []; + } + + /** + * @param int $userId + * @param string $setting + * @return mixed|null + */ + public function getValueForUser($userId, $setting) + { + $output = $this->find('first', array( + 'recursive' => -1, + 'fields' => ['value'], + 'conditions' => array( + 'UserSetting.user_id' => $userId, + 'UserSetting.setting' => $setting, + ) + )); + if ($output) { + return $output['UserSetting']['value']; + } + return null; } /* From d0ec184796c1bdb8c3c08a4264ca05289920911b Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Sat, 14 Nov 2020 13:00:10 +0100 Subject: [PATCH 04/42] fix: [internal] Remove unused $user siteadmin variable --- app/Controller/AppController.php | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/app/Controller/AppController.php b/app/Controller/AppController.php index b2427daa8..74554f403 100755 --- a/app/Controller/AppController.php +++ b/app/Controller/AppController.php @@ -36,10 +36,10 @@ App::uses('RequestRearrangeTool', 'Tools'); * @package app.Controller * @link http://book.cakephp.org/2.0/en/controllers.html#the-app-controller * - * @throws ForbiddenException // TODO Exception * @property ACLComponent $ACL * @property RestResponseComponent $RestResponse * @property CRUDComponent $CRUD + * @property IndexFilterComponent $IndexFilter */ class AppController extends Controller { @@ -60,7 +60,6 @@ class AppController extends Controller public $baseurl = ''; public $sql_dump = false; - private $isRest = null; public $restResponsePayload = null; // Used for _isAutomation(), a check that returns true if the controller & action combo matches an action that is a non-xml and non-json automation method @@ -895,9 +894,6 @@ class AppController extends Controller if (!$user['Role']['perm_auth']) { return false; } - if ($user['Role']['perm_site_admin']) { - $user['siteadmin'] = true; - } return $user; } @@ -908,9 +904,6 @@ class AppController extends Controller if (empty($user)) { return false; } - if ($user['Role']['perm_site_admin']) { - $user['siteadmin'] = true; - } return $user; } From dbad8d545dcc5f7822d42a844f906d25f03f8d77 Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Mon, 16 Nov 2020 09:39:43 +0100 Subject: [PATCH 05/42] chg: [UI] Change description for user edit checkboxes --- app/View/Users/admin_add.ctp | 2 +- app/View/Users/admin_edit.ctp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/View/Users/admin_add.ctp b/app/View/Users/admin_add.ctp index 6aa435e81..33fc1a1e4 100644 --- a/app/View/Users/admin_add.ctp +++ b/app/View/Users/admin_add.ctp @@ -86,7 +86,7 @@ 'type' => 'checkbox', 'checked' => isset($this->request->data['User']['contactalert']) ? $this->request->data['User']['contactalert'] : true )); - echo $this->Form->input('disabled', array('type' => 'checkbox', 'label' => __('Disable this user account'))); + echo $this->Form->input('disabled', array('type' => 'checkbox', 'label' => __('Immediately disable this user account'))); echo $this->Form->input('notify', array( 'label' => __('Send credentials automatically'), 'type' => 'checkbox', diff --git a/app/View/Users/admin_edit.ctp b/app/View/Users/admin_edit.ctp index 36d984c13..f876163f4 100644 --- a/app/View/Users/admin_edit.ctp +++ b/app/View/Users/admin_edit.ctp @@ -81,13 +81,13 @@ echo $this->Form->input('termsaccepted', array('type' => 'checkbox', 'label' => __('Terms accepted'))); echo $this->Form->input('change_pw', [ 'type' => 'checkbox', - 'label' => __('User must change password after next login'), + 'label' => __('User must change password'), 'disabled' => !$canChangePassword, 'data-disabled-reason' => !$canChangePassword ? __('User password change is disabled on this instance') : '', ]); echo $this->Form->input('autoalert', array('label' => __('Receive email alerts when events are published'), 'type' => 'checkbox')); echo $this->Form->input('contactalert', array('label' => __('Receive email alerts from "Contact reporter" requests'), 'type' => 'checkbox')); - echo $this->Form->input('disabled', array('type' => 'checkbox', 'label' => __('Disable this user account'))); + echo $this->Form->input('disabled', array('type' => 'checkbox', 'label' => __('Immediately disable this user account'))); echo ''; ?> From b7eef315df57dff8de410a432e000088496e1dca Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Mon, 16 Nov 2020 09:46:26 +0100 Subject: [PATCH 06/42] chg: [internal] Do not fetch user settings for User::getAuthUser --- app/Model/User.php | 44 ++++++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/app/Model/User.php b/app/Model/User.php index 1959c714e..438e1fa85 100644 --- a/app/Model/User.php +++ b/app/Model/User.php @@ -641,39 +641,50 @@ class User extends AppModel // get the current user and rearrange it to be in the same format as in the auth component public function getAuthUser($id) { - $user = $this->getUserById($id); - if (empty($user)) { - return $user; + if (empty($id)) { + throw new InvalidArgumentException('Invalid user ID.'); } - return $this->rearrangeToAuthForm($user); + $conditions = ['User.id' => $id]; + return $this->getAuthUserByConditions($conditions); } // get the current user and rearrange it to be in the same format as in the auth component - public function getAuthUserByAuthkey($id) + public function getAuthUserByAuthkey($authkey) { - $conditions = array('User.authkey' => $id); - $user = $this->find('first', array('conditions' => $conditions, 'recursive' => -1,'contain' => array('Organisation', 'Role', 'Server'))); - if (empty($user)) { - return $user; + if (empty($authkey)) { + throw new InvalidArgumentException('Invalid user auth key.'); } - return $this->rearrangeToAuthForm($user); + $conditions = array('User.authkey' => $authkey); + return $this->getAuthUserByConditions($conditions); } public function getAuthUserByExternalAuth($auth_key) { + if (empty($auth_key)) { + throw new InvalidArgumentException('Invalid user external auth key.'); + } $conditions = array( 'User.external_auth_key' => $auth_key, 'User.external_auth_required' => true ); - $user = $this->find('first', array( + return $this->getAuthUserByConditions($conditions); + } + + /** + * @param array $conditions + * @return array|null + */ + private function getAuthUserByConditions(array $conditions) + { + $user = $this->find('first', [ 'conditions' => $conditions, 'recursive' => -1, - 'contain' => array( + 'contain' => [ 'Organisation', 'Role', - 'Server' - ) - )); + 'Server', + ], + ]); if (empty($user)) { return $user; } @@ -696,9 +707,6 @@ class User extends AppModel $user['User']['Role'] = $user['Role']; $user['User']['Organisation'] = $user['Organisation']; $user['User']['Server'] = $user['Server']; - if (isset($user['UserSetting'])) { - $user['User']['UserSetting'] = $user['UserSetting']; - } return $user['User']; } From cdf47d705e0960f0b08b33d023e8d98b34e24ce5 Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Mon, 16 Nov 2020 10:15:03 +0100 Subject: [PATCH 07/42] chg: [internal] Update role changes immediately --- app/Model/Role.php | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/app/Model/Role.php b/app/Model/Role.php index 6600f9a16..2fac591b6 100644 --- a/app/Model/Role.php +++ b/app/Model/Role.php @@ -1,6 +1,9 @@ data)) { + $roleId = $this->data['Role']['id']; + $this->User->updateAll(['date_modified' => time()], ['role_id' => $roleId]); + } + + parent::afterSave($created, $options); + } + public function afterFind($results, $primary = false) { foreach ($results as $key => $val) { From a0fb186a3c84f44a2c98081e0b2dcc85a9f20872 Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Mon, 16 Nov 2020 20:28:52 +0100 Subject: [PATCH 08/42] chg: [internal] Simplify User::describeAuthFields --- app/Controller/AppController.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/Controller/AppController.php b/app/Controller/AppController.php index 74554f403..afdbb92cf 100755 --- a/app/Controller/AppController.php +++ b/app/Controller/AppController.php @@ -165,11 +165,12 @@ class AppController extends Controller Configure::write('Config.language', 'eng'); } - //if fresh installation (salt empty) generate a new salt + // For fresh installation (salt empty) generate a new salt if (!Configure::read('Security.salt')) { $this->loadModel('Server'); $this->Server->serverSettingsSaveValue('Security.salt', $this->User->generateRandomPassword(32)); } + // Check if the instance has a UUID, if not assign one. if (!Configure::read('MISP.uuid')) { $this->loadModel('Server'); @@ -884,7 +885,6 @@ class AppController extends Controller $this->loadModel('AuthKey'); $user = $this->AuthKey->getAuthUserByAuthKey($authkey); } else { - $this->loadModel('User'); $user = $this->User->getAuthUserByAuthKey($authkey); } @@ -899,7 +899,6 @@ class AppController extends Controller public function checkExternalAuthUser($authkey) { - $this->loadModel('User'); $user = $this->User->getAuthUserByExternalAuth($authkey); if (empty($user)) { return false; From 6ce13b8168e3b3edf145afb664c425657aa12b11 Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Tue, 17 Nov 2020 17:33:34 +0100 Subject: [PATCH 09/42] chg: [internal] Do not log full authkeys --- app/Controller/AppController.php | 61 +++++++++++++++++--------------- 1 file changed, 33 insertions(+), 28 deletions(-) diff --git a/app/Controller/AppController.php b/app/Controller/AppController.php index afdbb92cf..b472d3d8b 100755 --- a/app/Controller/AppController.php +++ b/app/Controller/AppController.php @@ -237,64 +237,69 @@ class AppController extends Controller } // Authenticate user with authkey in Authorization HTTP header if (!empty($_SERVER['HTTP_AUTHORIZATION']) || !empty($namedParamAuthkey)) { - $found_misp_auth_key = false; + $foundMispAuthKey = false; $authentication = explode(',', $_SERVER['HTTP_AUTHORIZATION']); if (!empty($namedParamAuthkey)) { $authentication[] = $namedParamAuthkey; } $user = false; - foreach ($authentication as $auth_key) { - if (preg_match('/^[a-zA-Z0-9]{40}$/', trim($auth_key))) { - $found_misp_auth_key = true; - $temp = $this->checkAuthUser(trim($auth_key)); + foreach ($authentication as $authKey) { + $authKey = trim($authKey); + if (preg_match('/^[a-zA-Z0-9]{40}$/', $authKey)) { + $foundMispAuthKey = true; + $temp = $this->checkAuthUser($authKey); if ($temp) { - $user['User'] = $temp; + $user = $temp; + break; } } } - if ($found_misp_auth_key) { + if ($foundMispAuthKey) { + $authKeyToStore = substr($authKey, 0, 4) + . str_repeat('*', 32) + . substr($authKey, -4); if ($user) { - unset($user['User']['gpgkey']); - unset($user['User']['certif_public']); + unset($user['gpgkey']); + unset($user['certif_public']); // User found in the db, add the user info to the session if (Configure::read('MISP.log_auth')) { $this->Log = ClassRegistry::init('Log'); $this->Log->create(); $log = array( - 'org' => $user['User']['Organisation']['name'], - 'model' => 'User', - 'model_id' => $user['User']['id'], - 'email' => $user['User']['email'], - 'action' => 'auth', - 'title' => 'Successful authentication using API key', - 'change' => 'HTTP method: ' . $_SERVER['REQUEST_METHOD'] . PHP_EOL . 'Target: ' . $this->here, + 'org' => $user['Organisation']['name'], + 'model' => 'User', + 'model_id' => $user['id'], + 'email' => $user['email'], + 'action' => 'auth', + 'title' => "Successful authentication using API key ($authKeyToStore)", + 'change' => 'HTTP method: ' . $_SERVER['REQUEST_METHOD'] . PHP_EOL . 'Target: ' . $this->here, ); $this->Log->save($log); } $this->Session->renew(); - $this->Session->write(AuthComponent::$sessionKey, $user['User']); + $this->Session->write(AuthComponent::$sessionKey, $user); $this->isApiAuthed = true; } else { // User not authenticated correctly // reset the session information $redis = $this->{$this->modelClass}->setupRedis(); - if ($redis && !$redis->exists('misp:auth_fail_throttling:' . trim($auth_key))) { - $redis->set('misp:auth_fail_throttling:' . trim($auth_key), 1); - $redis->expire('misp:auth_fail_throttling:' . trim($auth_key), 3600); - $this->Session->destroy(); + // Do not log every fail, but just once per hour + if ($redis && !$redis->exists('misp:auth_fail_throttling:' . $authKeyToStore)) { + $redis->setex('misp:auth_fail_throttling:' . $authKeyToStore, 3600, 1); $this->Log = ClassRegistry::init('Log'); $this->Log->create(); $log = array( - 'org' => 'SYSTEM', - 'model' => 'User', - 'model_id' => 0, - 'email' => 'SYSTEM', - 'action' => 'auth_fail', - 'title' => 'Failed authentication using API key (' . trim($auth_key) . ')', - 'change' => null, + 'org' => 'SYSTEM', + 'model' => 'User', + 'model_id' => 0, + 'email' => 'SYSTEM', + 'action' => 'auth_fail', + 'title' => "Failed authentication using API key ($authKeyToStore)", + 'change' => null, ); $this->Log->save($log); } + $this->Session->destroy(); throw new ForbiddenException('Authentication failed. Please make sure you pass the API key of an API enabled user along in the Authorization header.'); } unset($user); From e5e855b3c290af23eeba12bf7fb3b5cd57b1183c Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Tue, 17 Nov 2020 20:09:49 +0100 Subject: [PATCH 10/42] new: [internal] Allow to log authkey usage in Redis --- app/Controller/AppController.php | 171 +++++++++++++++++-------------- app/Model/AuthKey.php | 8 +- 2 files changed, 101 insertions(+), 78 deletions(-) diff --git a/app/Controller/AppController.php b/app/Controller/AppController.php index b472d3d8b..55a3acead 100755 --- a/app/Controller/AppController.php +++ b/app/Controller/AppController.php @@ -230,82 +230,7 @@ class AppController extends Controller if (array_key_exists('Security', $this->components)) { $this->Security->csrfCheck = false; } - // If enabled, allow passing the API key via a named parameter (for crappy legacy systems only) - $namedParamAuthkey = false; - if (Configure::read('Security.allow_unsafe_apikey_named_param') && !empty($this->params['named']['apikey'])) { - $namedParamAuthkey = $this->params['named']['apikey']; - } - // Authenticate user with authkey in Authorization HTTP header - if (!empty($_SERVER['HTTP_AUTHORIZATION']) || !empty($namedParamAuthkey)) { - $foundMispAuthKey = false; - $authentication = explode(',', $_SERVER['HTTP_AUTHORIZATION']); - if (!empty($namedParamAuthkey)) { - $authentication[] = $namedParamAuthkey; - } - $user = false; - foreach ($authentication as $authKey) { - $authKey = trim($authKey); - if (preg_match('/^[a-zA-Z0-9]{40}$/', $authKey)) { - $foundMispAuthKey = true; - $temp = $this->checkAuthUser($authKey); - if ($temp) { - $user = $temp; - break; - } - } - } - if ($foundMispAuthKey) { - $authKeyToStore = substr($authKey, 0, 4) - . str_repeat('*', 32) - . substr($authKey, -4); - if ($user) { - unset($user['gpgkey']); - unset($user['certif_public']); - // User found in the db, add the user info to the session - if (Configure::read('MISP.log_auth')) { - $this->Log = ClassRegistry::init('Log'); - $this->Log->create(); - $log = array( - 'org' => $user['Organisation']['name'], - 'model' => 'User', - 'model_id' => $user['id'], - 'email' => $user['email'], - 'action' => 'auth', - 'title' => "Successful authentication using API key ($authKeyToStore)", - 'change' => 'HTTP method: ' . $_SERVER['REQUEST_METHOD'] . PHP_EOL . 'Target: ' . $this->here, - ); - $this->Log->save($log); - } - $this->Session->renew(); - $this->Session->write(AuthComponent::$sessionKey, $user); - $this->isApiAuthed = true; - } else { - // User not authenticated correctly - // reset the session information - $redis = $this->{$this->modelClass}->setupRedis(); - // Do not log every fail, but just once per hour - if ($redis && !$redis->exists('misp:auth_fail_throttling:' . $authKeyToStore)) { - $redis->setex('misp:auth_fail_throttling:' . $authKeyToStore, 3600, 1); - $this->Log = ClassRegistry::init('Log'); - $this->Log->create(); - $log = array( - 'org' => 'SYSTEM', - 'model' => 'User', - 'model_id' => 0, - 'email' => 'SYSTEM', - 'action' => 'auth_fail', - 'title' => "Failed authentication using API key ($authKeyToStore)", - 'change' => null, - ); - $this->Log->save($log); - } - $this->Session->destroy(); - throw new ForbiddenException('Authentication failed. Please make sure you pass the API key of an API enabled user along in the Authorization header.'); - } - unset($user); - } - } - if ($this->Auth->user() == null) { + if ($this->__logByAuthKey() === false ||$this->Auth->user() === null) { throw new ForbiddenException('Authentication failed. Please make sure you pass the API key of an API enabled user along in the Authorization header.'); } } elseif (!$this->Session->read(AuthComponent::$sessionKey)) { @@ -489,6 +414,100 @@ class AppController extends Controller } } + /** + * @return null|bool True if authkey was correct, False if incorrect and Null if not provided + * @throws Exception + */ + private function __logByAuthKey() + { + // If enabled, allow passing the API key via a named parameter (for crappy legacy systems only) + $namedParamAuthkey = false; + if (Configure::read('Security.allow_unsafe_apikey_named_param') && !empty($this->params['named']['apikey'])) { + $namedParamAuthkey = $this->params['named']['apikey']; + } + // Authenticate user with authkey in Authorization HTTP header + if (!empty($_SERVER['HTTP_AUTHORIZATION']) || !empty($namedParamAuthkey)) { + $foundMispAuthKey = false; + $authentication = explode(',', $_SERVER['HTTP_AUTHORIZATION']); + if (!empty($namedParamAuthkey)) { + $authentication[] = $namedParamAuthkey; + } + $user = false; + foreach ($authentication as $authKey) { + $authKey = trim($authKey); + if (preg_match('/^[a-zA-Z0-9]{40}$/', $authKey)) { + $foundMispAuthKey = true; + $temp = $this->checkAuthUser($authKey); + if ($temp) { + $user = $temp; + break; + } + } + } + if ($foundMispAuthKey) { + $authKeyToStore = substr($authKey, 0, 4) + . str_repeat('*', 32) + . substr($authKey, -4); + if ($user) { + unset($user['gpgkey']); + unset($user['certif_public']); + // User found in the db, add the user info to the session + if (Configure::read('MISP.log_auth')) { + $this->loadModel('Log'); + $this->Log->create(); + $log = array( + 'org' => $user['Organisation']['name'], + 'model' => 'User', + 'model_id' => $user['id'], + 'email' => $user['email'], + 'action' => 'auth', + 'title' => "Successful authentication using API key ($authKeyToStore)", + 'change' => 'HTTP method: ' . $_SERVER['REQUEST_METHOD'] . PHP_EOL . 'Target: ' . $this->here, + ); + $this->Log->save($log); + } + // Allow to log key usage + if (isset($user['authkey_id']) && Configure::read('MISP.log_auth_redis')) { + /** @var Redis $redis */ + $redis = $this->{$this->modelClass}->setupRedis(); + if ($redis) { + $key = "misp:authkey:{$user['authkey_id']}"; + $hashKey = date("Y-m-d") . ":{$_SERVER['REMOTE_ADDR']}"; + $redis->hIncrBy($key, $hashKey, 1); + } + } + $this->Session->renew(); + $this->Session->write(AuthComponent::$sessionKey, $user); + $this->isApiAuthed = true; + return true; + } else { + // User not authenticated correctly + // reset the session information + $redis = $this->{$this->modelClass}->setupRedis(); + // Do not log every fail, but just once per hour + if ($redis && !$redis->exists('misp:auth_fail_throttling:' . $authKeyToStore)) { + $redis->setex('misp:auth_fail_throttling:' . $authKeyToStore, 3600, 1); + $this->loadModel('Log'); + $this->Log->create(); + $log = array( + 'org' => 'SYSTEM', + 'model' => 'User', + 'model_id' => 0, + 'email' => 'SYSTEM', + 'action' => 'auth_fail', + 'title' => "Failed authentication using API key ($authKeyToStore)", + 'change' => null, + ); + $this->Log->save($log); + } + $this->Session->destroy(); + } + } + return false; + } + return null; + } + /** * Check if: * - user exists in database diff --git a/app/Model/AuthKey.php b/app/Model/AuthKey.php index 68bc698f7..9bbbe0e21 100644 --- a/app/Model/AuthKey.php +++ b/app/Model/AuthKey.php @@ -57,7 +57,7 @@ class AuthKey extends AppModel $end = substr($authkey, -4); $existing_authkeys = $this->find('all', [ 'recursive' => -1, - 'fields' => ['authkey', 'user_id'], + 'fields' => ['id', 'authkey', 'user_id'], 'conditions' => [ 'OR' => [ 'expiration >' => time(), @@ -70,7 +70,11 @@ class AuthKey extends AppModel $passwordHasher = $this->getHasher(); foreach ($existing_authkeys as $existing_authkey) { if ($passwordHasher->check($authkey, $existing_authkey['AuthKey']['authkey'])) { - return $this->User->getAuthUser($existing_authkey['AuthKey']['user_id']); + $user = $this->User->getAuthUser($existing_authkey['AuthKey']['user_id']); + if ($user) { + $user['authkey_id'] = $existing_authkey['AuthKey']['id']; + } + return $user; } } return false; From 68215560006f9971d16b8e9bec4bfcfb4a569512 Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Sat, 21 Nov 2020 15:42:05 +0100 Subject: [PATCH 11/42] chg: [internal] Allow to reuse session for API requests --- app/Controller/AppController.php | 58 +++++++++++++++++--------------- 1 file changed, 31 insertions(+), 27 deletions(-) diff --git a/app/Controller/AppController.php b/app/Controller/AppController.php index 55a3acead..0f84de8cf 100755 --- a/app/Controller/AppController.php +++ b/app/Controller/AppController.php @@ -55,7 +55,7 @@ class AppController extends Controller public $phprec = '7.4'; public $pythonmin = '3.6'; public $pythonrec = '3.7'; - public $isApiAuthed = false; + private $isApiAuthed = false; public $baseurl = ''; public $sql_dump = false; @@ -230,7 +230,7 @@ class AppController extends Controller if (array_key_exists('Security', $this->components)) { $this->Security->csrfCheck = false; } - if ($this->__logByAuthKey() === false ||$this->Auth->user() === null) { + if ($this->__loginByAuthKey() === false || $this->Auth->user() === null) { throw new ForbiddenException('Authentication failed. Please make sure you pass the API key of an API enabled user along in the Authorization header.'); } } elseif (!$this->Session->read(AuthComponent::$sessionKey)) { @@ -249,35 +249,44 @@ class AppController extends Controller } if ($this->Auth->user()) { - Configure::write('CurrentUserId', $this->Auth->user('id')); - $this->User->setMonitoring($this->Auth->user()); + $user = $this->Auth->user(); + Configure::write('CurrentUserId', $user['id']); + $this->User->setMonitoring($user); if (Configure::read('MISP.log_user_ips')) { $redis = $this->{$this->modelClass}->setupRedis(); if ($redis) { - $redis->set('misp:ip_user:' . trim($_SERVER['REMOTE_ADDR']), $this->Auth->user('id')); - $redis->expire('misp:ip_user:' . trim($_SERVER['REMOTE_ADDR']), 60*60*24*30); - $redis->sadd('misp:user_ip:' . $this->Auth->user('id'), trim($_SERVER['REMOTE_ADDR'])); + $remoteAddress = trim($_SERVER['REMOTE_ADDR']); + $redis->set('misp:ip_user:' . $remoteAddress, $user['id']); + $redis->expire('misp:ip_user:' . $remoteAddress, 60*60*24*30); + $redis->sadd('misp:user_ip:' . $user['id'], $remoteAddress); + + // Allow to log key usage + if (isset($user['authkey_id']) && Configure::read('MISP.log_user_ips_auth')) { + $key = "misp:authkey:{$user['authkey_id']}"; + $hashKey = date("Y-m-d") . ":$remoteAddress"; + $redis->hIncrBy($key, $hashKey, 1); + } } } // update script - if ($this->Auth->user('Role')['perm_site_admin'] || (Configure::read('MISP.live') && !$this->_isRest())) { + if ($user['Role']['perm_site_admin'] || (Configure::read('MISP.live') && !$this->_isRest())) { $this->{$this->modelClass}->runUpdates(); } - if (!$this->__verifyUser($this->User, $this->Auth->user())) { + if (!$this->__verifyUser($this->User, $user)) { $this->_stop(); // just for sure } $this->set('default_memory_limit', ini_get('memory_limit')); - if (isset($this->Auth->user('Role')['memory_limit'])) { - if ($this->Auth->user('Role')['memory_limit'] !== '') { - ini_set('memory_limit', $this->Auth->user('Role')['memory_limit']); + if (isset($user['Role']['memory_limit'])) { + if ($user['Role']['memory_limit'] !== '') { + ini_set('memory_limit', $user['Role']['memory_limit']); } } $this->set('default_max_execution_time', ini_get('max_execution_time')); - if (isset($this->Auth->user('Role')['max_execution_time'])) { - if ($this->Auth->user('Role')['max_execution_time'] !== '') { - ini_set('max_execution_time', $this->Auth->user('Role')['max_execution_time']); + if (isset($user['Role']['max_execution_time'])) { + if ($user['Role']['max_execution_time'] !== '') { + ini_set('max_execution_time', $user['Role']['max_execution_time']); } } } else { @@ -418,8 +427,13 @@ class AppController extends Controller * @return null|bool True if authkey was correct, False if incorrect and Null if not provided * @throws Exception */ - private function __logByAuthKey() + private function __loginByAuthKey() { + if (Configure::read('Security.authkey_keep_session') && $this->Auth->user()) { + // Do not check authkey if session is establish and correct + return true; + } + // If enabled, allow passing the API key via a named parameter (for crappy legacy systems only) $namedParamAuthkey = false; if (Configure::read('Security.allow_unsafe_apikey_named_param') && !empty($this->params['named']['apikey'])) { @@ -466,16 +480,6 @@ class AppController extends Controller ); $this->Log->save($log); } - // Allow to log key usage - if (isset($user['authkey_id']) && Configure::read('MISP.log_auth_redis')) { - /** @var Redis $redis */ - $redis = $this->{$this->modelClass}->setupRedis(); - if ($redis) { - $key = "misp:authkey:{$user['authkey_id']}"; - $hashKey = date("Y-m-d") . ":{$_SERVER['REMOTE_ADDR']}"; - $redis->hIncrBy($key, $hashKey, 1); - } - } $this->Session->renew(); $this->Session->write(AuthComponent::$sessionKey, $user); $this->isApiAuthed = true; @@ -675,7 +679,7 @@ class AppController extends Controller public function afterFilter() { - if ($this->isApiAuthed && $this->_isRest() && $this->Session->started()) { + if ($this->isApiAuthed && $this->_isRest() &&!Configure::read('Security.authkey_keep_session')) { $this->Session->destroy(); } } From c6bf9de3cadaeec2f8ac6ac12706ea93f3c4c432 Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Sat, 21 Nov 2020 19:27:41 +0100 Subject: [PATCH 12/42] fix: [internal] Remove unused variables --- app/Controller/AppController.php | 43 +++++++++++-------------------- app/View/Helper/UtilityHelper.php | 12 --------- 2 files changed, 15 insertions(+), 40 deletions(-) delete mode 100644 app/View/Helper/UtilityHelper.php diff --git a/app/Controller/AppController.php b/app/Controller/AppController.php index 0f84de8cf..867afbd63 100755 --- a/app/Controller/AppController.php +++ b/app/Controller/AppController.php @@ -40,6 +40,7 @@ App::uses('RequestRearrangeTool', 'Tools'); * @property RestResponseComponent $RestResponse * @property CRUDComponent $CRUD * @property IndexFilterComponent $IndexFilter + * @property User $User */ class AppController extends Controller { @@ -47,7 +48,7 @@ class AppController extends Controller public $debugMode = false; - public $helpers = array('Utility', 'OrgImg', 'FontAwesome', 'UserName', 'DataPathCollector'); + public $helpers = array('OrgImg', 'FontAwesome', 'UserName', 'DataPathCollector'); private $__queryVersion = '119'; public $pyMispVersion = '2.4.135'; @@ -113,14 +114,12 @@ class AppController extends Controller public function beforeFilter() { - $this->Auth->loginRedirect = Configure::read('MISP.baseurl') . '/users/routeafterlogin'; + $this->_setupBaseurl(); + $this->Auth->loginRedirect = $this->baseurl. '/users/routeafterlogin'; $customLogout = Configure::read('Plugin.CustomAuth_custom_logout'); - if ($customLogout) { - $this->Auth->logoutRedirect = $customLogout; - } else { - $this->Auth->logoutRedirect = Configure::read('MISP.baseurl') . '/users/login'; - } + $this->Auth->logoutRedirect = $customLogout ?: ($this->baseurl . '/users/login'); + $this->__sessionMassage(); if (Configure::read('Security.allow_cors')) { // Add CORS headers @@ -199,7 +198,6 @@ class AppController extends Controller $versionArray = $this->{$this->modelClass}->checkMISPVersion(); $this->mispVersion = implode('.', array_values($versionArray)); $this->Security->blackHoleCallback = 'blackHole'; - $this->_setupBaseurl(); // send users away that are using ancient versions of IE // Make sure to update this if IE 20 comes out :) @@ -237,16 +235,6 @@ class AppController extends Controller $this->_loadAuthenticationPlugins(); } } - $this->set('externalAuthUser', $userLoggedIn); - // user must accept terms - // - // grab the base path from our base url for use in the following checks - $base_dir = parse_url($this->baseurl, PHP_URL_PATH); - - // if MISP is running out of the web root already, just set this variable to blank so we don't wind up with '//' in the following if statements - if ($base_dir == '/') { - $base_dir = ''; - } if ($this->Auth->user()) { $user = $this->Auth->user(); @@ -257,7 +245,7 @@ class AppController extends Controller if ($redis) { $remoteAddress = trim($_SERVER['REMOTE_ADDR']); $redis->set('misp:ip_user:' . $remoteAddress, $user['id']); - $redis->expire('misp:ip_user:' . $remoteAddress, 60*60*24*30); + $redis->expire('misp:ip_user:' . $remoteAddress, 60 * 60 * 24 * 30); // 30 days $redis->sadd('misp:user_ip:' . $user['id'], $remoteAddress); // Allow to log key usage @@ -294,7 +282,7 @@ class AppController extends Controller if (!empty(Configure::read('Security.email_otp_enabled'))) { $pre_auth_actions[] = 'email_otp'; } - if ($this->params['controller'] !== 'users' || !in_array($this->params['action'], $pre_auth_actions)) { + if (!$this->_isControllerAction(['users' => $pre_auth_actions])) { if (!$this->request->is('ajax')) { $this->Session->write('pre_login_requested_url', $this->here); } @@ -302,7 +290,6 @@ class AppController extends Controller } } - unset($base_dir); // We don't want to run these role checks before the user is logged in, but we want them available for every view once the user is logged on if ($this->Auth->user()) { $this->set('mispVersion', implode('.', array($versionArray['major'], $versionArray['minor'], 0))); @@ -375,7 +362,7 @@ class AppController extends Controller } if ($this->Auth->user() && $this->_isSiteAdmin()) { - if (Configure::read('Session.defaults') == 'database') { + if (Configure::read('Session.defaults') === 'database') { $db = ConnectionManager::getDataSource('default'); $sqlResult = $db->query('SELECT COUNT(id) AS session_count FROM cake_sessions WHERE expires < ' . time() . ';'); if (isset($sqlResult[0][0]['session_count']) && $sqlResult[0][0]['session_count'] > 1000) { @@ -415,8 +402,7 @@ class AppController extends Controller } $this->set('notifications', $notifications); - $this->loadModel('UserSetting'); - $homepage = $this->UserSetting->getValueForUser($this->Auth->user('id'), 'homepage'); + $homepage = $this->User->UserSetting->getValueForUser($this->Auth->user('id'), 'homepage'); if (!empty($homepage)) { $this->set('homepage', $homepage); } @@ -646,7 +632,7 @@ class AppController extends Controller if (!isset($actionsToCheck[$controller])) { return false; } - return in_array($this->action, $actionsToCheck[$controller]); + return in_array($this->action, $actionsToCheck[$controller], true); } private function __rateLimitCheck() @@ -726,16 +712,17 @@ class AppController extends Controller /* * Sanitize the configured `MISP.baseurl` and expose it to the view as `baseurl`. */ - protected function _setupBaseurl() { + protected function _setupBaseurl() + { // Let us access $baseurl from all views $baseurl = Configure::read('MISP.baseurl'); - if (substr($baseurl, -1) == '/') { + if (substr($baseurl, -1) === '/') { // if the baseurl has a trailing slash, remove it. It can lead to issues with the CSRF protection $baseurl = rtrim($baseurl, '/'); $this->loadModel('Server'); $this->Server->serverSettingsSaveValue('MISP.baseurl', $baseurl); } - if (trim($baseurl) == 'http://') { + if (trim($baseurl) === 'http://') { $this->Server->serverSettingsSaveValue('MISP.baseurl', ''); } $this->baseurl = $baseurl; diff --git a/app/View/Helper/UtilityHelper.php b/app/View/Helper/UtilityHelper.php deleted file mode 100644 index 7ddd7490e..000000000 --- a/app/View/Helper/UtilityHelper.php +++ /dev/null @@ -1,12 +0,0 @@ - Date: Sat, 28 Nov 2020 15:14:31 +0100 Subject: [PATCH 13/42] chg: [internal] Force to update session data after database update --- app/Model/AppModel.php | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/app/Model/AppModel.php b/app/Model/AppModel.php index 509cceead..da3da6841 100644 --- a/app/Model/AppModel.php +++ b/app/Model/AppModel.php @@ -1627,10 +1627,8 @@ class AppModel extends Model break; default: return false; - break; } - $now = new DateTime(); // switch MISP instance live to false if ($liveOff) { $this->Server = Classregistry::init('Server'); @@ -1643,7 +1641,7 @@ class AppModel extends Model $this->__setUpdateProgress(0, $total_update_count, $command); $str_index_array = array(); foreach($indexArray as $toIndex) { - $str_index_array[] = __('Indexing ') . sprintf('%s -> %s', $toIndex[0], $toIndex[1]); + $str_index_array[] = __('Indexing %s -> %s', $toIndex[0], $toIndex[1]); } $this->__setUpdateCmdMessages(array_merge($sqlArray, $str_index_array)); $flagStop = false; @@ -1679,10 +1677,10 @@ class AppModel extends Model 'email' => 'SYSTEM', 'action' => 'update_database', 'user_id' => 0, - 'title' => __('Successfuly executed the SQL query for ') . $command, + 'title' => __('Successfully executed the SQL query for ') . $command, 'change' => sprintf(__('The executed SQL query was: %s'), $sql) )); - $this->__setUpdateResMessages($i, sprintf(__('Successfuly executed the SQL query for %s'), $command)); + $this->__setUpdateResMessages($i, sprintf(__('Successfully executed the SQL query for %s'), $command)); } catch (Exception $e) { $errorMessage = $e->getMessage(); $this->Log->create(); @@ -1736,14 +1734,13 @@ class AppModel extends Model } } } - $this->__setUpdateProgress(count($sqlArray)+count($indexArray), false); + $this->__setUpdateProgress(count($sqlArray) + count($indexArray), false); } if ($clean) { $this->cleanCacheFiles(); } if ($liveOff) { - $liveSetting = 'MISP.live'; - $this->Server->serverSettingsSaveValue($liveSetting, true); + $this->Server->serverSettingsSaveValue('MISP.live', true); } if (!$flagStop && $errorCount == 0) { $this->__postUpdate($command); @@ -2132,11 +2129,20 @@ class AppModel extends Model } } if ($requiresLogout) { - $this->updateDatabase('destroyAllSessions'); + $this->refreshSessions(); } return true; } + /** + * Update date_modified for all users, this will ensure that all users will refresh their session data. + */ + private function refreshSessions() + { + $this->User = ClassRegistry::init('User'); + $this->User->updateAll(['date_modified' => time()]); + } + private function __setUpdateProgress($current, $total=false, $toward_db_version=false) { $updateProgress = $this->getUpdateProgress(); From ee8a495d89ffb7cacff3898dc4210bcc9233f520 Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Sat, 28 Nov 2020 16:46:00 +0100 Subject: [PATCH 14/42] new: [internal] Show auth key usage in key view page --- app/Controller/AppController.php | 70 ++++++++++++++----- app/Controller/AuthKeysController.php | 17 ++++- app/Model/AuthKey.php | 28 ++++++++ app/Model/User.php | 18 ----- app/View/AuthKeys/index.ctp | 8 +++ app/View/AuthKeys/view.ctp | 29 +++++++- .../SingleViews/Fields/sparklineField.ctp | 14 ++++ .../SingleViews/single_view.ctp | 4 ++ 8 files changed, 148 insertions(+), 40 deletions(-) create mode 100644 app/View/Elements/genericElements/SingleViews/Fields/sparklineField.ctp diff --git a/app/Controller/AppController.php b/app/Controller/AppController.php index 867afbd63..5a63b1fe1 100755 --- a/app/Controller/AppController.php +++ b/app/Controller/AppController.php @@ -236,26 +236,11 @@ class AppController extends Controller } } + $userMonitoringEnabled = false; if ($this->Auth->user()) { $user = $this->Auth->user(); Configure::write('CurrentUserId', $user['id']); - $this->User->setMonitoring($user); - if (Configure::read('MISP.log_user_ips')) { - $redis = $this->{$this->modelClass}->setupRedis(); - if ($redis) { - $remoteAddress = trim($_SERVER['REMOTE_ADDR']); - $redis->set('misp:ip_user:' . $remoteAddress, $user['id']); - $redis->expire('misp:ip_user:' . $remoteAddress, 60 * 60 * 24 * 30); // 30 days - $redis->sadd('misp:user_ip:' . $user['id'], $remoteAddress); - - // Allow to log key usage - if (isset($user['authkey_id']) && Configure::read('MISP.log_user_ips_auth')) { - $key = "misp:authkey:{$user['authkey_id']}"; - $hashKey = date("Y-m-d") . ":$remoteAddress"; - $redis->hIncrBy($key, $hashKey, 1); - } - } - } + $userMonitoringEnabled = $this->__accessMonitor($user); // update script if ($user['Role']['perm_site_admin'] || (Configure::read('MISP.live') && !$this->_isRest())) { $this->{$this->modelClass}->runUpdates(); @@ -325,7 +310,7 @@ class AppController extends Controller if ( Configure::read('MISP.log_paranoid') || - !empty(Configure::read('Security.monitored')) + $userMonitoringEnabled ) { $this->Log = ClassRegistry::init('Log'); $this->Log->create(); @@ -337,7 +322,7 @@ class AppController extends Controller ) && ( !empty(Configure::read('MISP.log_paranoid_include_post_body')) || - !empty(Configure::read('Security.monitored')) + $userMonitoringEnabled ) ) { $payload = $this->request->input(); @@ -635,6 +620,53 @@ class AppController extends Controller return in_array($this->action, $actionsToCheck[$controller], true); } + /** + * User access monitoring + * @param array $user + * @return bool True is user monitoring is enabled + */ + private function __accessMonitor(array $user) + { + $userMonitoringEnabled = Configure::read('Security.user_monitoring_enabled'); + $logUserIps = Configure::read('MISP.log_user_ips'); + + if (!$userMonitoringEnabled && !$logUserIps) { + return false; + } + + $redis = $this->User->setupRedis(); + if (!$redis) { + return false; + } + + if ($logUserIps) { + $remoteAddress = trim($_SERVER['REMOTE_ADDR']); + + $pipe = $redis->multi(Redis::PIPELINE); + // keep for 30 days + $pipe->setex('misp:ip_user:' . $remoteAddress, 60 * 60 * 24 * 30, $user['id']); + $pipe->sadd('misp:user_ip:' . $user['id'], $remoteAddress); + + // Log key usage if enabled + if (isset($user['authkey_id']) && Configure::read('MISP.log_user_ips_auth')) { + // Use request time if defined + $time = isset($_SERVER['REQUEST_TIME']) ? $_SERVER['REQUEST_TIME'] : time(); + $hashKey = date("Y-m-d", $time) . ":$remoteAddress"; + $pipe->hIncrBy("misp:authkey_usage:{$user['authkey_id']}", $hashKey, 1); + // delete after one year of inactivity + $pipe->expire("misp:authkey_usage:{$user['authkey_id']}", 3600 * 24 * 365); + $pipe->set("misp:authkey_last_usage:{$user['authkey_id']}", $time); + } + $pipe->exec(); + } + + // Return true for the current user if monitoring of this user is enabled in Redis. + if ($userMonitoringEnabled && $redis->sismember('misp:monitored_users', $user['id'])) { + return true; + } + return false; + } + private function __rateLimitCheck() { $info = array(); diff --git a/app/Controller/AuthKeysController.php b/app/Controller/AuthKeysController.php index a721a5b3d..da2499dda 100644 --- a/app/Controller/AuthKeysController.php +++ b/app/Controller/AuthKeysController.php @@ -1,6 +1,9 @@ CRUD->index([ 'filters' => ['User.username', 'authkey', 'comment', 'User.id'], 'quickFilters' => ['authkey', 'comment'], - 'contain' => ['User'], + 'contain' => ['User.id', 'User.email'], 'conditions' => $conditions, 'afterFind' => function (array $authKeys) { + $keyUsageEnabled = Configure::read('MISP.log_user_ips') && Configure::read('MISP.log_user_ips_auth'); foreach ($authKeys as &$authKey) { + if ($keyUsageEnabled) { + $lastUsed = $this->AuthKey->getKeyUsage($authKey['AuthKey']['id'])[1]; + $key['AuthKey']['last_used'] = $lastUsed ? $lastUsed->format('c') : null; + } unset($authKey['AuthKey']['authkey']); } return $authKeys; @@ -101,6 +109,13 @@ class AuthKeysController extends AppController if ($this->IndexFilter->isRest()) { return $this->restResponsePayload; } + + if (Configure::read('MISP.log_user_ips') && Configure::read('MISP.log_user_ips_auth')) { + list($keyUsage, $lastUsed) = $this->AuthKey->getKeyUsage($id); + $this->set('keyUsage', $keyUsage); + $this->set('lastUsed', $lastUsed); + } + $this->set('menuData', [ 'menuList' => $this->_isSiteAdmin() ? 'admin' : 'globalActions', 'menuItem' => 'authKeyView', diff --git a/app/Model/AuthKey.php b/app/Model/AuthKey.php index 9bbbe0e21..9ab1e40f9 100644 --- a/app/Model/AuthKey.php +++ b/app/Model/AuthKey.php @@ -109,6 +109,34 @@ class AuthKey extends AppModel } } + /** + * @param int $id + * @return array + * @throws Exception + */ + public function getKeyUsage($id) + { + $redis = $this->setupRedisWithException(); + $data = $redis->hGetAll("misp:authkey_usage:$id"); + + $output = []; + foreach ($data as $key => $count) { + list($date, $ip) = explode(':', $key); + if (isset($output[$date])) { + $output[$date] += $count; + } else { + $output[$date] = $count; + } + } + // Data from redis are not sorted + ksort($output); + + $lastUsage = $redis->get("misp:authkey_last_usage:$id"); + $lastUsage = $lastUsage === false ? null : new DateTime("@$lastUsage"); + + return [$output, $lastUsage]; + } + /** * @return AbstractPasswordHasher */ diff --git a/app/Model/User.php b/app/Model/User.php index 438e1fa85..944584b9f 100644 --- a/app/Model/User.php +++ b/app/Model/User.php @@ -1294,24 +1294,6 @@ class User extends AppModel return $data; } - /* - * Set the monitoring flag in Configure for the current user - * Reads the state from redis - */ - public function setMonitoring($user) - { - if ( - !empty(Configure::read('Security.user_monitoring_enabled')) - ) { - $redis = $this->setupRedis(); - if (!empty($redis->sismember('misp:monitored_users', $user['id']))) { - Configure::write('Security.monitored', 1); - return true; - } - } - Configure::write('Security.monitored', 0); - } - public function registerUser($added_by, $registration, $org_id, $role_id) { $user = array( 'email' => $registration['data']['email'], diff --git a/app/View/AuthKeys/index.ctp b/app/View/AuthKeys/index.ctp index 0808a936c..9a04d3a7c 100644 --- a/app/View/AuthKeys/index.ctp +++ b/app/View/AuthKeys/index.ctp @@ -66,6 +66,14 @@ 'description' => empty($ajax) ? __('A list of API keys bound to a user.') : false, 'pull' => 'right', 'actions' => [ + [ + 'url' => $baseurl . '/auth_keys/view', + 'url_params_data_paths' => array( + 'AuthKey.id' + ), + 'icon' => 'eye', + 'dbclickAction' => true + ], [ 'onclick' => sprintf( 'openGenericModal(\'%s/authKeys/delete/[onclick_params_data_path]\');', diff --git a/app/View/AuthKeys/view.ctp b/app/View/AuthKeys/view.ctp index 9cf7ea767..cbc4c9a75 100644 --- a/app/View/AuthKeys/view.ctp +++ b/app/View/AuthKeys/view.ctp @@ -1,4 +1,17 @@ element( 'genericElements/SingleViews/single_view', [ @@ -38,9 +51,21 @@ echo $this->element( [ 'key' => __('Comment'), 'path' => 'AuthKey.comment' + ], + [ + 'key' => __('Key usage'), + 'type' => 'sparkline', + 'path' => 'AuthKey.id', + 'csv' => [ + 'data' => $keyUsageCsv, + ], + 'requirement' => isset($keyUsage), + ], + [ + 'key' => __('Last used'), + 'raw' => $lastUsed ? $lastUsed->format('Y-m-d H:i:s') : __('Not used yet'), + 'requirement' => isset($keyUsage), ] ], - 'children' => [ - ] ] ); diff --git a/app/View/Elements/genericElements/SingleViews/Fields/sparklineField.ctp b/app/View/Elements/genericElements/SingleViews/Fields/sparklineField.ctp new file mode 100644 index 000000000..b0cd91a4d --- /dev/null +++ b/app/View/Elements/genericElements/SingleViews/Fields/sparklineField.ctp @@ -0,0 +1,14 @@ +element('sparkline', array('scope' => $scope, 'id' => $elementId, 'csv' => $csv)); +} diff --git a/app/View/Elements/genericElements/SingleViews/single_view.ctp b/app/View/Elements/genericElements/SingleViews/single_view.ctp index 89c061fc2..46c31222b 100644 --- a/app/View/Elements/genericElements/SingleViews/single_view.ctp +++ b/app/View/Elements/genericElements/SingleViews/single_view.ctp @@ -30,6 +30,10 @@ $listElements = ''; if (!empty($fields)) { foreach ($fields as $field) { + if (isset($field['requirement']) && !$field['requirement']) { + continue; + } + if (empty($field['type'])) { $field['type'] = 'generic'; } From 2ae6108b52e7b5c3e382694688e9dc9bd6943de3 Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Sun, 29 Nov 2020 21:30:57 +0100 Subject: [PATCH 15/42] new: [test] Check when `MISP.authkey_keep_session` is true --- app/Model/Server.php | 11 ++++++++++- tests/testlive_security.py | 8 +++++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/app/Model/Server.php b/app/Model/Server.php index 909894228..0b7eb282e 100644 --- a/app/Model/Server.php +++ b/app/Model/Server.php @@ -1156,7 +1156,7 @@ class Server extends AppModel 'test' => 'testForPositiveInteger', 'type' => 'numeric', 'null' => true, - ] + ], ), 'GnuPG' => array( 'branch' => 1, @@ -1343,6 +1343,15 @@ class Server extends AppModel 'test' => 'testBool', 'type' => 'boolean', ), + 'authkey_keep_session' => [ + 'level' => self::SETTING_OPTIONAL, + 'description' => __('When enabled, session is kept between API requests.'), + 'value' => false, + 'errorMessage' => '', + 'test' => 'testBool', + 'type' => 'boolean', + 'null' => true, + ], 'auth_enforced' => [ 'level' => self::SETTING_OPTIONAL, 'description' => __('This optional can be enabled if external auth provider is used and when set to true, it will disable default form authentication.'), diff --git a/tests/testlive_security.py b/tests/testlive_security.py index 328b0a5b6..a0cd0a58c 100644 --- a/tests/testlive_security.py +++ b/tests/testlive_security.py @@ -481,6 +481,12 @@ class TestSecurity(unittest.TestCase): self.__delete_advanced_authkey(auth_key["id"]) + def test_authkey_keep_session(self): + with MISPSetting(self.admin_misp_connector, "Security.authkey_keep_session", True): + logged_in = PyMISP(url, self.test_usr.authkey) + check_response(logged_in.get_user()) + check_response(logged_in.get_user()) + def test_change_login(self): new_email = 'testusr@user' + random() + '.local' @@ -530,7 +536,7 @@ class TestSecurity(unittest.TestCase): # Try to change email as org admin new_email = 'testusr@user' + random() + '.local' updated_user = self.org_admin_misp_connector.update_user({'email': new_email}, self.test_usr) - self.__assertErrorResponse(updated_user) + self.assertEqual(self.test_usr.email, updated_user.email, "Email should be still same") def test_change_pw_disabled(self): with self.__setting("MISP.disable_user_password_change", True): From 8662a7efaf18318bc15fa7157813dc2575cf147a Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Mon, 30 Nov 2020 17:16:31 +0100 Subject: [PATCH 16/42] chg: [internal] Move access monitoring to own method --- app/Controller/AppController.php | 135 +++++++++++++++---------------- tests/testlive_security.py | 93 ++++++++++++++++++++- 2 files changed, 155 insertions(+), 73 deletions(-) diff --git a/app/Controller/AppController.php b/app/Controller/AppController.php index 5a63b1fe1..fc30dfba9 100755 --- a/app/Controller/AppController.php +++ b/app/Controller/AppController.php @@ -195,7 +195,7 @@ class AppController extends Controller } Configure::write('CurrentController', $this->params['controller']); Configure::write('CurrentAction', $this->params['action']); - $versionArray = $this->{$this->modelClass}->checkMISPVersion(); + $versionArray = $this->User->checkMISPVersion(); $this->mispVersion = implode('.', array_values($versionArray)); $this->Security->blackHoleCallback = 'blackHole'; @@ -236,11 +236,10 @@ class AppController extends Controller } } - $userMonitoringEnabled = false; if ($this->Auth->user()) { $user = $this->Auth->user(); Configure::write('CurrentUserId', $user['id']); - $userMonitoringEnabled = $this->__accessMonitor($user); + $this->__logAccess($user); // update script if ($user['Role']['perm_site_admin'] || (Configure::read('MISP.live') && !$this->_isRest())) { $this->{$this->modelClass}->runUpdates(); @@ -277,10 +276,11 @@ class AppController extends Controller // We don't want to run these role checks before the user is logged in, but we want them available for every view once the user is logged on if ($this->Auth->user()) { + $user = $this->Auth->user(); $this->set('mispVersion', implode('.', array($versionArray['major'], $versionArray['minor'], 0))); $this->set('mispVersionFull', $this->mispVersion); - $role = $this->Auth->user('Role'); - $this->set('me', $this->Auth->user()); + $role = $user['Role']; + $this->set('me', $user); $this->set('isAdmin', $role['perm_admin']); $this->set('isSiteAdmin', $role['perm_site_admin']); $this->set('hostOrgUser', $this->Auth->user('org_id') == Configure::read('MISP.host_org_id')); @@ -307,41 +307,8 @@ class AppController extends Controller $this->userRole = $role; $this->set('loggedInUserName', $this->__convertEmailToName($this->Auth->user('email'))); + $this->__accessMonitor($user); - if ( - Configure::read('MISP.log_paranoid') || - $userMonitoringEnabled - ) { - $this->Log = ClassRegistry::init('Log'); - $this->Log->create(); - $change = 'HTTP method: ' . $_SERVER['REQUEST_METHOD'] . PHP_EOL . 'Target: ' . $this->here; - if ( - ( - $this->request->is('post') || - $this->request->is('put') - ) && - ( - !empty(Configure::read('MISP.log_paranoid_include_post_body')) || - $userMonitoringEnabled - ) - ) { - $payload = $this->request->input(); - if (!empty($payload['_Token'])) { - unset($payload['_Token']); - } - $change .= PHP_EOL . 'Request body: ' . json_encode($payload); - } - $log = array( - 'org' => $this->Auth->user('Organisation')['name'], - 'model' => 'User', - 'model_id' => $this->Auth->user('id'), - 'email' => $this->Auth->user('email'), - 'action' => 'request', - 'title' => 'Paranoid log entry', - 'change' => $change, - ); - $this->Log->save($log); - } } else { $this->set('me', false); } @@ -381,9 +348,9 @@ class AppController extends Controller // Notifications and homepage is not necessary for AJAX or REST requests if ($this->Auth->user() && !$this->_isRest() && !$this->request->is('ajax')) { if ($this->request->params['controller'] === 'users' && $this->request->params['action'] === 'dashboard') { - $notifications = $this->{$this->modelClass}->populateNotifications($this->Auth->user()); + $notifications = $this->User->populateNotifications($this->Auth->user()); } else { - $notifications = $this->{$this->modelClass}->populateNotifications($this->Auth->user(), 'fast'); + $notifications = $this->User->populateNotifications($this->Auth->user(), 'fast'); } $this->set('notifications', $notifications); @@ -458,7 +425,7 @@ class AppController extends Controller } else { // User not authenticated correctly // reset the session information - $redis = $this->{$this->modelClass}->setupRedis(); + $redis = $this->User->setupRedis(); // Do not log every fail, but just once per hour if ($redis && !$redis->exists('misp:auth_fail_throttling:' . $authKeyToStore)) { $redis->setex('misp:auth_fail_throttling:' . $authKeyToStore, 3600, 1); @@ -623,48 +590,74 @@ class AppController extends Controller /** * User access monitoring * @param array $user - * @return bool True is user monitoring is enabled */ - private function __accessMonitor(array $user) + private function __logAccess(array $user) { - $userMonitoringEnabled = Configure::read('Security.user_monitoring_enabled'); $logUserIps = Configure::read('MISP.log_user_ips'); - - if (!$userMonitoringEnabled && !$logUserIps) { - return false; + if (!$logUserIps) { + return; } $redis = $this->User->setupRedis(); if (!$redis) { - return false; + return; } - if ($logUserIps) { - $remoteAddress = trim($_SERVER['REMOTE_ADDR']); + $remoteAddress = trim($_SERVER['REMOTE_ADDR']); - $pipe = $redis->multi(Redis::PIPELINE); - // keep for 30 days - $pipe->setex('misp:ip_user:' . $remoteAddress, 60 * 60 * 24 * 30, $user['id']); - $pipe->sadd('misp:user_ip:' . $user['id'], $remoteAddress); + $pipe = $redis->multi(Redis::PIPELINE); + // keep for 30 days + $pipe->setex('misp:ip_user:' . $remoteAddress, 60 * 60 * 24 * 30, $user['id']); + $pipe->sadd('misp:user_ip:' . $user['id'], $remoteAddress); - // Log key usage if enabled - if (isset($user['authkey_id']) && Configure::read('MISP.log_user_ips_auth')) { - // Use request time if defined - $time = isset($_SERVER['REQUEST_TIME']) ? $_SERVER['REQUEST_TIME'] : time(); - $hashKey = date("Y-m-d", $time) . ":$remoteAddress"; - $pipe->hIncrBy("misp:authkey_usage:{$user['authkey_id']}", $hashKey, 1); - // delete after one year of inactivity - $pipe->expire("misp:authkey_usage:{$user['authkey_id']}", 3600 * 24 * 365); - $pipe->set("misp:authkey_last_usage:{$user['authkey_id']}", $time); + // Log key usage if enabled + if (isset($user['authkey_id']) && Configure::read('MISP.log_user_ips_auth')) { + // Use request time if defined + $time = isset($_SERVER['REQUEST_TIME']) ? $_SERVER['REQUEST_TIME'] : time(); + $hashKey = date("Y-m-d", $time) . ":$remoteAddress"; + $pipe->hIncrBy("misp:authkey_usage:{$user['authkey_id']}", $hashKey, 1); + // delete after one year of inactivity + $pipe->expire("misp:authkey_usage:{$user['authkey_id']}", 3600 * 24 * 365); + $pipe->set("misp:authkey_last_usage:{$user['authkey_id']}", $time); + } + $pipe->exec(); + } + + /** + * @param array $user + * @throws Exception + */ + private function __accessMonitor(array $user) + { + $userMonitoringEnabled = Configure::read('Security.user_monitoring_enabled'); + if ($userMonitoringEnabled) { + $redis = $this->User->setupRedis(); + $userMonitoringEnabled = $redis && $redis->sismember('misp:monitored_users', $user['id']); + } + + if (Configure::read('MISP.log_paranoid') || $userMonitoringEnabled) { + $change = 'HTTP method: ' . $_SERVER['REQUEST_METHOD'] . PHP_EOL . 'Target: ' . $this->here; + if ( + ( + $this->request->is('post') || + $this->request->is('put') + ) && + ( + !empty(Configure::read('MISP.log_paranoid_include_post_body')) || + $userMonitoringEnabled + ) + ) { + $payload = $this->request->input(); + unset($payload['_Token']); + $change .= PHP_EOL . 'Request body: ' . json_encode($payload); + } + $this->Log = ClassRegistry::init('Log'); + try { + $this->Log->createLogEntry($user, 'request', 'User', $user['id'], 'Paranoid log entry', $change); + } catch (Exception $e) { + // When `MISP.log_skip_db_logs_completely` is enabled, Log::createLogEntry method throws exception } - $pipe->exec(); } - - // Return true for the current user if monitoring of this user is enabled in Redis. - if ($userMonitoringEnabled && $redis->sismember('misp:monitored_users', $user['id'])) { - return true; - } - return false; } private function __rateLimitCheck() diff --git a/tests/testlive_security.py b/tests/testlive_security.py index a0cd0a58c..9c0e06968 100644 --- a/tests/testlive_security.py +++ b/tests/testlive_security.py @@ -3,7 +3,7 @@ import os import sys import json import unittest -from typing import Union +from typing import Union, List import urllib3 # type: ignore import logging import uuid @@ -14,7 +14,7 @@ from lxml.html import fromstring from enum import Enum try: - from pymisp import PyMISP, MISPOrganisation, MISPUser, MISPRole, MISPSharingGroup, MISPEvent + from pymisp import PyMISP, MISPOrganisation, MISPUser, MISPRole, MISPSharingGroup, MISPEvent, MISPLog from pymisp.exceptions import PyMISPError, NoKey, MISPServerError except ImportError: if sys.version_info < (3, 6): @@ -825,6 +825,90 @@ class TestSecurity(unittest.TestCase): with self.__setting(config): PyMISP(url, self.test_usr.authkey) + def test_user_monitoring_enabled_no_user(self): + request_logs_before = self.__get_logs(action="request") + + with MISPSetting(self.admin_misp_connector, "Security.user_monitoring_enabled", True): + logged_in = PyMISP(url, self.test_usr.authkey) + check_response(logged_in.get_user()) + + request_logs_after = self.__get_logs(action="request") + # Number of logs should be same, because user is not monitored + self.assertEqual(len(request_logs_after), len(request_logs_before)) + + def test_user_monitoring_enabled_add_user(self): + request_logs_before = self.__get_logs(action="request") + + with MISPSetting(self.admin_misp_connector, "Security.user_monitoring_enabled", True): + # Enable monitoring of test user + send(self.admin_misp_connector, "POST", f"/admin/users/monitor/{self.test_usr.id}", { + "value": 1, + }) + + logged_in = PyMISP(url, self.test_usr.authkey) + check_response(logged_in.get_user()) + + # Disable monitoring of test user + send(self.admin_misp_connector, "POST", f"/admin/users/monitor/{self.test_usr.id}", { + "value": 0, + }) + + request_logs_after = self.__get_logs(action="request") + self.assertGreater(len(request_logs_after), len(request_logs_before)) + + def test_log_paranoid(self): + request_logs_before = self.__get_logs(action="request") + + with MISPSetting(self.admin_misp_connector, "MISP.log_paranoid", True): + logged_in = PyMISP(url, self.test_usr.authkey) + check_response(logged_in.get_user()) + + request_logs_after = self.__get_logs(action="request") + self.assertGreater(len(request_logs_after), len(request_logs_before), "Number of logs should be greater") + + def test_log_paranoid_include_post_body(self): + request_logs_before = self.__get_logs(action="request") + + with MISPComplexSetting({ + "MISP": { + "log_paranoid": True, + "log_paranoid_include_post_body": True, + } + }): + logged_in = PyMISP(url, self.test_usr.authkey) + check_response(logged_in.get_user()) + + request_logs_after = self.__get_logs(action="request") + self.assertGreater(len(request_logs_after), len(request_logs_before), "Number of logs should be greater") + + def test_log_paranoid_skip_db(self): + request_logs_before = self.__get_logs(action="request") + + with MISPComplexSetting({ + "MISP": { + "log_paranoid": True, + "log_paranoid_skip_db": True, + } + }): + logged_in = PyMISP(url, self.test_usr.authkey) + check_response(logged_in.get_user()) + + request_logs_after = self.__get_logs(action="request") + # Number of logs should be same, because saving to database is disabled + self.assertEqual(len(request_logs_after), len(request_logs_before)) + + def test_log_auth_fail_multiple(self): + request_logs_before = self.__get_logs(action="auth_fail") + + with self.assertRaises(PyMISPError): + PyMISP(url, "JCZDbBr3wYPlY0DrlQzoD8EWrcClGc0Dqu2yMYyE") + with self.assertRaises(PyMISPError): + PyMISP(url, "JCZDbBr3wYPlY0DrlQzoD8EWrcClGc0Dqu2yMYyE") + + request_logs_after = self.__get_logs(action="auth_fail") + # Just one new record should be logged for multiple tries with same key + self.assertEqual(len(request_logs_after), len(request_logs_before) + 1) + def test_sg_index_user_cannot_see(self): org = self.__create_org() hidden_sg = self.__create_sharing_group() @@ -1125,6 +1209,11 @@ class TestSecurity(unittest.TestCase): def __delete_advanced_authkey(self, key_id: int): return send(self.admin_misp_connector, "POST", f'authKeys/delete/{key_id}') + def __get_logs(self, action: str) -> List[MISPLog]: + response = self.admin_misp_connector.search_logs(action=action) + check_response(response) + return response + def __setting(self, key, value=None) -> MISPSetting: if not isinstance(key, dict): new_setting = {key: value} From 4c6ffc69852b2dc58ea203408643c13fec3f0aea Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Mon, 30 Nov 2020 18:53:55 +0100 Subject: [PATCH 17/42] chg: [internal] Rename MISP.log_user_ips_auth -> MISP.log_user_ips_authkeys --- app/Controller/AppController.php | 2 +- app/Controller/AuthKeysController.php | 4 ++-- app/Model/Server.php | 9 +++++++++ tests/testlive_security.py | 15 +++++++++++++++ 4 files changed, 27 insertions(+), 3 deletions(-) diff --git a/app/Controller/AppController.php b/app/Controller/AppController.php index fc30dfba9..045d8e08c 100755 --- a/app/Controller/AppController.php +++ b/app/Controller/AppController.php @@ -611,7 +611,7 @@ class AppController extends Controller $pipe->sadd('misp:user_ip:' . $user['id'], $remoteAddress); // Log key usage if enabled - if (isset($user['authkey_id']) && Configure::read('MISP.log_user_ips_auth')) { + if (isset($user['authkey_id']) && Configure::read('MISP.log_user_ips_authkeys')) { // Use request time if defined $time = isset($_SERVER['REQUEST_TIME']) ? $_SERVER['REQUEST_TIME'] : time(); $hashKey = date("Y-m-d", $time) . ":$remoteAddress"; diff --git a/app/Controller/AuthKeysController.php b/app/Controller/AuthKeysController.php index da2499dda..edd97b83d 100644 --- a/app/Controller/AuthKeysController.php +++ b/app/Controller/AuthKeysController.php @@ -32,7 +32,7 @@ class AuthKeysController extends AppController 'contain' => ['User.id', 'User.email'], 'conditions' => $conditions, 'afterFind' => function (array $authKeys) { - $keyUsageEnabled = Configure::read('MISP.log_user_ips') && Configure::read('MISP.log_user_ips_auth'); + $keyUsageEnabled = Configure::read('MISP.log_user_ips') && Configure::read('MISP.log_user_ips_authkeys'); foreach ($authKeys as &$authKey) { if ($keyUsageEnabled) { $lastUsed = $this->AuthKey->getKeyUsage($authKey['AuthKey']['id'])[1]; @@ -110,7 +110,7 @@ class AuthKeysController extends AppController return $this->restResponsePayload; } - if (Configure::read('MISP.log_user_ips') && Configure::read('MISP.log_user_ips_auth')) { + if (Configure::read('MISP.log_user_ips') && Configure::read('MISP.log_user_ips_authkeys')) { list($keyUsage, $lastUsed) = $this->AuthKey->getKeyUsage($id); $this->set('keyUsage', $keyUsage); $this->set('lastUsed', $lastUsed); diff --git a/app/Model/Server.php b/app/Model/Server.php index 0b7eb282e..a11e0e1ae 100644 --- a/app/Model/Server.php +++ b/app/Model/Server.php @@ -865,6 +865,15 @@ class Server extends AppModel 'type' => 'boolean', 'null' => true ), + 'log_user_ips_authkeys' => [ + 'level' => self::SETTING_RECOMMENDED, + 'description' => __('Log user IP and key usage on each API request. All logs for given keys are deleted after one year when this key is not used.'), + 'value' => false, + 'errorMessage' => '', + 'test' => 'testBool', + 'type' => 'boolean', + 'null' => true + ], 'delegation' => array( 'level' => 1, 'description' => __('This feature allows users to create org only events and ask another organisation to take ownership of the event. This allows organisations to remain anonymous by asking a partner to publish an event for them.'), diff --git a/tests/testlive_security.py b/tests/testlive_security.py index 9c0e06968..a886c9090 100644 --- a/tests/testlive_security.py +++ b/tests/testlive_security.py @@ -909,6 +909,21 @@ class TestSecurity(unittest.TestCase): # Just one new record should be logged for multiple tries with same key self.assertEqual(len(request_logs_after), len(request_logs_before) + 1) + def test_log_user_ips(self): + with MISPSetting(self.admin_misp_connector, "MISP.log_user_ips", True): + logged_in = PyMISP(url, self.test_usr.authkey) + check_response(logged_in.get_user()) + + def test_log_user_ips_auth(self): + with MISPComplexSetting({ + "MISP": { + "log_user_ips": True, + "log_user_ips_authkeys": True, + } + }): + logged_in = PyMISP(url, self.test_usr.authkey) + check_response(logged_in.get_user()) + def test_sg_index_user_cannot_see(self): org = self.__create_org() hidden_sg = self.__create_sharing_group() From feab5f553b838343f27bb580aca2b25c17f8f63d Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Mon, 30 Nov 2020 21:32:53 +0100 Subject: [PATCH 18/42] chg: [interna] AppController code cleanup --- app/Controller/AppController.php | 54 +++++++++++++++----------------- 1 file changed, 25 insertions(+), 29 deletions(-) diff --git a/app/Controller/AppController.php b/app/Controller/AppController.php index 045d8e08c..d8eacf745 100755 --- a/app/Controller/AppController.php +++ b/app/Controller/AppController.php @@ -40,7 +40,7 @@ App::uses('RequestRearrangeTool', 'Tools'); * @property RestResponseComponent $RestResponse * @property CRUDComponent $CRUD * @property IndexFilterComponent $IndexFilter - * @property User $User + * @property RateLimitComponent $RateLimit */ class AppController extends Controller { @@ -236,16 +236,17 @@ class AppController extends Controller } } - if ($this->Auth->user()) { - $user = $this->Auth->user(); + $user = $this->Auth->user(); + if ($user) { Configure::write('CurrentUserId', $user['id']); $this->__logAccess($user); + // update script if ($user['Role']['perm_site_admin'] || (Configure::read('MISP.live') && !$this->_isRest())) { - $this->{$this->modelClass}->runUpdates(); + $this->User->runUpdates(); } - if (!$this->__verifyUser($this->User, $user)) { + if (!$this->__verifyUser($user)) { $this->_stop(); // just for sure } @@ -261,29 +262,14 @@ class AppController extends Controller ini_set('max_execution_time', $user['Role']['max_execution_time']); } } - } else { - $pre_auth_actions = array('login', 'register', 'getGpgPublicKey'); - if (!empty(Configure::read('Security.email_otp_enabled'))) { - $pre_auth_actions[] = 'email_otp'; - } - if (!$this->_isControllerAction(['users' => $pre_auth_actions])) { - if (!$this->request->is('ajax')) { - $this->Session->write('pre_login_requested_url', $this->here); - } - $this->_redirectToLogin(); - } - } - // We don't want to run these role checks before the user is logged in, but we want them available for every view once the user is logged on - if ($this->Auth->user()) { - $user = $this->Auth->user(); $this->set('mispVersion', implode('.', array($versionArray['major'], $versionArray['minor'], 0))); $this->set('mispVersionFull', $this->mispVersion); - $role = $user['Role']; $this->set('me', $user); + $role = $user['Role']; $this->set('isAdmin', $role['perm_admin']); $this->set('isSiteAdmin', $role['perm_site_admin']); - $this->set('hostOrgUser', $this->Auth->user('org_id') == Configure::read('MISP.host_org_id')); + $this->set('hostOrgUser', $user['org_id'] == Configure::read('MISP.host_org_id')); $this->set('isAclAdd', $role['perm_add']); $this->set('isAclModify', $role['perm_modify']); $this->set('isAclModifyOrg', $role['perm_modify_org']); @@ -306,10 +292,21 @@ class AppController extends Controller $this->set('aclComponent', $this->ACL); $this->userRole = $role; - $this->set('loggedInUserName', $this->__convertEmailToName($this->Auth->user('email'))); + $this->set('loggedInUserName', $this->__convertEmailToName($user['email'])); $this->__accessMonitor($user); } else { + $pre_auth_actions = array('login', 'register', 'getGpgPublicKey'); + if (!empty(Configure::read('Security.email_otp_enabled'))) { + $pre_auth_actions[] = 'email_otp'; + } + if (!$this->_isControllerAction(['users' => $pre_auth_actions])) { + if (!$this->request->is('ajax')) { + $this->Session->write('pre_login_requested_url', $this->here); + } + $this->_redirectToLogin(); + } + $this->set('me', false); } @@ -459,11 +456,10 @@ class AppController extends Controller * - must change password * - reads latest news * - * @param User $userModel * @param array $user * @return bool */ - private function __verifyUser(User $userModel, array $user) + private function __verifyUser(array $user) { // Skip these checks for 'checkIfLoggedIn' action to make that call fast if ($this->_isControllerAction(['users' => ['checkIfLoggedIn']])) { @@ -471,7 +467,7 @@ class AppController extends Controller } // Load last user profile modification from database - $userFromDb = $userModel->find('first', [ + $userFromDb = $this->User->find('first', [ 'conditions' => ['id' => $user['id']], 'recursive' => -1, 'fields' => ['date_modified'], @@ -520,8 +516,8 @@ class AppController extends Controller // Force logout doesn't make sense for API key authentication if (!$this->isApiAuthed && $user['force_logout']) { - $userModel->id = $user['id']; - $userModel->saveField('force_logout', false); + $this->User->id = $user['id']; + $this->User->saveField('force_logout', false); $this->Auth->logout(); $this->_redirectToLogin(); return false; @@ -690,7 +686,7 @@ class AppController extends Controller public function afterFilter() { - if ($this->isApiAuthed && $this->_isRest() &&!Configure::read('Security.authkey_keep_session')) { + if ($this->isApiAuthed && $this->_isRest() && !Configure::read('Security.authkey_keep_session')) { $this->Session->destroy(); } } From 9896f67358fa1055b427849787b9006894eedb11 Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Tue, 1 Dec 2020 17:15:48 +0100 Subject: [PATCH 19/42] new: [security] New setting Security.username_in_response_header --- app/Controller/AppController.php | 13 +++++++- app/Controller/UsersController.php | 8 ----- app/Model/AuthKey.php | 4 +++ app/Model/Server.php | 12 ++++++- tests/testlive_security.py | 52 ++++++++++++++++++++++++------ 5 files changed, 70 insertions(+), 19 deletions(-) diff --git a/app/Controller/AppController.php b/app/Controller/AppController.php index d8eacf745..075013c3d 100755 --- a/app/Controller/AppController.php +++ b/app/Controller/AppController.php @@ -241,11 +241,21 @@ class AppController extends Controller Configure::write('CurrentUserId', $user['id']); $this->__logAccess($user); - // update script + // If user is site admin, try to run updates if ($user['Role']['perm_site_admin'] || (Configure::read('MISP.live') && !$this->_isRest())) { $this->User->runUpdates(); } + // Put username to response header for webserver or proxy logging + if (Configure::read('Security.username_in_response_header')) { + $headerValue = $user['email']; + if (isset($user['logged_by_authkey']) && $user['logged_by_authkey']) { + $headerValue .= isset($user['authkey_id']) ? "/API/{$user['authkey_id']}" : '/API/default'; + } + $this->response->header('X-Username', $headerValue); + $this->RestResponse->setHeader('X-Username', $headerValue); + } + if (!$this->__verifyUser($user)) { $this->_stop(); // just for sure } @@ -930,6 +940,7 @@ class AppController extends Controller if (!$user['Role']['perm_auth']) { return false; } + $user['logged_by_authkey'] = true; return $user; } diff --git a/app/Controller/UsersController.php b/app/Controller/UsersController.php index a558282bc..7090df547 100644 --- a/app/Controller/UsersController.php +++ b/app/Controller/UsersController.php @@ -51,14 +51,6 @@ class UsersController extends AppController if (!$this->_isSiteAdmin() && $this->Auth->user('id') != $id) { throw new NotFoundException(__('Invalid user or not authorised.')); } - if (!is_numeric($id) && !empty($id)) { - $userId = $this->User->find('first', array( - 'conditions' => array('email' => $id), - 'fields' => array('id') - )); - $id = $userid['User']['id']; - } - $user = $this->User->read(null, $id); $user = $this->User->find('first', array( 'recursive' => -1, 'conditions' => array('User.id' => $id), diff --git a/app/Model/AuthKey.php b/app/Model/AuthKey.php index 9ab1e40f9..60a9d6c9f 100644 --- a/app/Model/AuthKey.php +++ b/app/Model/AuthKey.php @@ -51,6 +51,10 @@ class AuthKey extends AppModel return true; } + /** + * @param string $authkey + * @return array|false + */ public function getAuthUserByAuthKey($authkey) { $start = substr($authkey, 0, 4); diff --git a/app/Model/Server.php b/app/Model/Server.php index a11e0e1ae..59b15ba11 100644 --- a/app/Model/Server.php +++ b/app/Model/Server.php @@ -1603,7 +1603,16 @@ class Server extends AppModel 'test' => 'testBool', 'type' => 'boolean', 'null' => true - ) + ), + 'username_in_response_header' => [ + 'level' => self::SETTING_OPTIONAL, + 'description' => __('When enabled, logged in username will be included in X-Username HTTP response header. This is useful for request logging on webserver/proxy side.'), + 'value' => false, + 'errorMessage' => '', + 'test' => 'testBool', + 'type' => 'boolean', + 'null' => true + ] ), 'SecureAuth' => array( 'branch' => 1, @@ -4934,6 +4943,7 @@ class Server extends AppModel public function dbSchemaDiagnostic() { + $this->AdminSetting = ClassRegistry::init('AdminSetting'); $actualDbVersion = $this->AdminSetting->find('first', array( 'conditions' => array('setting' => 'db_version') ))['AdminSetting']['value']; diff --git a/tests/testlive_security.py b/tests/testlive_security.py index a886c9090..463ee51aa 100644 --- a/tests/testlive_security.py +++ b/tests/testlive_security.py @@ -45,7 +45,7 @@ def check_response(response): raise Exception(response["errors"]) -def login(url: str, email: str, password: str) -> bool: +def login(url: str, email: str, password: str) -> requests.Session: session = requests.Session() r = session.get(url) @@ -77,7 +77,7 @@ def login(url: str, email: str, password: str) -> bool: r = r.json() if email != r["User"]["email"]: raise Exception(r) # logged in as different user - return True + return session class MISPSetting: @@ -166,7 +166,7 @@ class TestSecurity(unittest.TestCase): # Try to connect as user to check if everything works PyMISP(url, cls.test_usr.authkey) # Check if user can login with given password - assert login(url, cls.test_usr.email, cls.test_usr_password) + assert login(url, cls.test_usr.email, cls.test_usr_password) is not False @classmethod def tearDownClass(cls): @@ -302,7 +302,7 @@ class TestSecurity(unittest.TestCase): self.assertFalse(updated_user.disabled) # Try to login - self.assertTrue(login(url, self.test_usr.email, self.test_usr_password)) + self.assertIsInstance(login(url, self.test_usr.email, self.test_usr_password), requests.Session) def test_disabled_user_api_access(self): # Disable user @@ -327,7 +327,7 @@ class TestSecurity(unittest.TestCase): self.assertFalse(login(url, self.test_usr.email, self.test_usr_password)) # Check if user can login with given password - self.assertTrue(login(url, self.test_usr.email, self.test_usr_password)) + self.assertIsInstance(login(url, self.test_usr.email, self.test_usr_password), requests.Session) def test_disabled_misp_api_access(self): with self.__setting("MISP.live", False): @@ -545,7 +545,7 @@ class TestSecurity(unittest.TestCase): logged_in.change_user_password(str(uuid.uuid4())) # Password should be still the same - self.assertTrue(login(url, self.test_usr.email, self.test_usr_password)) + self.assertIsInstance(login(url, self.test_usr.email, self.test_usr_password), requests.Session) def test_change_pw_disabled_different_way(self): with self.__setting("MISP.disable_user_password_change", True): @@ -554,14 +554,14 @@ class TestSecurity(unittest.TestCase): logged_in.update_user({"password": str(uuid.uuid4())}, self.test_usr.id) # Password should be still the same - self.assertTrue(login(url, self.test_usr.email, self.test_usr_password)) + self.assertIsInstance(login(url, self.test_usr.email, self.test_usr_password), requests.Session) def test_change_pw_disabled_by_org_admin(self): with self.__setting("MISP.disable_user_password_change", True): self.org_admin_misp_connector.update_user({"password": str(uuid.uuid4())}, self.test_usr.id) # Password should be still the same - self.assertTrue(login(url, self.test_usr.email, self.test_usr_password)) + self.assertIsInstance(login(url, self.test_usr.email, self.test_usr_password), requests.Session) def test_add_user_by_org_admin(self): user = MISPUser() @@ -790,7 +790,7 @@ class TestSecurity(unittest.TestCase): def test_shibb_form_login(self): with self.__setting(self.__default_shibb_config()): # Form login should still works when no header provided - self.assertTrue(login(url, self.test_usr.email, self.test_usr_password)) + self.assertIsInstance(login(url, self.test_usr.email, self.test_usr_password), requests.Session) def test_shibb_api_login(self): with self.__setting(self.__default_shibb_config()): @@ -924,6 +924,40 @@ class TestSecurity(unittest.TestCase): logged_in = PyMISP(url, self.test_usr.authkey) check_response(logged_in.get_user()) + def test_username_in_response_header(self): + with MISPSetting(self.admin_misp_connector, "Security.username_in_response_header", True): + logged_in = login(url, self.test_usr.email, self.test_usr_password) + self.assertIsInstance(logged_in, requests.Session) + + response = logged_in.get(url + "/users/view/me.json") + self.assertIn("X-Username", response.headers) + self.assertEqual(self.test_usr.email, response.headers["X-Username"]) + + def test_username_in_response_header_api_access(self): + with MISPSetting(self.admin_misp_connector, "Security.username_in_response_header", True): + logged_in = PyMISP(url, self.test_usr.authkey) + + response = logged_in._prepare_request('GET', 'users/view/me') + self.assertIn("X-Username", response.headers) + self.assertEqual(self.test_usr.email + "/API/default", response.headers["X-Username"]) + + def test_username_in_response_header_advanced_api_access(self): + with MISPComplexSetting({ + "Security": { + "advanced_authkeys": True, + "username_in_response_header": True, + } + }): + auth_key = self.__create_advanced_authkey(self.test_usr.id) + + logged_in = PyMISP(url, auth_key["authkey_raw"]) + response = logged_in._prepare_request('GET', 'users/view/me') + + self.__delete_advanced_authkey(auth_key["id"]) + + self.assertIn("X-Username", response.headers) + self.assertEqual(f"{self.test_usr.email}/API/{auth_key['id']}", response.headers["X-Username"]) + def test_sg_index_user_cannot_see(self): org = self.__create_org() hidden_sg = self.__create_sharing_group() From d92123c915efe4502cc1c7263261ce5da2b536af Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Wed, 2 Dec 2020 19:35:31 +0100 Subject: [PATCH 20/42] fix: [security] Do not allow to use API key authenticated session to do non API calls --- app/Controller/AppController.php | 11 +++++------ .../Component/IndexFilterComponent.php | 18 ++++++++++-------- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/app/Controller/AppController.php b/app/Controller/AppController.php index 075013c3d..50e6b0b56 100755 --- a/app/Controller/AppController.php +++ b/app/Controller/AppController.php @@ -260,6 +260,10 @@ class AppController extends Controller $this->_stop(); // just for sure } + if (isset($user['logged_by_authkey']) && $user['logged_by_authkey'] && !($this->_isRest() || $this->_isAutomation())) { + throw new ForbiddenException("When user is authenticated by authkey, just REST request can be processed"); + } + $this->set('default_memory_limit', ini_get('memory_limit')); if (isset($user['Role']['memory_limit'])) { if ($user['Role']['memory_limit'] !== '') { @@ -788,12 +792,7 @@ class AppController extends Controller protected function _isAutomation() { - foreach ($this->automationArray as $controllerName => $controllerActions) { - if ($this->params['controller'] == $controllerName && in_array($this->params['action'], $controllerActions)) { - return true; - } - } - return false; + return $this->IndexFilter->isApiFunction($this->params['controller'], $this->params['action']); } /** diff --git a/app/Controller/Component/IndexFilterComponent.php b/app/Controller/Component/IndexFilterComponent.php index 4116d0ba4..53b75df35 100644 --- a/app/Controller/Component/IndexFilterComponent.php +++ b/app/Controller/Component/IndexFilterComponent.php @@ -6,7 +6,8 @@ class IndexFilterComponent extends Component { - public $Controller = false; + /** @var Controller */ + public $Controller; public $isRest = null; public function initialize(Controller $controller) { @@ -74,7 +75,7 @@ class IndexFilterComponent extends Component } } } - $this->Controller->set('passedArgs', json_encode($this->Controller->passedArgs, true)); + $this->Controller->set('passedArgs', json_encode($this->Controller->passedArgs)); return $data; } @@ -85,7 +86,7 @@ class IndexFilterComponent extends Component return $this->isRest; } $api = $this->isApiFunction($this->Controller->request->params['controller'], $this->Controller->request->params['action']); - if (isset($this->Controller->RequestHandler) && ($api || $this->Controller->RequestHandler->isXml() || $this->isJson() || $this->isCsv())) { + if (isset($this->Controller->RequestHandler) && ($api || $this->isJson() || $this->Controller->RequestHandler->isXml() || $this->isCsv())) { if ($this->isJson()) { if (!empty($this->Controller->request->input()) && empty($this->Controller->request->input('json_decode'))) { throw new MethodNotAllowedException('Invalid JSON input. Make sure that the JSON input is a correctly formatted JSON string. This request has been blocked to avoid an unfiltered request.'); @@ -117,12 +118,13 @@ class IndexFilterComponent extends Component } + /** + * @param string $controller + * @param string $action + * @return bool + */ public function isApiFunction($controller, $action) { - if (isset($this->Controller->automationArray[$controller]) && in_array($action, $this->Controller->automationArray[$controller])) { - return true; - } - return false; + return isset($this->Controller->automationArray[$controller]) && in_array($action, $this->Controller->automationArray[$controller], true); } - } From 8df77748b0a15f0068f6e6960b8becdc110b3b1f Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Wed, 2 Dec 2020 20:13:47 +0100 Subject: [PATCH 21/42] chg: [internal] Small optimisations --- app/Controller/AppController.php | 20 +++++++------------ .../Component/RestResponseComponent.php | 5 +++-- app/Model/AppModel.php | 2 +- 3 files changed, 11 insertions(+), 16 deletions(-) diff --git a/app/Controller/AppController.php b/app/Controller/AppController.php index 50e6b0b56..4b0db6665 100755 --- a/app/Controller/AppController.php +++ b/app/Controller/AppController.php @@ -46,8 +46,6 @@ class AppController extends Controller { public $defaultModel = ''; - public $debugMode = false; - public $helpers = array('OrgImg', 'FontAwesome', 'UserName', 'DataPathCollector'); private $__queryVersion = '119'; @@ -175,7 +173,8 @@ class AppController extends Controller $this->loadModel('Server'); $this->Server->serverSettingsSaveValue('MISP.uuid', CakeText::uuid()); } - // check if Apache provides kerberos authentication data + + // Check if Apache provides kerberos authentication data $authUserFields = $this->User->describeAuthFields(); $envvar = Configure::read('ApacheSecureAuth.apacheEnv'); if ($envvar && isset($_SERVER[$envvar])) { @@ -265,19 +264,15 @@ class AppController extends Controller } $this->set('default_memory_limit', ini_get('memory_limit')); - if (isset($user['Role']['memory_limit'])) { - if ($user['Role']['memory_limit'] !== '') { - ini_set('memory_limit', $user['Role']['memory_limit']); - } + if (isset($user['Role']['memory_limit']) && $user['Role']['memory_limit'] !== '') { + ini_set('memory_limit', $user['Role']['memory_limit']); } $this->set('default_max_execution_time', ini_get('max_execution_time')); - if (isset($user['Role']['max_execution_time'])) { - if ($user['Role']['max_execution_time'] !== '') { - ini_set('max_execution_time', $user['Role']['max_execution_time']); - } + if (isset($user['Role']['max_execution_time']) && $user['Role']['max_execution_time'] !== '') { + ini_set('max_execution_time', $user['Role']['max_execution_time']); } - $this->set('mispVersion', implode('.', array($versionArray['major'], $versionArray['minor'], 0))); + $this->set('mispVersion', "{$versionArray['major']}.{$versionArray['minor']}.0"); $this->set('mispVersionFull', $this->mispVersion); $this->set('me', $user); $role = $user['Role']; @@ -354,7 +349,6 @@ class AppController extends Controller } } } - $this->components['RestResponse']['sql_dump'] = $this->sql_dump; // Notifications and homepage is not necessary for AJAX or REST requests if ($this->Auth->user() && !$this->_isRest() && !$this->request->is('ajax')) { diff --git a/app/Controller/Component/RestResponseComponent.php b/app/Controller/Component/RestResponseComponent.php index f4d690de3..64692d01d 100644 --- a/app/Controller/Component/RestResponseComponent.php +++ b/app/Controller/Component/RestResponseComponent.php @@ -519,11 +519,12 @@ class RestResponseComponent extends Component } else { $type = $format; } + $dumpSql = !empty($this->Controller->sql_dump) && Configure::read('debug') > 1; if (!$raw) { if (is_string($response)) { $response = array('message' => $response); } - if (Configure::read('debug') > 1 && !empty($this->Controller->sql_dump)) { + if ($dumpSql) { $this->Log = ClassRegistry::init('Log'); if ($this->Controller->sql_dump === 2) { $response = array('sql_dump' => $this->Log->getDataSource()->getLog(false, false)); @@ -533,7 +534,7 @@ class RestResponseComponent extends Component } $response = json_encode($response, JSON_PRETTY_PRINT); } else { - if (Configure::read('debug') > 1 && !empty($this->Controller->sql_dump)) { + if ($dumpSql) { $this->Log = ClassRegistry::init('Log'); if ($this->Controller->sql_dump === 2) { $response = json_encode(array('sql_dump' => $this->Log->getDataSource()->getLog(false, false))); diff --git a/app/Model/AppModel.php b/app/Model/AppModel.php index da3da6841..92d9b0ab1 100644 --- a/app/Model/AppModel.php +++ b/app/Model/AppModel.php @@ -2741,7 +2741,7 @@ class AppModel extends Model { static $versionArray; if ($versionArray === null) { - $file = new File(ROOT . DS . 'VERSION.json', true); + $file = new File(ROOT . DS . 'VERSION.json'); $versionArray = $this->jsonDecode($file->read()); $file->close(); } From f27580f1e6881b9413cb4ac18f243ef582ba0d58 Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Thu, 3 Dec 2020 14:26:03 +0100 Subject: [PATCH 22/42] new: [security] Allow to set key validity --- app/Controller/AuthKeysController.php | 6 +++- app/Controller/Component/CRUDComponent.php | 16 +++++---- app/Model/AuthKey.php | 16 +++++++-- app/Model/Server.php | 9 +++++ app/View/AuthKeys/add.ctp | 4 +-- tests/testlive_security.py | 41 ++++++++++++++++++++++ 6 files changed, 81 insertions(+), 11 deletions(-) diff --git a/app/Controller/AuthKeysController.php b/app/Controller/AuthKeysController.php index edd97b83d..a7e6de56e 100644 --- a/app/Controller/AuthKeysController.php +++ b/app/Controller/AuthKeysController.php @@ -69,7 +69,6 @@ class AuthKeysController extends AppController public function add($user_id = false) { - $this->set('menuData', array('menuList' => $this->_isSiteAdmin() ? 'admin' : 'globalActions', 'menuItem' => 'authKeyAdd')); $params = [ 'displayOnSuccess' => 'authkey_display', 'saveModelVariable' => ['authkey_raw'] @@ -94,6 +93,11 @@ class AuthKeysController extends AppController ]) ]; $this->set(compact('dropdownData')); + $this->set('menuData', [ + 'menuList' => $this->_isSiteAdmin() ? 'admin' : 'globalActions', + 'menuItem' => 'authKeyAdd', + ]); + $this->set('validity', Configure::read('Security.advanced_authkeys_validity')); } public function view($id = false) diff --git a/app/Controller/Component/CRUDComponent.php b/app/Controller/Component/CRUDComponent.php index 01355d3c7..c15399ac7 100644 --- a/app/Controller/Component/CRUDComponent.php +++ b/app/Controller/Component/CRUDComponent.php @@ -86,17 +86,19 @@ class CRUDComponent extends Component } else { $data = $input; } - if ($this->Controller->{$modelName}->save($data)) { - $data = $this->Controller->{$modelName}->find('first', [ + /** @var Model $model */ + $model = $this->Controller->{$modelName}; + if ($model->save($data)) { + $data = $model->find('first', [ 'recursive' => -1, 'conditions' => [ - 'id' => $this->Controller->{$modelName}->id + 'id' => $model->id ] ]); if (!empty($params['saveModelVariable'])) { foreach ($params['saveModelVariable'] as $var) { - if (isset($this->Controller->{$modelName}->$var)) { - $data[$modelName][$var] = $this->Controller->{$modelName}->$var; + if (isset($model->$var)) { + $data[$modelName][$var] = $model->$var; } } } @@ -116,7 +118,9 @@ class CRUDComponent extends Component } else { $message = __('%s could not be added.', $modelName); if ($this->Controller->IndexFilter->isRest()) { - + $controllerName = $this->Controller->params['controller']; + $actionName = $this->Controller->params['action']; + $this->Controller->restResponsePayload = $this->Controller->RestResponse->saveFailResponse($controllerName, $actionName, false, $model->validationErrors, 'json'); } else { $this->Controller->Flash->error($message); } diff --git a/app/Model/AuthKey.php b/app/Model/AuthKey.php index 60a9d6c9f..17b702669 100644 --- a/app/Model/AuthKey.php +++ b/app/Model/AuthKey.php @@ -42,10 +42,22 @@ class AuthKey extends AppModel $this->data['AuthKey']['authkey_end'] = substr($authkey, -4); $this->data['AuthKey']['authkey_raw'] = $authkey; $this->authkey_raw = $authkey; + + $validity = Configure::read('Security.advanced_authkeys_validity'); if (empty($this->data['AuthKey']['expiration'])) { - $this->data['AuthKey']['expiration'] = 0; + $this->data['AuthKey']['expiration'] = $validity ? strtotime("+$validity days") : 0; } else { - $this->data['AuthKey']['expiration'] = strtotime($this->data['AuthKey']['expiration']); + $expiration = is_numeric($this->data['AuthKey']['expiration']) ? + (int)$this->data['AuthKey']['expiration'] : + strtotime($this->data['AuthKey']['expiration']); + + if ($expiration === false) { + $this->invalidate('expiration', __('Expiration must be in YYYY-MM-DD format.')); + } + if ($validity && $expiration > strtotime("+$validity days")) { + $this->invalidate('expiration', __('Maximal key validity is %s days.', $validity)); + } + $this->data['AuthKey']['expiration'] = $expiration; } } return true; diff --git a/app/Model/Server.php b/app/Model/Server.php index 59b15ba11..357d29c55 100644 --- a/app/Model/Server.php +++ b/app/Model/Server.php @@ -1352,6 +1352,15 @@ class Server extends AppModel 'test' => 'testBool', 'type' => 'boolean', ), + 'advanced_authkeys_validity' => [ + 'level' => self::SETTING_OPTIONAL, + 'description' => __('Maximal key lifetime in days. Use can limit that validity even more. Just newly created keys will be affected. When not set, key validity is not limited.'), + 'value' => '', + 'errorMessage' => '', + 'type' => 'numeric', + 'test' => 'testForNumeric', + 'null' => true, + ], 'authkey_keep_session' => [ 'level' => self::SETTING_OPTIONAL, 'description' => __('When enabled, session is kept between API requests.'), diff --git a/app/View/AuthKeys/add.ctp b/app/View/AuthKeys/add.ctp index 86a3e8ad7..62e5a40db 100644 --- a/app/View/AuthKeys/add.ctp +++ b/app/View/AuthKeys/add.ctp @@ -1,5 +1,4 @@ element('genericElements/Form/genericForm', [ 'data' => [ 'title' => __('Add auth key'), @@ -13,11 +12,12 @@ echo $this->element('genericElements/Form/genericForm', [ ], [ 'field' => 'comment', + 'label' => __('Comment'), 'class' => 'span6' ], [ 'field' => 'expiration', - 'label' => 'Expiration', + 'label' => __('Expiration (%s)', $validity ? __('keep empty for maximal validity of %s days', $validity) : __('keep empty for indefinite')), 'class' => 'datepicker span6', 'placeholder' => "YYYY-MM-DD", 'type' => 'text' diff --git a/tests/testlive_security.py b/tests/testlive_security.py index 463ee51aa..e2a055dd9 100644 --- a/tests/testlive_security.py +++ b/tests/testlive_security.py @@ -2,6 +2,7 @@ import os import sys import json +import datetime import unittest from typing import Union, List import urllib3 # type: ignore @@ -441,6 +442,46 @@ class TestSecurity(unittest.TestCase): self.__delete_advanced_authkey(auth_key["id"]) # TODO: Delete new key + def test_advanced_authkeys_expiration_invalid(self): + with MISPSetting(self.admin_misp_connector, "Security.advanced_authkeys", True): + with self.assertRaises(Exception) as cm: + self.__create_advanced_authkey(self.test_usr.id, {"expiration": "__nonsense__"}) + self.assertIn("expiration", cm.exception.args[0][1]["errors"]) + + def test_advanced_authkeys_validity_autoset(self): + with MISPComplexSetting({ + "Security": { + "advanced_authkeys": True, + "advanced_authkeys_validity": 365, + } + }): + auth_key = self.__create_advanced_authkey(self.test_usr.id) + self.assertNotEqual(0, auth_key["expiration"]) + + def test_advanced_authkeys_validity_in_range(self): + with MISPComplexSetting({ + "Security": { + "advanced_authkeys": True, + "advanced_authkeys_validity": 365, + } + }): + expiration = int((datetime.datetime.now() + datetime.timedelta(days=300)).timestamp()) + auth_key = self.__create_advanced_authkey(self.test_usr.id, {"expiration": expiration}) + self.__delete_advanced_authkey(auth_key["id"]) + self.assertEqual(expiration, int(auth_key["expiration"])) + + def test_advanced_authkeys_validity_not_in_range(self): + with MISPComplexSetting({ + "Security": { + "advanced_authkeys": True, + "advanced_authkeys_validity": 365, + } + }): + expiration = int((datetime.datetime.now() + datetime.timedelta(days=400)).timestamp()) + with self.assertRaises(Exception) as cm: + self.__create_advanced_authkey(self.test_usr.id, {"expiration": expiration}) + self.assertIn("expiration", cm.exception.args[0][1]["errors"]) + def test_advanced_authkeys_view(self): with self.__setting("Security.advanced_authkeys", True): auth_key = self.__create_advanced_authkey(self.test_usr.id) From c06782226a68c0559f7188c1fb7f80edd64f4182 Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Thu, 3 Dec 2020 17:24:26 +0100 Subject: [PATCH 23/42] fix: [security] Auth key must be always random generated at server side --- app/Controller/AuthKeysController.php | 7 ++++--- app/Controller/Component/CRUDComponent.php | 2 -- tests/testlive_security.py | 7 +++++++ 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/app/Controller/AuthKeysController.php b/app/Controller/AuthKeysController.php index a7e6de56e..8750a3b11 100644 --- a/app/Controller/AuthKeysController.php +++ b/app/Controller/AuthKeysController.php @@ -71,15 +71,16 @@ class AuthKeysController extends AppController { $params = [ 'displayOnSuccess' => 'authkey_display', - 'saveModelVariable' => ['authkey_raw'] + 'saveModelVariable' => ['authkey_raw'], + 'override' => ['authkey' => null], // do not allow to use own key, always generate random one ]; $selectConditions = []; if (!$this->_isSiteAdmin()) { $selectConditions['AND'][] = ['User.id' => $this->Auth->user('id')]; - $params['override'] = ['user_id' => $this->Auth->user('id')]; + $params['override']['user_id'] = $this->Auth->user('id'); } else if ($user_id) { $selectConditions['AND'][] = ['User.id' => $user_id]; - $params['override'] = ['user_id' => $user_id]; + $params['override']['user_id'] = $user_id; } $this->CRUD->add($params); if ($this->IndexFilter->isRest()) { diff --git a/app/Controller/Component/CRUDComponent.php b/app/Controller/Component/CRUDComponent.php index c15399ac7..f19f77ec0 100644 --- a/app/Controller/Component/CRUDComponent.php +++ b/app/Controller/Component/CRUDComponent.php @@ -75,8 +75,6 @@ class CRUDComponent extends Component $input[$modelName][$field] = $value; } } - if (isset($input[$modelName]['id'])) { - } unset($input[$modelName]['id']); if (!empty($params['fields'])) { $data = []; diff --git a/tests/testlive_security.py b/tests/testlive_security.py index e2a055dd9..30731d1c7 100644 --- a/tests/testlive_security.py +++ b/tests/testlive_security.py @@ -375,6 +375,13 @@ class TestSecurity(unittest.TestCase): self.__delete_advanced_authkey(auth_key["id"]) + def test_advanced_authkeys_own_key_not_possible(self): + with MISPSetting(self.admin_misp_connector, "Security.advanced_authkeys", True): + authkey = ("a" * 40) + auth_key = self.__create_advanced_authkey(self.test_usr.id, {"authkey": authkey}) + self.__delete_advanced_authkey(auth_key["id"]) + self.assertNotEqual(authkey, auth_key["authkey"]) + def test_advanced_authkeys_reset_own(self): with self.__setting("Security.advanced_authkeys", True): # Create advanced authkey From 640e9492d7b30b93c2ec4405a770a70df08ba68e Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Thu, 3 Dec 2020 18:24:37 +0100 Subject: [PATCH 24/42] new: [security] Put information about key expiration into response header --- app/Controller/AppController.php | 31 ++++++++++++++++++++++++++----- app/Model/AuthKey.php | 3 ++- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/app/Controller/AppController.php b/app/Controller/AppController.php index 4b0db6665..5e46d7493 100755 --- a/app/Controller/AppController.php +++ b/app/Controller/AppController.php @@ -263,6 +263,13 @@ class AppController extends Controller throw new ForbiddenException("When user is authenticated by authkey, just REST request can be processed"); } + // Put token expiration time to response header that can be processed by automation tool + if (isset($user['authkey_expiration']) && $user['authkey_expiration']) { + $expiration = date('c', $user['authkey_expiration']); + $this->response->header('X-Auth-Key-Expiration', $expiration); + $this->RestResponse->setHeader('X-Auth-Key-Expiration', $expiration); + } + $this->set('default_memory_limit', ini_get('memory_limit')); if (isset($user['Role']['memory_limit']) && $user['Role']['memory_limit'] !== '') { ini_set('memory_limit', $user['Role']['memory_limit']); @@ -545,6 +552,15 @@ class AppController extends Controller return false; } + // Check if auth key is not expired. Make sense when Security.authkey_keep_session is enabled. + if (isset($user['authkey_expiration']) && $user['authkey_expiration']) { + $time = isset($_SERVER['REQUEST_TIME']) ? $_SERVER['REQUEST_TIME'] : time(); + if ($user['authkey_expiration'] < $time) { + $this->Auth->logout(); + throw new ForbiddenException('Auth key is expired'); + } + } + $isUserRequest = !$this->_isRest() && !$this->request->is('ajax'); // Next checks makes sense just for user direct HTTP request, so skip REST and AJAX calls if (!$isUserRequest) { @@ -1394,15 +1410,20 @@ class AppController extends Controller } /** - * Refresh user data in session. + * Refresh user data in session, but keep information about authkey. * @return array User data in auth format */ protected function _refreshAuth() { - $userId = $this->Auth->user('id'); - $user = $this->User->getAuthUser($userId); - if (!$user){ - throw new RuntimeException("User with ID $userId not exists."); + $sessionUser = $this->Auth->user(); + $user = $this->User->getAuthUser($sessionUser['id']); + if (!$user) { + throw new RuntimeException("User with ID {$sessionUser['id']} not exists."); + } + foreach (['authkey_id', 'authkey_expiration', 'logged_by_authkey'] as $copy) { + if (isset($sessionUser[$copy])) { + $user[$copy] = $sessionUser[$copy]; + } } $this->Auth->login($user); return $user; diff --git a/app/Model/AuthKey.php b/app/Model/AuthKey.php index 17b702669..390973f85 100644 --- a/app/Model/AuthKey.php +++ b/app/Model/AuthKey.php @@ -73,7 +73,7 @@ class AuthKey extends AppModel $end = substr($authkey, -4); $existing_authkeys = $this->find('all', [ 'recursive' => -1, - 'fields' => ['id', 'authkey', 'user_id'], + 'fields' => ['id', 'authkey', 'user_id', 'expiration'], 'conditions' => [ 'OR' => [ 'expiration >' => time(), @@ -89,6 +89,7 @@ class AuthKey extends AppModel $user = $this->User->getAuthUser($existing_authkey['AuthKey']['user_id']); if ($user) { $user['authkey_id'] = $existing_authkey['AuthKey']['id']; + $user['authkey_expiration'] = $existing_authkey['AuthKey']['expiration']; } return $user; } From c0f6463d57ab41b346804235088e1752b623cad3 Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Thu, 3 Dec 2020 18:33:19 +0100 Subject: [PATCH 25/42] new: [security] Cancel API session right after auth key is deleted --- app/Controller/AppController.php | 6 ++++++ app/Model/AuthKey.php | 12 ++++++++++++ tests/testlive_security.py | 30 ++++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+) diff --git a/app/Controller/AppController.php b/app/Controller/AppController.php index 5e46d7493..9c96b6b32 100755 --- a/app/Controller/AppController.php +++ b/app/Controller/AppController.php @@ -1420,6 +1420,12 @@ class AppController extends Controller if (!$user) { throw new RuntimeException("User with ID {$sessionUser['id']} not exists."); } + if (isset($sessionUser['authkey_id'])) { + $this->loadModel('AuthKey'); + if (!$this->AuthKey->exists($sessionUser['authkey_id'])) { + throw new RuntimeException("Auth key with ID {$sessionUser['authkey_id']} not exists."); + } + } foreach (['authkey_id', 'authkey_expiration', 'logged_by_authkey'] as $copy) { if (isset($sessionUser[$copy])) { $user[$copy] = $sessionUser[$copy]; diff --git a/app/Model/AuthKey.php b/app/Model/AuthKey.php index 390973f85..06b115261 100644 --- a/app/Model/AuthKey.php +++ b/app/Model/AuthKey.php @@ -154,6 +154,18 @@ class AuthKey extends AppModel return [$output, $lastUsage]; } + /** + * When key is deleted, update after `date_modified` for user that was assigned to that key, so session data + * will be realoaded and canceled. + * @see AppController::_refreshAuth + */ + public function afterDelete() + { + parent::afterDelete(); + $userId = $this->data['AuthKey']['user_id']; + $this->User->updateAll(['date_modified' => time()], ['User.id' => $userId]); + } + /** * @return AbstractPasswordHasher */ diff --git a/tests/testlive_security.py b/tests/testlive_security.py index 30731d1c7..30dca3713 100644 --- a/tests/testlive_security.py +++ b/tests/testlive_security.py @@ -1,6 +1,7 @@ #!/usr/bin/env python3 import os import sys +import time import json import datetime import unittest @@ -375,6 +376,35 @@ class TestSecurity(unittest.TestCase): self.__delete_advanced_authkey(auth_key["id"]) + def test_advanced_authkeys_deleted(self): + with MISPSetting(self.admin_misp_connector, "Security.advanced_authkeys", True): + auth_key = self.__create_advanced_authkey(self.test_usr.id) + + logged_in = PyMISP(url, auth_key["authkey_raw"]) + self.assertEqual(logged_in._current_user.id, self.test_usr.id) + + self.__delete_advanced_authkey(auth_key["id"]) + + assert_error_response(logged_in.get_user()) + + def test_advanced_authkeys_deleted_keep_session(self): + with MISPComplexSetting({ + "Security": { + "advanced_authkeys": True, + "authkey_keep_session": True, + } + }): + auth_key = self.__create_advanced_authkey(self.test_usr.id) + + logged_in = PyMISP(url, auth_key["authkey_raw"]) + self.assertEqual(logged_in._current_user.id, self.test_usr.id) + + self.__delete_advanced_authkey(auth_key["id"]) + # Wait one second to really know that session will be reloaded + time.sleep(1) + with self.assertRaises(MISPServerError): + logged_in.get_user() + def test_advanced_authkeys_own_key_not_possible(self): with MISPSetting(self.admin_misp_connector, "Security.advanced_authkeys", True): authkey = ("a" * 40) From 197b1a341aa8f19a98645d0240757978a828cc1d Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Thu, 3 Dec 2020 20:44:39 +0100 Subject: [PATCH 26/42] chg: [internal] Code cleanup --- app/Controller/AppController.php | 33 +++++--------------------------- 1 file changed, 5 insertions(+), 28 deletions(-) diff --git a/app/Controller/AppController.php b/app/Controller/AppController.php index 9c96b6b32..89999f838 100755 --- a/app/Controller/AppController.php +++ b/app/Controller/AppController.php @@ -1,27 +1,4 @@ _isRest() || $this->_isAutomation()) { // disable CSRF for REST access - if (array_key_exists('Security', $this->components)) { + if (isset($this->components['Security'])) { $this->Security->csrfCheck = false; } if ($this->__loginByAuthKey() === false || $this->Auth->user() === null) { @@ -240,7 +219,7 @@ class AppController extends Controller Configure::write('CurrentUserId', $user['id']); $this->__logAccess($user); - // If user is site admin, try to run updates + // Try to run updates if ($user['Role']['perm_site_admin'] || (Configure::read('MISP.live') && !$this->_isRest())) { $this->User->runUpdates(); } @@ -561,7 +540,7 @@ class AppController extends Controller } } - $isUserRequest = !$this->_isRest() && !$this->request->is('ajax'); + $isUserRequest = !$this->_isRest() && !$this->request->is('ajax') && !$this->_isAutomation(); // Next checks makes sense just for user direct HTTP request, so skip REST and AJAX calls if (!$isUserRequest) { return true; @@ -793,8 +772,6 @@ class AppController extends Controller throw new BadRequestException('The request has been black-holed'); } - public $userRole = null; - protected function _isRest() { return $this->IndexFilter->isRest(); From 8a045673c77de4a3fff38d0175f039902d1dd297 Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Thu, 3 Dec 2020 21:02:47 +0100 Subject: [PATCH 27/42] new: [UI] Show information about key expiration in server list --- app/Model/Server.php | 20 ++++++++++++-------- app/webroot/js/misp.js | 8 +++----- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/app/Model/Server.php b/app/Model/Server.php index 357d29c55..5e0f8046c 100644 --- a/app/Model/Server.php +++ b/app/Model/Server.php @@ -6636,7 +6636,7 @@ class Server extends AppModel } catch (Exception $e) { $this->Log = ClassRegistry::init('Log'); $this->Log->create(); - $message = __('Could not reset fetch remote user account.'); + $message = __('Could not fetch remote user account.'); $this->Log->save(array( 'org' => 'SYSTEM', 'model' => 'Server', @@ -6649,14 +6649,18 @@ class Server extends AppModel return $message; } if ($response->isOk()) { - $user = json_decode($response->body, true); + $user = $this->jsonDecode($response->body); if (!empty($user['User'])) { - $result = array( - 'Email' => $user['User']['email'], - 'Role name' => isset($user['Role']['name']) ? $user['Role']['name'] : 'Unknown, outdated instance', - 'Sync flag' => isset($user['Role']['perm_sync']) ? ($user['Role']['perm_sync'] ? 1 : 0) : 'Unknown, outdated instance' - ); - return $result; + $results = [ + __('User') => $user['User']['email'], + __('Role name') => isset($user['Role']['name']) ? $user['Role']['name'] : __('Unknown, outdated instance'), + __('Sync flag') => isset($user['Role']['perm_sync']) ? ($user['Role']['perm_sync'] ? __('Yes') : __('No')) : __('Unknown, outdated instance'), + ]; + if (isset($response->headers['X-Auth-Key-Expiration'])) { + $date = new DateTime($response->headers['X-Auth-Key-Expiration']); + $results[__('Auth key expiration')] = $date->format('Y-m-d H:i:s'); + } + return $results; } else { return __('No user object received in response.'); } diff --git a/app/webroot/js/misp.js b/app/webroot/js/misp.js index f5f74233e..6d36177ba 100644 --- a/app/webroot/js/misp.js +++ b/app/webroot/js/misp.js @@ -3371,15 +3371,15 @@ function getRemoteSyncUser(id) { $.ajax({ url: baseurl + '/servers/getRemoteUser/' + id, type:'GET', - beforeSend: function (XMLHttpRequest) { + beforeSend: function () { resultContainer.html('Running test...'); }, - error: function(){ + error: function() { resultContainer.html('Internal error.'); }, success: function(response) { + resultContainer.empty(); if (typeof(response.message) != 'undefined') { - resultContainer.empty(); resultContainer.append( $('') .attr('class', 'red bold') @@ -3389,7 +3389,6 @@ function getRemoteSyncUser(id) { .text(': #' + response.message) ); } else { - resultContainer.empty(); Object.keys(response).forEach(function(key) { var value = response[key]; resultContainer.append( @@ -3404,7 +3403,6 @@ function getRemoteSyncUser(id) { ); }); } - var result = response; } }); } From 5bc7037c45ec83b7530281d552e9e87337ef8f0f Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Fri, 4 Dec 2020 09:23:12 +0100 Subject: [PATCH 28/42] fix: [internal] Check if setting value is scalar --- app/Controller/ServersController.php | 4 ++-- tests/testlive_security.py | 13 +++++++++++-- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/app/Controller/ServersController.php b/app/Controller/ServersController.php index d82fda6a9..4596f1db5 100644 --- a/app/Controller/ServersController.php +++ b/app/Controller/ServersController.php @@ -1448,7 +1448,7 @@ class ServersController extends AppController if (!isset($this->request->data['Server'])) { $this->request->data = array('Server' => $this->request->data); } - if (!isset($this->request->data['Server']['value'])) { + if (!isset($this->request->data['Server']['value']) || !is_scalar($this->request->data['Server']['value'])) { if ($this->_isRest()) { return $this->RestResponse->saveFailResponse('Servers', 'serverSettingsEdit', false, 'Invalid input. Expected: {"value": "new_setting"}', $this->response->type()); } @@ -1491,7 +1491,7 @@ class ServersController extends AppController return new CakeResponse(array('body'=> json_encode(array('saved' => true, 'success' => 'Field updated.')), 'status'=>200, 'type' => 'json')); } } else { - if ($this->_isRest) { + if ($this->_isRest()) { return $this->RestResponse->saveFailResponse('Servers', 'serverSettingsEdit', false, $result, $this->response->type()); } else { return new CakeResponse(array('body'=> json_encode(array('saved' => false, 'errors' => $result)), 'status'=>200, 'type' => 'json')); diff --git a/tests/testlive_security.py b/tests/testlive_security.py index 30dca3713..48b70e8bd 100644 --- a/tests/testlive_security.py +++ b/tests/testlive_security.py @@ -184,6 +184,11 @@ class TestSecurity(unittest.TestCase): # TODO: Try to reload config cache self.admin_misp_connector.get_server_setting("MISP.live") + def tearDown(self): + # Ensure correct config + setting = self.admin_misp_connector.get_server_setting("Security.advanced_authkeys") + self.assertEqual(setting["value"], False, "Security.advanced_authkeys should be False after test") + def test_not_logged_in(self): session = requests.Session() @@ -399,18 +404,22 @@ class TestSecurity(unittest.TestCase): logged_in = PyMISP(url, auth_key["authkey_raw"]) self.assertEqual(logged_in._current_user.id, self.test_usr.id) - self.__delete_advanced_authkey(auth_key["id"]) # Wait one second to really know that session will be reloaded time.sleep(1) + + self.__delete_advanced_authkey(auth_key["id"]) + with self.assertRaises(MISPServerError): logged_in.get_user() + time.sleep(1) + def test_advanced_authkeys_own_key_not_possible(self): with MISPSetting(self.admin_misp_connector, "Security.advanced_authkeys", True): authkey = ("a" * 40) auth_key = self.__create_advanced_authkey(self.test_usr.id, {"authkey": authkey}) self.__delete_advanced_authkey(auth_key["id"]) - self.assertNotEqual(authkey, auth_key["authkey"]) + self.assertNotEqual(authkey, auth_key["authkey_raw"]) def test_advanced_authkeys_reset_own(self): with self.__setting("Security.advanced_authkeys", True): From 790087ca60b03307c8ded0f81b90631b2514f5f6 Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Fri, 4 Dec 2020 15:59:26 +0100 Subject: [PATCH 29/42] fix: [security] Do not return hashed authentication key after creation --- app/Controller/AuthKeysController.php | 4 ++++ app/Controller/Component/CRUDComponent.php | 3 +++ tests/testlive_security.py | 1 + 3 files changed, 8 insertions(+) diff --git a/app/Controller/AuthKeysController.php b/app/Controller/AuthKeysController.php index 8750a3b11..c0cff4df6 100644 --- a/app/Controller/AuthKeysController.php +++ b/app/Controller/AuthKeysController.php @@ -73,6 +73,10 @@ class AuthKeysController extends AppController 'displayOnSuccess' => 'authkey_display', 'saveModelVariable' => ['authkey_raw'], 'override' => ['authkey' => null], // do not allow to use own key, always generate random one + 'afterFind' => function ($authKey) { // remove hashed key from response + unset($authKey['AuthKey']['authkey']); + return $authKey; + } ]; $selectConditions = []; if (!$this->_isSiteAdmin()) { diff --git a/app/Controller/Component/CRUDComponent.php b/app/Controller/Component/CRUDComponent.php index f19f77ec0..4228fa5b8 100644 --- a/app/Controller/Component/CRUDComponent.php +++ b/app/Controller/Component/CRUDComponent.php @@ -100,6 +100,9 @@ class CRUDComponent extends Component } } } + if (isset($params['afterFind'])) { + $data = $params['afterFind']($data); + } $message = __('%s added.', $modelName); if ($this->Controller->IndexFilter->isRest()) { $this->Controller->restResponsePayload = $this->Controller->RestResponse->viewData($data, 'json'); diff --git a/tests/testlive_security.py b/tests/testlive_security.py index 48b70e8bd..01a7a33cb 100644 --- a/tests/testlive_security.py +++ b/tests/testlive_security.py @@ -349,6 +349,7 @@ class TestSecurity(unittest.TestCase): with self.__setting("Security.advanced_authkeys", True): # Create advanced authkey auth_key = self.__create_advanced_authkey(self.test_usr.id) + self.assertNotIn("authkey", auth_key) # Try to login logged_in = PyMISP(url, auth_key["authkey_raw"]) From 2b30bab9b0d36b11d87de75cbfe3dba5ba75ae9b Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Fri, 4 Dec 2020 17:13:55 +0100 Subject: [PATCH 30/42] new: [UI] Show last key usage in index table --- app/Controller/AuthKeysController.php | 13 +++++++++---- app/Model/AuthKey.php | 19 +++++++++++++++++++ app/View/AuthKeys/index.ctp | 6 ++++++ 3 files changed, 34 insertions(+), 4 deletions(-) diff --git a/app/Controller/AuthKeysController.php b/app/Controller/AuthKeysController.php index c0cff4df6..8883d2010 100644 --- a/app/Controller/AuthKeysController.php +++ b/app/Controller/AuthKeysController.php @@ -26,17 +26,21 @@ class AuthKeysController extends AppController $this->set('user_id', $id); $conditions['AND'][] = ['AuthKey.user_id' => $id]; } + $keyUsageEnabled = Configure::read('MISP.log_user_ips') && Configure::read('MISP.log_user_ips_authkeys'); $this->CRUD->index([ 'filters' => ['User.username', 'authkey', 'comment', 'User.id'], 'quickFilters' => ['authkey', 'comment'], 'contain' => ['User.id', 'User.email'], 'conditions' => $conditions, - 'afterFind' => function (array $authKeys) { - $keyUsageEnabled = Configure::read('MISP.log_user_ips') && Configure::read('MISP.log_user_ips_authkeys'); + 'afterFind' => function (array $authKeys) use ($keyUsageEnabled) { + if ($keyUsageEnabled) { + $keyIds = Hash::extract($authKeys, "{n}.AuthKey.id"); + $lastUsedById = $this->AuthKey->getLastUsageForKeys($keyIds); + } foreach ($authKeys as &$authKey) { if ($keyUsageEnabled) { - $lastUsed = $this->AuthKey->getKeyUsage($authKey['AuthKey']['id'])[1]; - $key['AuthKey']['last_used'] = $lastUsed ? $lastUsed->format('c') : null; + $lastUsed = $lastUsedById[$authKey['AuthKey']['id']]; + $authKey['AuthKey']['last_used'] = $lastUsed; } unset($authKey['AuthKey']['authkey']); } @@ -46,6 +50,7 @@ class AuthKeysController extends AppController if ($this->IndexFilter->isRest()) { return $this->restResponsePayload; } + $this->set('keyUsageEnabled', $keyUsageEnabled); $this->set('metaGroup', $this->_isAdmin ? 'admin' : 'globalActions'); $this->set('metaAction', 'authkeys_index'); } diff --git a/app/Model/AuthKey.php b/app/Model/AuthKey.php index 06b115261..6c276cd49 100644 --- a/app/Model/AuthKey.php +++ b/app/Model/AuthKey.php @@ -154,6 +154,25 @@ class AuthKey extends AppModel return [$output, $lastUsage]; } + /** + * @param array $ids + * @return array + * @throws Exception + */ + public function getLastUsageForKeys(array $ids) + { + $redis = $this->setupRedisWithException(); + $keys = array_map(function($id) { + return "misp:authkey_last_usage:$id"; + }, $ids); + $lastUsages = $redis->mget($keys); + $output = []; + foreach (array_values($ids) as $i => $id) { + $output[$id] = $lastUsages[$i] === false ? null : (int)$lastUsages[$i]; + } + return $output; + } + /** * When key is deleted, update after `date_modified` for user that was assigned to that key, so session data * will be realoaded and canceled. diff --git a/app/View/AuthKeys/index.ctp b/app/View/AuthKeys/index.ctp index 9a04d3a7c..7c131eb32 100644 --- a/app/View/AuthKeys/index.ctp +++ b/app/View/AuthKeys/index.ctp @@ -56,6 +56,12 @@ 'data_path' => 'AuthKey.expiration', 'element' => 'expiration' ], + [ + 'name' => ('Last used'), + 'data_path' => 'AuthKey.last_used', + 'element' => 'datetime', + 'requirements' => $keyUsageEnabled, + ], [ 'name' => __('Comment'), 'sort' => 'AuthKey.comment', From 35e470eb4d9e54b01517dc6d652c0b5a46ece4f9 Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Fri, 4 Dec 2020 17:38:46 +0100 Subject: [PATCH 31/42] new: [UI] Show number of unique IPs for key usage --- app/Controller/AuthKeysController.php | 3 ++- app/Model/AuthKey.php | 6 ++++-- app/View/AuthKeys/view.ctp | 7 ++++++- .../genericElements/SingleViews/Fields/genericField.ctp | 2 +- 4 files changed, 13 insertions(+), 5 deletions(-) diff --git a/app/Controller/AuthKeysController.php b/app/Controller/AuthKeysController.php index 8883d2010..26c99b88c 100644 --- a/app/Controller/AuthKeysController.php +++ b/app/Controller/AuthKeysController.php @@ -125,9 +125,10 @@ class AuthKeysController extends AppController } if (Configure::read('MISP.log_user_ips') && Configure::read('MISP.log_user_ips_authkeys')) { - list($keyUsage, $lastUsed) = $this->AuthKey->getKeyUsage($id); + list($keyUsage, $lastUsed, $uniqueIps) = $this->AuthKey->getKeyUsage($id); $this->set('keyUsage', $keyUsage); $this->set('lastUsed', $lastUsed); + $this->set('uniqueIps', $uniqueIps); } $this->set('menuData', [ diff --git a/app/Model/AuthKey.php b/app/Model/AuthKey.php index 6c276cd49..f4711b6b7 100644 --- a/app/Model/AuthKey.php +++ b/app/Model/AuthKey.php @@ -137,8 +137,10 @@ class AuthKey extends AppModel $data = $redis->hGetAll("misp:authkey_usage:$id"); $output = []; + $uniqueIps = []; foreach ($data as $key => $count) { list($date, $ip) = explode(':', $key); + $uniqueIps[$ip] = true; if (isset($output[$date])) { $output[$date] += $count; } else { @@ -149,9 +151,9 @@ class AuthKey extends AppModel ksort($output); $lastUsage = $redis->get("misp:authkey_last_usage:$id"); - $lastUsage = $lastUsage === false ? null : new DateTime("@$lastUsage"); + $lastUsage = $lastUsage === false ? null : (int)$lastUsage; - return [$output, $lastUsage]; + return [$output, $lastUsage, count($uniqueIps)]; } /** diff --git a/app/View/AuthKeys/view.ctp b/app/View/AuthKeys/view.ctp index cbc4c9a75..17aa4263a 100644 --- a/app/View/AuthKeys/view.ctp +++ b/app/View/AuthKeys/view.ctp @@ -63,7 +63,12 @@ echo $this->element( ], [ 'key' => __('Last used'), - 'raw' => $lastUsed ? $lastUsed->format('Y-m-d H:i:s') : __('Not used yet'), + 'raw' => $lastUsed ? date('Y-m-d H:i:s', $lastUsed) : __('Not used yet'), + 'requirement' => isset($keyUsage), + ], + [ + 'key' => __('Unique IPs'), + 'raw' => $uniqueIps, 'requirement' => isset($keyUsage), ] ], diff --git a/app/View/Elements/genericElements/SingleViews/Fields/genericField.ctp b/app/View/Elements/genericElements/SingleViews/Fields/genericField.ctp index d61e6ee84..17cd2cf78 100644 --- a/app/View/Elements/genericElements/SingleViews/Fields/genericField.ctp +++ b/app/View/Elements/genericElements/SingleViews/Fields/genericField.ctp @@ -1,5 +1,5 @@ Date: Fri, 4 Dec 2020 18:13:05 +0100 Subject: [PATCH 32/42] fix: [UI] Days to expire count --- .../Elements/genericElements/IndexTable/Fields/expiration.ctp | 4 ++-- .../genericElements/SingleViews/Fields/expirationField.ctp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/View/Elements/genericElements/IndexTable/Fields/expiration.ctp b/app/View/Elements/genericElements/IndexTable/Fields/expiration.ctp index 34b47d134..09fbdf8bb 100644 --- a/app/View/Elements/genericElements/IndexTable/Fields/expiration.ctp +++ b/app/View/Elements/genericElements/IndexTable/Fields/expiration.ctp @@ -20,9 +20,9 @@ $title = __('Expired at %s', date('Y-m-d H:i:s', $data)); $data = '' . __('Expired') . ''; } else { - $diffInDays = floor(($data - time()) / 3600 * 24); + $diffInDays = floor(($data - time()) / (3600 * 24)); $class = $diffInDays <= 14 ? 'text-warning bold' : 'text-success'; - $title = __n('Will expire at %s day', 'Will expire at %s days', $diffInDays, $diffInDays); + $title = __n('Will expire in %s day', 'Will expire in %s days', $diffInDays, $diffInDays); $data = '' . date('Y-m-d H:i:s', $data) . ''; } } diff --git a/app/View/Elements/genericElements/SingleViews/Fields/expirationField.ctp b/app/View/Elements/genericElements/SingleViews/Fields/expirationField.ctp index 61b544519..4dbfe5f9b 100644 --- a/app/View/Elements/genericElements/SingleViews/Fields/expirationField.ctp +++ b/app/View/Elements/genericElements/SingleViews/Fields/expirationField.ctp @@ -20,9 +20,9 @@ $title = __('Expired at %s', date('Y-m-d H:i:s', $data)); $data = '' . __('Expired') . ''; } else { - $diffInDays = floor(($data - time()) / 3600 * 24); + $diffInDays = floor(($data - time()) / (3600 * 24)); $class = $diffInDays <= 14 ? 'text-warning bold' : 'text-success'; - $title = __n('Will expire at %s day', 'Will expire at %s days', $diffInDays, $diffInDays); + $title = __n('Will expire in %s day', 'Will expire in %s days', $diffInDays, $diffInDays); $data = '' . date('Y-m-d H:i:s', $data) . ''; } } From 99f8e94ecb532275dde1b68faa60d4a35e79c8a5 Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Mon, 7 Dec 2020 10:07:54 +0100 Subject: [PATCH 33/42] chg: [test] Update testlive_security.py to new version --- tests/testlive_security.py | 47 ++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 27 deletions(-) diff --git a/tests/testlive_security.py b/tests/testlive_security.py index 01a7a33cb..ec8044f14 100644 --- a/tests/testlive_security.py +++ b/tests/testlive_security.py @@ -168,7 +168,7 @@ class TestSecurity(unittest.TestCase): # Try to connect as user to check if everything works PyMISP(url, cls.test_usr.authkey) # Check if user can login with given password - assert login(url, cls.test_usr.email, cls.test_usr_password) is not False + assert isinstance(login(url, cls.test_usr.email, cls.test_usr_password), requests.Session) @classmethod def tearDownClass(cls): @@ -181,13 +181,6 @@ class TestSecurity(unittest.TestCase): def setUp(self): # Do not show warning about not closed resources, because that something we want warnings.simplefilter("ignore", ResourceWarning) - # TODO: Try to reload config cache - self.admin_misp_connector.get_server_setting("MISP.live") - - def tearDown(self): - # Ensure correct config - setting = self.admin_misp_connector.get_server_setting("Security.advanced_authkeys") - self.assertEqual(setting["value"], False, "Security.advanced_authkeys should be False after test") def test_not_logged_in(self): session = requests.Session() @@ -383,7 +376,7 @@ class TestSecurity(unittest.TestCase): self.__delete_advanced_authkey(auth_key["id"]) def test_advanced_authkeys_deleted(self): - with MISPSetting(self.admin_misp_connector, "Security.advanced_authkeys", True): + with self.__setting("Security.advanced_authkeys", True): auth_key = self.__create_advanced_authkey(self.test_usr.id) logged_in = PyMISP(url, auth_key["authkey_raw"]) @@ -391,10 +384,10 @@ class TestSecurity(unittest.TestCase): self.__delete_advanced_authkey(auth_key["id"]) - assert_error_response(logged_in.get_user()) + self.__assertErrorResponse(logged_in.get_user()) def test_advanced_authkeys_deleted_keep_session(self): - with MISPComplexSetting({ + with self.__setting({ "Security": { "advanced_authkeys": True, "authkey_keep_session": True, @@ -416,7 +409,7 @@ class TestSecurity(unittest.TestCase): time.sleep(1) def test_advanced_authkeys_own_key_not_possible(self): - with MISPSetting(self.admin_misp_connector, "Security.advanced_authkeys", True): + with self.__setting("Security.advanced_authkeys", True): authkey = ("a" * 40) auth_key = self.__create_advanced_authkey(self.test_usr.id, {"authkey": authkey}) self.__delete_advanced_authkey(auth_key["id"]) @@ -490,13 +483,13 @@ class TestSecurity(unittest.TestCase): # TODO: Delete new key def test_advanced_authkeys_expiration_invalid(self): - with MISPSetting(self.admin_misp_connector, "Security.advanced_authkeys", True): + with self.__setting("Security.advanced_authkeys", True): with self.assertRaises(Exception) as cm: self.__create_advanced_authkey(self.test_usr.id, {"expiration": "__nonsense__"}) self.assertIn("expiration", cm.exception.args[0][1]["errors"]) def test_advanced_authkeys_validity_autoset(self): - with MISPComplexSetting({ + with self.__setting({ "Security": { "advanced_authkeys": True, "advanced_authkeys_validity": 365, @@ -506,7 +499,7 @@ class TestSecurity(unittest.TestCase): self.assertNotEqual(0, auth_key["expiration"]) def test_advanced_authkeys_validity_in_range(self): - with MISPComplexSetting({ + with self.__setting({ "Security": { "advanced_authkeys": True, "advanced_authkeys_validity": 365, @@ -518,7 +511,7 @@ class TestSecurity(unittest.TestCase): self.assertEqual(expiration, int(auth_key["expiration"])) def test_advanced_authkeys_validity_not_in_range(self): - with MISPComplexSetting({ + with self.__setting({ "Security": { "advanced_authkeys": True, "advanced_authkeys_validity": 365, @@ -570,7 +563,7 @@ class TestSecurity(unittest.TestCase): self.__delete_advanced_authkey(auth_key["id"]) def test_authkey_keep_session(self): - with MISPSetting(self.admin_misp_connector, "Security.authkey_keep_session", True): + with self.__setting( "Security.authkey_keep_session", True): logged_in = PyMISP(url, self.test_usr.authkey) check_response(logged_in.get_user()) check_response(logged_in.get_user()) @@ -916,7 +909,7 @@ class TestSecurity(unittest.TestCase): def test_user_monitoring_enabled_no_user(self): request_logs_before = self.__get_logs(action="request") - with MISPSetting(self.admin_misp_connector, "Security.user_monitoring_enabled", True): + with self.__setting("Security.user_monitoring_enabled", True): logged_in = PyMISP(url, self.test_usr.authkey) check_response(logged_in.get_user()) @@ -927,7 +920,7 @@ class TestSecurity(unittest.TestCase): def test_user_monitoring_enabled_add_user(self): request_logs_before = self.__get_logs(action="request") - with MISPSetting(self.admin_misp_connector, "Security.user_monitoring_enabled", True): + with self.__setting("Security.user_monitoring_enabled", True): # Enable monitoring of test user send(self.admin_misp_connector, "POST", f"/admin/users/monitor/{self.test_usr.id}", { "value": 1, @@ -947,7 +940,7 @@ class TestSecurity(unittest.TestCase): def test_log_paranoid(self): request_logs_before = self.__get_logs(action="request") - with MISPSetting(self.admin_misp_connector, "MISP.log_paranoid", True): + with self.__setting("MISP.log_paranoid", True): logged_in = PyMISP(url, self.test_usr.authkey) check_response(logged_in.get_user()) @@ -957,7 +950,7 @@ class TestSecurity(unittest.TestCase): def test_log_paranoid_include_post_body(self): request_logs_before = self.__get_logs(action="request") - with MISPComplexSetting({ + with self.__setting({ "MISP": { "log_paranoid": True, "log_paranoid_include_post_body": True, @@ -972,7 +965,7 @@ class TestSecurity(unittest.TestCase): def test_log_paranoid_skip_db(self): request_logs_before = self.__get_logs(action="request") - with MISPComplexSetting({ + with self.__setting({ "MISP": { "log_paranoid": True, "log_paranoid_skip_db": True, @@ -998,12 +991,12 @@ class TestSecurity(unittest.TestCase): self.assertEqual(len(request_logs_after), len(request_logs_before) + 1) def test_log_user_ips(self): - with MISPSetting(self.admin_misp_connector, "MISP.log_user_ips", True): + with self.__setting("MISP.log_user_ips", True): logged_in = PyMISP(url, self.test_usr.authkey) check_response(logged_in.get_user()) def test_log_user_ips_auth(self): - with MISPComplexSetting({ + with self.__setting({ "MISP": { "log_user_ips": True, "log_user_ips_authkeys": True, @@ -1013,7 +1006,7 @@ class TestSecurity(unittest.TestCase): check_response(logged_in.get_user()) def test_username_in_response_header(self): - with MISPSetting(self.admin_misp_connector, "Security.username_in_response_header", True): + with self.__setting("Security.username_in_response_header", True): logged_in = login(url, self.test_usr.email, self.test_usr_password) self.assertIsInstance(logged_in, requests.Session) @@ -1022,7 +1015,7 @@ class TestSecurity(unittest.TestCase): self.assertEqual(self.test_usr.email, response.headers["X-Username"]) def test_username_in_response_header_api_access(self): - with MISPSetting(self.admin_misp_connector, "Security.username_in_response_header", True): + with self.__setting("Security.username_in_response_header", True): logged_in = PyMISP(url, self.test_usr.authkey) response = logged_in._prepare_request('GET', 'users/view/me') @@ -1030,7 +1023,7 @@ class TestSecurity(unittest.TestCase): self.assertEqual(self.test_usr.email + "/API/default", response.headers["X-Username"]) def test_username_in_response_header_advanced_api_access(self): - with MISPComplexSetting({ + with self.__setting({ "Security": { "advanced_authkeys": True, "username_in_response_header": True, From e9e47b0a86feace070b9c3125ec7f040288c7984 Mon Sep 17 00:00:00 2001 From: Jakub Onderka Date: Mon, 7 Dec 2020 11:10:27 +0100 Subject: [PATCH 34/42] fix: [UI] Auth Key index and view changes and fixes --- app/Controller/AuthKeysController.php | 8 ++++++-- app/View/AuthKeys/index.ctp | 15 ++++++--------- app/View/AuthKeys/view.ctp | 22 +++++++++++----------- app/View/genericTemplates/delete.ctp | 9 ++++++--- 4 files changed, 29 insertions(+), 25 deletions(-) diff --git a/app/Controller/AuthKeysController.php b/app/Controller/AuthKeysController.php index 26c99b88c..b012ae4ac 100644 --- a/app/Controller/AuthKeysController.php +++ b/app/Controller/AuthKeysController.php @@ -50,9 +50,12 @@ class AuthKeysController extends AppController if ($this->IndexFilter->isRest()) { return $this->restResponsePayload; } + $this->set('title_for_layout', __('Auth Keys')); $this->set('keyUsageEnabled', $keyUsageEnabled); - $this->set('metaGroup', $this->_isAdmin ? 'admin' : 'globalActions'); - $this->set('metaAction', 'authkeys_index'); + $this->set('menuData', [ + 'menuList' => $this->_isSiteAdmin() ? 'admin' : 'globalActions', + 'menuItem' => 'authkeys_index', + ]); } public function delete($id) @@ -131,6 +134,7 @@ class AuthKeysController extends AppController $this->set('uniqueIps', $uniqueIps); } + $this->set('title_for_layout', __('Auth Key')); $this->set('menuData', [ 'menuList' => $this->_isSiteAdmin() ? 'admin' : 'globalActions', 'menuItem' => 'authKeyView', diff --git a/app/View/AuthKeys/index.ctp b/app/View/AuthKeys/index.ctp index 7c131eb32..65009e4c1 100644 --- a/app/View/AuthKeys/index.ctp +++ b/app/View/AuthKeys/index.ctp @@ -28,7 +28,6 @@ 'type' => 'search', 'button' => __('Filter'), 'placeholder' => __('Enter value to search'), - 'data' => '', 'searchKey' => 'value' ] ] @@ -43,9 +42,12 @@ 'name' => __('User'), 'sort' => 'User.email', 'data_path' => 'User.email', + 'element' => empty($user_id) ? 'links' : 'generic_field', + 'url' => $baseurl . '/users/view', + 'url_params_data_paths' => ['User.id'], ], [ - 'name' => __('Auth key'), + 'name' => __('Auth Key'), 'sort' => 'AuthKey.authkey_start', 'element' => 'authkey', 'data_path' => 'AuthKey', @@ -94,19 +96,14 @@ ]); echo ''; if (empty($ajax)) { - echo $this->element('/genericElements/SideMenu/side_menu', array('menuList' => $metaGroup, 'menuItem' => $this->action)); + echo $this->element('/genericElements/SideMenu/side_menu', $menuData); } ?> diff --git a/app/View/AuthKeys/view.ctp b/app/View/AuthKeys/view.ctp index 17aa4263a..b2ccacc4e 100644 --- a/app/View/AuthKeys/view.ctp +++ b/app/View/AuthKeys/view.ctp @@ -27,20 +27,10 @@ echo $this->element( 'path' => 'AuthKey.uuid', ], [ - 'key' => __('Auth key'), + 'key' => __('Auth Key'), 'path' => 'AuthKey', 'type' => 'authkey' ], - [ - 'key' => __('Created'), - 'path' => 'AuthKey.created', - 'type' => 'datetime' - ], - [ - 'key' => __('Expiration'), - 'path' => 'AuthKey.expiration', - 'type' => 'expiration' - ], [ 'key' => __('User'), 'path' => 'User.id', @@ -52,6 +42,16 @@ echo $this->element( 'key' => __('Comment'), 'path' => 'AuthKey.comment' ], + [ + 'key' => __('Created'), + 'path' => 'AuthKey.created', + 'type' => 'datetime' + ], + [ + 'key' => __('Expiration'), + 'path' => 'AuthKey.expiration', + 'type' => 'expiration' + ], [ 'key' => __('Key usage'), 'type' => 'sparkline', diff --git a/app/View/genericTemplates/delete.ctp b/app/View/genericTemplates/delete.ctp index 332d6a39f..c33404aa8 100644 --- a/app/View/genericTemplates/delete.ctp +++ b/app/View/genericTemplates/delete.ctp @@ -1,12 +1,15 @@ +params['controller']))); +?>