From 6caccac94dd2818050345b45ef25ec4e02f6e10a Mon Sep 17 00:00:00 2001 From: Christophe Vandeplas Date: Fri, 19 May 2023 06:57:16 +0200 Subject: [PATCH 01/13] new: [security] TOTP authentication --- app/Controller/AppController.php | 2 +- app/Controller/Component/ACLComponent.php | 1 + app/Controller/UsersController.php | 56 +++++++++++++++++++---- app/Model/AppModel.php | 5 +- app/View/Users/totp.ctp | 26 +++++++++++ app/View/Users/view.ctp | 13 ++++++ app/composer.json | 6 ++- 7 files changed, 96 insertions(+), 13 deletions(-) create mode 100644 app/View/Users/totp.ctp diff --git a/app/Controller/AppController.php b/app/Controller/AppController.php index 7b7372071..c8ee04d59 100755 --- a/app/Controller/AppController.php +++ b/app/Controller/AppController.php @@ -310,7 +310,7 @@ class AppController extends Controller $this->__accessMonitor($user); } else { - $preAuthActions = array('login', 'register', 'getGpgPublicKey', 'logout401'); + $preAuthActions = array('login', 'register', 'getGpgPublicKey', 'logout401', 'totp'); if (!empty(Configure::read('Security.email_otp_enabled'))) { $preAuthActions[] = 'email_otp'; } diff --git a/app/Controller/Component/ACLComponent.php b/app/Controller/Component/ACLComponent.php index 36152778e..e8fdeeb6c 100644 --- a/app/Controller/Component/ACLComponent.php +++ b/app/Controller/Component/ACLComponent.php @@ -745,6 +745,7 @@ class ACLComponent extends Component 'downloadTerms' => array('*'), 'edit' => array('self_management_enabled'), 'email_otp' => array('*'), + 'totp' => array('*'), 'searchGpgKey' => array('*'), 'fetchGpgKey' => array('*'), 'histogram' => array('*'), diff --git a/app/Controller/UsersController.php b/app/Controller/UsersController.php index b31275758..53916e839 100644 --- a/app/Controller/UsersController.php +++ b/app/Controller/UsersController.php @@ -1,5 +1,5 @@ User->find('first', [ + $unauth_user = $this->User->find('first', [ 'conditions' => ['User.email' => $this->request->data['User']['email']], - 'fields' => ['User.password'], + 'fields' => ['User.password', 'User.totp'], 'recursive' => -1, ]); - if (!empty($userPass) && strlen($userPass['User']['password']) === 40) { - $oldHash = true; - unset($this->Auth->authenticate['Form']['passwordHasher']); // use default password hasher - $this->Auth->constructAuthenticate(); + if ($unauth_user) { + // Check the length of the user's authkey match old format. This can be removed in future. + $userPass = $unauth_user['User']['password']; + if (!empty($userPass) && strlen($userPass) === 40) { + $oldHash = true; + unset($this->Auth->authenticate['Form']['passwordHasher']); // use default password hasher + $this->Auth->constructAuthenticate(); + } + // user has TOTP token, check creds and redirect to TOTP validation + if ($unauth_user['User']['totp'] && !$unauth_user['User']['disabled'] && class_exists('\OTPHP\TOTP')) { + $user = $this->Auth->identify($this->request, $this->response); + if ($user && !$user['disabled']) { + $this->Session->write('totp_user', $user); + return $this->redirect('totp'); + } + } } } + // if instance requires email OTP if ($this->request->is('post') && Configure::read('Security.email_otp_enabled')) { $user = $this->Auth->identify($this->request, $this->response); if ($user && !$user['disabled']) { @@ -1748,6 +1760,31 @@ class UsersController extends AppController } } + public function totp() + { + $user = $this->Session->read('totp_user'); + if (empty($user)) { + $this->redirect('login'); + } + if ($this->request->is('post') && isset($this->request->data['User']['otp'])) { + $secret = $user['totp']; + $otp = \OTPHP\TOTP::create($secret); + $otp_now = $otp->now(); + if (trim($this->request->data['User']['otp']) == $otp_now) { + // we invalidate the previously generated OTP + // We login the user with CakePHP + $this->Auth->login($user); + $this->_postlogin(); + } else { + $this->Flash->error(__("The OTP is incorrect or has expired")); + // FIXME chri - log error that OTP was wrong + } + } else { + // GET Request, just show the form + } + + } + public function email_otp() { $user = $this->Session->read('email_otp_user'); @@ -1767,6 +1804,7 @@ class UsersController extends AppController $this->_postlogin(); } else { $this->Flash->error(__("The OTP is incorrect or has expired")); + // FIXME chri - log error that OTP was wrong. } } else { // GET Request diff --git a/app/Model/AppModel.php b/app/Model/AppModel.php index f0bd48fdc..8b2cd9d4d 100644 --- a/app/Model/AppModel.php +++ b/app/Model/AppModel.php @@ -84,7 +84,7 @@ class AppModel extends Model 87 => false, 88 => false, 89 => false, 90 => false, 91 => false, 92 => false, 93 => false, 94 => false, 95 => true, 96 => false, 97 => true, 98 => false, 99 => false, 100 => false, 101 => false, 102 => false, 103 => false, 104 => false, - 105 => false, 106 => false, 107 => false, 108 => false, + 105 => false, 106 => false, 107 => false, 108 => false, 109 => false ); const ADVANCED_UPDATES_DESCRIPTION = array( @@ -1950,6 +1950,9 @@ class AppModel extends Model case 108: $sqlArray[] = "ALTER TABLE `workflows` MODIFY `data` LONGTEXT;"; break; + case 109: + $sqlArray[] = "ALTER TABLE `users` ADD `totp` varchar(255) DEFAULT NULL;"; + break; case 'fixNonEmptySharingGroupID': $sqlArray[] = 'UPDATE `events` SET `sharing_group_id` = 0 WHERE `distribution` != 4;'; $sqlArray[] = 'UPDATE `attributes` SET `sharing_group_id` = 0 WHERE `distribution` != 4;'; diff --git a/app/View/Users/totp.ctp b/app/View/Users/totp.ctp new file mode 100644 index 000000000..8d84c42ec --- /dev/null +++ b/app/View/Users/totp.ctp @@ -0,0 +1,26 @@ +Flash->render(); ?> + +
+
+

+
+
+ +element('/genericElements/Form/genericForm', array( + "form" => $this->Form, + "data" => array( + "title" => __("Validate your One Time Password"), + "fields" => array( + array( + "field" => "otp", + "label" => __("One Time Password"), + "type" => "text", + "placeholder" => __("Enter your OTP here"), + ), + ), + "submit" => array ( + "action" => "totp", + ), +))); +?> diff --git a/app/View/Users/view.ctp b/app/View/Users/view.ctp index c13b42454..172f7b8de 100755 --- a/app/View/Users/view.ctp +++ b/app/View/Users/view.ctp @@ -18,6 +18,14 @@ foreach ($notificationTypes as $notificationType => $description) { } $notificationsHtml .= ''; +$isTotp = isset($user['User']['totp']) ? true : false; +$boolean = sprintf( +'%s', + $isTotp ? 'label label-success label-padding' : 'label label-important label-padding', +$isTotp ? __('Yes') : __('No')); +$totpHtml = $boolean . ''. __('Generate TOTP'); // FIXME chri - create link to generate a new TOTP, only save in DB after validation + + $table_data = [ array('key' => __('ID'), 'value' => $user['User']['id']), array( @@ -37,6 +45,11 @@ $notificationsHtml .= ''; 'key' => __('Role'), 'html' => $this->Html->link($user['Role']['name'], array('controller' => 'roles', 'action' => 'view', $user['Role']['id'])), ), + // array('key' => __('TOTP'), 'boolean' => isset($user['User']['totp']) ? true : false), + array( + 'key' => __('TOTP'), + 'html' => $totpHtml + ), array( 'key' => __('Email notifications'), 'html' => $notificationsHtml, diff --git a/app/composer.json b/app/composer.json index 4127df945..987346a71 100644 --- a/app/composer.json +++ b/app/composer.json @@ -11,7 +11,8 @@ "ext-pcre": "*", "kamisama/cake-resque": "4.1.2", "pear/crypt_gpg": "1.6.7", - "monolog/monolog": "1.24.0" + "monolog/monolog": "1.24.0", + "spomky-labs/otphp": "^10.0" }, "require-dev": { "phpunit/phpunit": "^8", @@ -38,7 +39,8 @@ "supervisorphp/supervisor": "For managing background jobs", "lstrojny/fxmlrpc": "Required for supervisorphp/supervisor XML-RPC requests", "guzzlehttp/guzzle": "Required for supervisorphp/supervisor XML-RPC requests", - "php-http/message": "Required for supervisorphp/supervisor XML-RPC requests" + "php-http/message": "Required for supervisorphp/supervisor XML-RPC requests", + "spomky-labs/otphp": "Required for strong authentication with TOTP" }, "config": { "vendor-dir": "Vendor", From 61573392ea4d222dec520bee1017c02759aab843 Mon Sep 17 00:00:00 2001 From: Christophe Vandeplas Date: Fri, 19 May 2023 20:56:52 +0200 Subject: [PATCH 02/13] chg: [security] allow creation of TOTP token --- app/Controller/Component/ACLComponent.php | 1 + app/Controller/UsersController.php | 48 +++++++++++++++++++++++ app/View/Users/totp_new.ctp | 35 +++++++++++++++++ app/View/Users/view.ctp | 4 +- app/composer.json | 6 ++- 5 files changed, 91 insertions(+), 3 deletions(-) create mode 100644 app/View/Users/totp_new.ctp diff --git a/app/Controller/Component/ACLComponent.php b/app/Controller/Component/ACLComponent.php index e8fdeeb6c..2ea20b247 100644 --- a/app/Controller/Component/ACLComponent.php +++ b/app/Controller/Component/ACLComponent.php @@ -746,6 +746,7 @@ class ACLComponent extends Component 'edit' => array('self_management_enabled'), 'email_otp' => array('*'), 'totp' => array('*'), + 'totp_new' => array('*'), 'searchGpgKey' => array('*'), 'fetchGpgKey' => array('*'), 'histogram' => array('*'), diff --git a/app/Controller/UsersController.php b/app/Controller/UsersController.php index 53916e839..882130421 100644 --- a/app/Controller/UsersController.php +++ b/app/Controller/UsersController.php @@ -1207,6 +1207,10 @@ class UsersController extends AppController return $this->redirect('totp'); } } + // FIXME chri - implement a way for the user to login the first time when TOTP is required for all users, + // - redirect to TOTP generation page just after login (or any page requested) + // - for new account creations + // - for when the org admin wants to allow the user to generate a new TOTP } } // if instance requires email OTP @@ -1782,7 +1786,51 @@ class UsersController extends AppController } else { // GET Request, just show the form } + } + public function totp_new($id = null) + { + // FIXME chri - reuse $id to load user if (org)site admin + $user = $this->Auth->user(); + // do not allow this page to be accessed if the current already has a TOTP. Just redirect to the users details page with a Flash->error() + if ($user['totp']) { + $this->Flash->error(__("Your account already has an TOTP. Please contact your organisational administrator to change or delete it.")); + $this->redirect($this->referer()); + } + + $secret = $this->Session->read('totp_secret'); // Reload secret from session. + if ($secret) { + $otp = \OTPHP\TOTP::create($secret); + } else { + $otp = \OTPHP\TOTP::create(); + $secret = $otp->getSecret(); + $this->Session->write('totp_secret', $secret); // Store in session, this is to keep the same QR code even if the page refreshes. + } + if ($this->request->is('post') && isset($this->request->data['User']['otp'])) { + $otp_now = $otp->now(); + if (trim($this->request->data['User']['otp']) == $otp_now) { + // we know the user can generate TOTP tokens, save the new TOTP to the database + $this->User->id = $user['id']; + $this->User->saveField('totp', $secret); + $this->Flash->info(__('The OTP is correct and now active for your account.')); + $this->redirect(array('controller' => 'events', 'action'=> 'index')); + } else { + $this->Flash->error(__("The OTP is incorrect or has expired.")); + } + } else { + // GET Request, just show the form + } + // generate QR code with the secret + $renderer = new \BaconQrCode\Renderer\ImageRenderer( + new \BaconQrCode\Renderer\RendererStyle\RendererStyle(200), + new \BaconQrCode\Renderer\Image\SvgImageBackEnd() + ); + $writer = new \BaconQrCode\Writer($renderer); + $qrcode = $writer->writeString('otpauth://totp/' . Configure::read('MISP.org') . ' MISP (' . $user['email'] . ')?secret=' . $secret); + $writer = preg_replace('/^.+\n/', '', $qrcode); // ignore first set('qrcode', $qrcode); + $this->set('secret', $secret); } public function email_otp() diff --git a/app/View/Users/totp_new.ctp b/app/View/Users/totp_new.ctp new file mode 100644 index 000000000..52e3a04c5 --- /dev/null +++ b/app/View/Users/totp_new.ctp @@ -0,0 +1,35 @@ +Flash->render(); ?> + +
+
+

+

+
+
+ +
+ +

Alternatively you can enter the following secret in your TOTP application:

+element('/genericElements/Form/genericForm', array( + "form" => $this->Form, + "data" => array( + "title" => __("Validate your One Time Password"), + "fields" => array( + array( + "field" => "otp", + "label" => __("One Time Password"), + "type" => "text", + "placeholder" => __("Enter your OTP code here"), + ) + ), + "submit" => array ( + "action" => "totp", + ), +))); +?> +
diff --git a/app/View/Users/view.ctp b/app/View/Users/view.ctp index 172f7b8de..4ee4dbfa8 100755 --- a/app/View/Users/view.ctp +++ b/app/View/Users/view.ctp @@ -23,7 +23,9 @@ $boolean = sprintf( '%s', $isTotp ? 'label label-success label-padding' : 'label label-important label-padding', $isTotp ? __('Yes') : __('No')); -$totpHtml = $boolean . ''. __('Generate TOTP'); // FIXME chri - create link to generate a new TOTP, only save in DB after validation +$totpHtml = $boolean; +$totpHtml .= ($isTotp ? '' : $this->Html->link(__('Generate'), array('action' => 'totp_new', $user['User']['id']))); +$totpHtml .= ($admin_view && $isTotp ? ' ' . __('Delete') : ''); // FIXME chri - only allow delete for (org)admin $table_data = [ diff --git a/app/composer.json b/app/composer.json index 987346a71..952681602 100644 --- a/app/composer.json +++ b/app/composer.json @@ -12,7 +12,8 @@ "kamisama/cake-resque": "4.1.2", "pear/crypt_gpg": "1.6.7", "monolog/monolog": "1.24.0", - "spomky-labs/otphp": "^10.0" + "spomky-labs/otphp": "^10.0", + "bacon/bacon-qr-code": "^2.0" }, "require-dev": { "phpunit/phpunit": "^8", @@ -40,7 +41,8 @@ "lstrojny/fxmlrpc": "Required for supervisorphp/supervisor XML-RPC requests", "guzzlehttp/guzzle": "Required for supervisorphp/supervisor XML-RPC requests", "php-http/message": "Required for supervisorphp/supervisor XML-RPC requests", - "spomky-labs/otphp": "Required for strong authentication with TOTP" + "spomky-labs/otphp": "Required for strong authentication with TOTP", + "bacon/bacon-qr-code": "Required for strong authentication with TOTP" }, "config": { "vendor-dir": "Vendor", From 28cec403b9d410a89b9c17a7847e21668a1af5a4 Mon Sep 17 00:00:00 2001 From: Christophe Vandeplas Date: Sat, 20 May 2023 07:01:14 +0200 Subject: [PATCH 03/13] chg: [security] TOTP UI love --- .../genericElements/Form/Fields/htmlField.ctp | 5 +++ app/View/Users/totp_new.ctp | 33 ++++++++++--------- 2 files changed, 23 insertions(+), 15 deletions(-) create mode 100644 app/View/Elements/genericElements/Form/Fields/htmlField.ctp diff --git a/app/View/Elements/genericElements/Form/Fields/htmlField.ctp b/app/View/Elements/genericElements/Form/Fields/htmlField.ctp new file mode 100644 index 000000000..6c394efdb --- /dev/null +++ b/app/View/Elements/genericElements/Form/Fields/htmlField.ctp @@ -0,0 +1,5 @@ +Flash->render(); ?> - -
-
-

-

-
-
- -
- -

Alternatively you can enter the following secret in your TOTP application:

" . $secret . ""; echo $this->element('/genericElements/Form/genericForm', array( "form" => $this->Form, "data" => array( "title" => __("Validate your One Time Password"), "fields" => array( + array( + "type" => 'html', + "field" => "html", + "html" => $detailsHtml + ), + array( + "type" => 'html', + "field" => 'qrcode', + "html" => $qrcode + ), + array( + "type" => 'html', + "field" => "secret", + "html" => $secretHtml + ), array( "field" => "otp", - "label" => __("One Time Password"), + "label" => __("One Time Password verification"), "type" => "text", "placeholder" => __("Enter your OTP code here"), ) From 856a9e4b4cc59820eb4ddb5d12b57625013f4a95 Mon Sep 17 00:00:00 2001 From: Christophe Vandeplas Date: Sat, 20 May 2023 08:05:48 +0200 Subject: [PATCH 04/13] chg: [security] admins can delete user TOTP --- app/Controller/Component/ACLComponent.php | 1 + app/Controller/UsersController.php | 69 +++++++++++++++++------ app/View/Users/admin_index.ctp | 10 ++++ app/View/Users/view.ctp | 3 +- 4 files changed, 65 insertions(+), 18 deletions(-) diff --git a/app/Controller/Component/ACLComponent.php b/app/Controller/Component/ACLComponent.php index 2ea20b247..94582d5a4 100644 --- a/app/Controller/Component/ACLComponent.php +++ b/app/Controller/Component/ACLComponent.php @@ -747,6 +747,7 @@ class ACLComponent extends Component 'email_otp' => array('*'), 'totp' => array('*'), 'totp_new' => array('*'), + 'totp_delete' => array('perm_admin'), 'searchGpgKey' => array('*'), 'fetchGpgKey' => array('*'), 'histogram' => array('*'), diff --git a/app/Controller/UsersController.php b/app/Controller/UsersController.php index 882130421..e77851482 100644 --- a/app/Controller/UsersController.php +++ b/app/Controller/UsersController.php @@ -67,6 +67,7 @@ class UsersController extends AppController $user['User']['pgp_status'] = isset($pgpDetails[2]) ? $pgpDetails[2] : 'OK'; $user['User']['fingerprint'] = !empty($pgpDetails[4]) ? $pgpDetails[4] : 'N/A'; } + // FIXME chri - show warning for TOTP if LinOTP is enabled if ($this->_isRest()) { return $this->RestResponse->viewData($this->__massageUserObject($user), $this->response->type()); } else { @@ -88,6 +89,7 @@ class UsersController extends AppController unset($user['User']['authkey']); } $user['User']['password'] = '*****'; + $user['User']['totp'] = '*****'; $temp = []; $objectsToInclude = array('User', 'Role', 'UserSetting', 'Organisation'); foreach ($objectsToInclude as $objectToInclude) { @@ -588,17 +590,7 @@ class UsersController extends AppController unset($user['User']['authkey']); } if ($this->_isRest()) { - $user['User']['password'] = '*****'; - $temp = array(); - foreach ($user['UserSetting'] as $v) { - $temp[$v['setting']] = $v['value']; - } - $user['UserSetting'] = $temp; - return $this->RestResponse->viewData(array( - 'User' => $user['User'], - 'Role' => $user['Role'], - 'UserSetting' => $user['UserSetting'] - ), $this->response->type()); + return $this->RestResponse->viewData($this->__massageUserObject($user), $this->response->type()); } $this->set('user', $user); @@ -1788,12 +1780,22 @@ class UsersController extends AppController } } - public function totp_new($id = null) + public function totp_new() { - // FIXME chri - reuse $id to load user if (org)site admin - $user = $this->Auth->user(); + // only allow the users themselves to generate a TOTP secret. + // If TOTP is enforced they will be invited to generate it at first login + $user = $this->User->find('first', array( + 'recursive' => -1, + 'conditions' => array('User.id' => $this->Auth->user('id')), + 'fields' => array( + 'totp', 'email', 'id' + ) + )); + if (empty($user)) { + throw new NotFoundException(__('Invalid user')); + } // do not allow this page to be accessed if the current already has a TOTP. Just redirect to the users details page with a Flash->error() - if ($user['totp']) { + if ($user['User']['totp']) { $this->Flash->error(__("Your account already has an TOTP. Please contact your organisational administrator to change or delete it.")); $this->redirect($this->referer()); } @@ -1810,7 +1812,7 @@ class UsersController extends AppController $otp_now = $otp->now(); if (trim($this->request->data['User']['otp']) == $otp_now) { // we know the user can generate TOTP tokens, save the new TOTP to the database - $this->User->id = $user['id']; + $this->User->id = $user['User']['id']; $this->User->saveField('totp', $secret); $this->Flash->info(__('The OTP is correct and now active for your account.')); $this->redirect(array('controller' => 'events', 'action'=> 'index')); @@ -1826,13 +1828,46 @@ class UsersController extends AppController new \BaconQrCode\Renderer\Image\SvgImageBackEnd() ); $writer = new \BaconQrCode\Writer($renderer); - $qrcode = $writer->writeString('otpauth://totp/' . Configure::read('MISP.org') . ' MISP (' . $user['email'] . ')?secret=' . $secret); + $qrcode = $writer->writeString('otpauth://totp/' . Configure::read('MISP.org') . ' MISP (' . $user['User']['email'] . ')?secret=' . $secret); $writer = preg_replace('/^.+\n/', '', $qrcode); // ignore first set('qrcode', $qrcode); $this->set('secret', $secret); } + public function totp_delete($id) { + if ($this->request->is('post') || $this->request->is('delete')) { + $user = $this->User->find('first', array( + 'conditions' => $this->__adminFetchConditions($id), + 'recursive' => -1 + )); + if (empty($user)) { + throw new NotFoundException(__('Invalid user')); + } + $this->User->id = $id; + if ($this->User->saveField('totp', null)) { + $fieldsDescrStr = 'User (' . $id . '): ' . $user['User']['email'] . 'TOTP deleted'; + $this->User->extralog($this->Auth->user(), "update", $fieldsDescrStr, ''); + if ($this->_isRest()) { + return $this->RestResponse->saveSuccessResponse('User', 'admin_totp_delete', $id, $this->response->type(), 'User TOTP deleted.'); + } else { + $this->Flash->success(__('User TOTP deleted')); + $this->redirect('/admin/users/index'); + } + } + $this->Flash->error(__('User TOTP was not deleted')); + $this->redirect('/admin/users/index'); + } else { + $this->set( + 'question', + __('Are you sure you want to delete the TOTP of the user?.') + ); + $this->set('title', __('Delete user TOTP')); + $this->set('actionName', 'Delete'); + $this->render('/genericTemplates/confirm'); + } + } + public function email_otp() { $user = $this->Session->read('email_otp_user'); diff --git a/app/View/Users/admin_index.ctp b/app/View/Users/admin_index.ctp index 8205f269e..fa51b49f8 100755 --- a/app/View/Users/admin_index.ctp +++ b/app/View/Users/admin_index.ctp @@ -138,6 +138,16 @@ 'privacy' => 1, 'requirement' => empty(Configure::read('Security.advanced_authkeys')) ), + array( + 'name' => '', + 'header_title' => __('TOTP'), + 'icon' => 'mobile', + 'element' => 'boolean', + 'sort' => 'User.totp', + 'class' => 'short', + 'data_path' => 'User.totp', + 'colors' => true, + ), array( 'name' => '', 'header_title' => __('Contact alert'), diff --git a/app/View/Users/view.ctp b/app/View/Users/view.ctp index 4ee4dbfa8..c4d32e6d0 100755 --- a/app/View/Users/view.ctp +++ b/app/View/Users/view.ctp @@ -25,7 +25,8 @@ $boolean = sprintf( $isTotp ? __('Yes') : __('No')); $totpHtml = $boolean; $totpHtml .= ($isTotp ? '' : $this->Html->link(__('Generate'), array('action' => 'totp_new', $user['User']['id']))); -$totpHtml .= ($admin_view && $isTotp ? ' ' . __('Delete') : ''); // FIXME chri - only allow delete for (org)admin + +$totpHtml .= ($admin_view && $isTotp ? ' ' . $this->Form->postLink(__('Delete'), array('action' => 'totp_delete', $user['User']['id'])) : ''); // FIXME chri - only allow delete for (org)admin $table_data = [ From 81db5958d9574b86b5197daee36703e241e05b8c Mon Sep 17 00:00:00 2001 From: Christophe Vandeplas Date: Sat, 20 May 2023 08:56:40 +0200 Subject: [PATCH 05/13] chg: [security] Allow enforcement of TOTP --- app/Controller/AppController.php | 6 ++++++ app/Controller/UsersController.php | 5 +---- app/Model/Server.php | 20 ++++++++++++++++++++ app/View/Users/view.ctp | 3 +-- 4 files changed, 28 insertions(+), 6 deletions(-) diff --git a/app/Controller/AppController.php b/app/Controller/AppController.php index c8ee04d59..6e9e21379 100755 --- a/app/Controller/AppController.php +++ b/app/Controller/AppController.php @@ -605,6 +605,12 @@ class AppController extends Controller return true; } + // Check if user must create TOTP secret, force them to be on that page as long as needed. + if (!$user['totp'] && Configure::read('Security.totp_required') && !$this->_isControllerAction(['users' => ['terms', 'change_pw', 'logout', 'login', 'totp_new']])) { // TOTP is mandatory for users, prevent login until the user has configured their TOTP + $this->redirect(array('controller' => 'users', 'action' => 'totp_new', 'admin' => false)); + return false; + } + // 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.'); diff --git a/app/Controller/UsersController.php b/app/Controller/UsersController.php index e77851482..bea9e387f 100644 --- a/app/Controller/UsersController.php +++ b/app/Controller/UsersController.php @@ -1199,10 +1199,6 @@ class UsersController extends AppController return $this->redirect('totp'); } } - // FIXME chri - implement a way for the user to login the first time when TOTP is required for all users, - // - redirect to TOTP generation page just after login (or any page requested) - // - for new account creations - // - for when the org admin wants to allow the user to generate a new TOTP } } // if instance requires email OTP @@ -1814,6 +1810,7 @@ class UsersController extends AppController // we know the user can generate TOTP tokens, save the new TOTP to the database $this->User->id = $user['User']['id']; $this->User->saveField('totp', $secret); + $this->_refreshAuth(); $this->Flash->info(__('The OTP is correct and now active for your account.')); $this->redirect(array('controller' => 'events', 'action'=> 'index')); } else { diff --git a/app/Model/Server.php b/app/Model/Server.php index b0d69eea5..0fb9860a9 100644 --- a/app/Model/Server.php +++ b/app/Model/Server.php @@ -2140,6 +2140,17 @@ class Server extends AppModel return true; } + public function totpBeforeHook($setting, $value) + { + if ($value && (!class_exists('\OTPHP\TOTP') || !class_exists('\BaconQrCode\Writer'))) { + return __('The TOTP and QR code generation libraries are not installed. Enabling OTP without those libraries installed would lock all users out.'); + } + if ($value && Configure::read('LinOTPAuth.enabled')) { + return __('The TOTP and LinOTPAuth should not be used at the same time.'); + } + return true; + } + public function testForRPZSerial($value) { if ($this->testForEmpty($value) !== true) { @@ -6364,6 +6375,15 @@ class Server extends AppModel 'type' => 'boolean', 'null' => true, ], + 'totp_required' => array( + 'level' => 2, + 'description' => __('Require authentication with TOTP. Users that do not have TOTP configured will be forced to create a token at first login. You cannot use it in combination with external authentication plugins.'), + 'value' => false, + 'test' => 'testBool', + 'beforeHook' => 'totpBeforeHook', + 'type' => 'boolean', + 'null' => true + ), 'email_otp_enabled' => array( 'level' => 2, 'description' => __('Enable two step authentication with a OTP sent by email. Requires e-mailing to be enabled. Warning: You cannot use it in combination with external authentication plugins.'), diff --git a/app/View/Users/view.ctp b/app/View/Users/view.ctp index c4d32e6d0..a1310976e 100755 --- a/app/View/Users/view.ctp +++ b/app/View/Users/view.ctp @@ -25,8 +25,7 @@ $boolean = sprintf( $isTotp ? __('Yes') : __('No')); $totpHtml = $boolean; $totpHtml .= ($isTotp ? '' : $this->Html->link(__('Generate'), array('action' => 'totp_new', $user['User']['id']))); - -$totpHtml .= ($admin_view && $isTotp ? ' ' . $this->Form->postLink(__('Delete'), array('action' => 'totp_delete', $user['User']['id'])) : ''); // FIXME chri - only allow delete for (org)admin +$totpHtml .= ($admin_view && $isTotp ? ' ' . $this->Form->postLink(__('Delete'), array('action' => 'totp_delete', $user['User']['id'])) : ''); $table_data = [ From dac7aaf7d693b4e5ae2be7e7d344f185c272a200 Mon Sep 17 00:00:00 2001 From: Christophe Vandeplas Date: Sat, 20 May 2023 09:20:36 +0200 Subject: [PATCH 06/13] chg: [security] Disallow creation of TOTP token if LinOTP is enabled --- app/Controller/UsersController.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/Controller/UsersController.php b/app/Controller/UsersController.php index bea9e387f..dd7470a4c 100644 --- a/app/Controller/UsersController.php +++ b/app/Controller/UsersController.php @@ -67,7 +67,6 @@ class UsersController extends AppController $user['User']['pgp_status'] = isset($pgpDetails[2]) ? $pgpDetails[2] : 'OK'; $user['User']['fingerprint'] = !empty($pgpDetails[4]) ? $pgpDetails[4] : 'N/A'; } - // FIXME chri - show warning for TOTP if LinOTP is enabled if ($this->_isRest()) { return $this->RestResponse->viewData($this->__massageUserObject($user), $this->response->type()); } else { @@ -1778,6 +1777,10 @@ class UsersController extends AppController public function totp_new() { + if (Configure::read('LinOTPAuth.enabled')) { + $this->Flash->error(__("LinOTP is enabled for this instance. Build-in TOTP should not be used.")); + $this->redirect($this->referer()); + } // only allow the users themselves to generate a TOTP secret. // If TOTP is enforced they will be invited to generate it at first login $user = $this->User->find('first', array( From 8e370fa6f09ee2502f72e27ee35e534f99988e8a Mon Sep 17 00:00:00 2001 From: Christophe Vandeplas Date: Sat, 20 May 2023 10:13:56 +0200 Subject: [PATCH 07/13] chg: [security] TOTP event logging --- app/Controller/UsersController.php | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/app/Controller/UsersController.php b/app/Controller/UsersController.php index dd7470a4c..6d9a5759e 100644 --- a/app/Controller/UsersController.php +++ b/app/Controller/UsersController.php @@ -1768,7 +1768,8 @@ class UsersController extends AppController $this->_postlogin(); } else { $this->Flash->error(__("The OTP is incorrect or has expired")); - // FIXME chri - log error that OTP was wrong + $fieldsDescrStr = 'User (' . $user['id'] . '): ' . $user['email']. ' wrong TOTP token'; + $this->User->extralog($user, "login_fail", $fieldsDescrStr, ''); } } else { // GET Request, just show the form @@ -1815,6 +1816,8 @@ class UsersController extends AppController $this->User->saveField('totp', $secret); $this->_refreshAuth(); $this->Flash->info(__('The OTP is correct and now active for your account.')); + $fieldsDescrStr = 'User (' . $user['User']['id'] . '): ' . $user['User']['email']. ' TOTP token created'; + $this->User->extralog($this->Auth->user(), "update", $fieldsDescrStr, ''); $this->redirect(array('controller' => 'events', 'action'=> 'index')); } else { $this->Flash->error(__("The OTP is incorrect or has expired.")); @@ -1846,7 +1849,7 @@ class UsersController extends AppController } $this->User->id = $id; if ($this->User->saveField('totp', null)) { - $fieldsDescrStr = 'User (' . $id . '): ' . $user['User']['email'] . 'TOTP deleted'; + $fieldsDescrStr = 'User (' . $id . '): ' . $user['User']['email'] . ' TOTP deleted'; $this->User->extralog($this->Auth->user(), "update", $fieldsDescrStr, ''); if ($this->_isRest()) { return $this->RestResponse->saveSuccessResponse('User', 'admin_totp_delete', $id, $this->response->type(), 'User TOTP deleted.'); @@ -1887,7 +1890,8 @@ class UsersController extends AppController $this->_postlogin(); } else { $this->Flash->error(__("The OTP is incorrect or has expired")); - // FIXME chri - log error that OTP was wrong. + $fieldsDescrStr = 'User (' . $user['id'] . '): ' . $user['email']. ' wrong email OTP token'; + $this->User->extralog($user, "login_fail", $fieldsDescrStr, ''); } } else { // GET Request From e90083020ff523cbcdcdcc8db4558ad8c114eab0 Mon Sep 17 00:00:00 2001 From: Christophe Vandeplas Date: Sat, 20 May 2023 10:26:45 +0200 Subject: [PATCH 08/13] chg: [security] Require TOTP and QR code lib for TOTP secret creation --- app/Controller/UsersController.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/Controller/UsersController.php b/app/Controller/UsersController.php index 6d9a5759e..9c80abe2c 100644 --- a/app/Controller/UsersController.php +++ b/app/Controller/UsersController.php @@ -1782,6 +1782,10 @@ class UsersController extends AppController $this->Flash->error(__("LinOTP is enabled for this instance. Build-in TOTP should not be used.")); $this->redirect($this->referer()); } + if (!class_exists('\OTPHP\TOTP') || !class_exists('\BaconQrCode\Writer')) { + $this->Flash->error(__("The required PHP libraries to support TOTP are not installed. Please contact your administrator to address this.")); + $this->redirect($this->referer()); + } // only allow the users themselves to generate a TOTP secret. // If TOTP is enforced they will be invited to generate it at first login $user = $this->User->find('first', array( From afbb9fab95da1828322a20cb18790bed75aa4bf5 Mon Sep 17 00:00:00 2001 From: Christophe Vandeplas Date: Thu, 25 May 2023 21:12:07 +0200 Subject: [PATCH 09/13] chg: [security] TOTP anti-bruteforce support --- app/Controller/UsersController.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/Controller/UsersController.php b/app/Controller/UsersController.php index 9a586f590..8bf3dd196 100644 --- a/app/Controller/UsersController.php +++ b/app/Controller/UsersController.php @@ -1764,6 +1764,11 @@ class UsersController extends AppController $this->redirect('login'); } if ($this->request->is('post') && isset($this->request->data['User']['otp'])) { + $this->Bruteforce = ClassRegistry::init('Bruteforce'); + if ($this->Bruteforce->isBlocklisted($user['email'])) { + $expire = Configure::check('SecureAuth.expire') ? Configure::read('SecureAuth.expire') : 300; + throw new ForbiddenException('You have reached the maximum number of login attempts. Please wait ' . $expire . ' seconds and try again.'); + } $secret = $user['totp']; $otp = \OTPHP\TOTP::create($secret); $otp_now = $otp->now(); @@ -1776,6 +1781,7 @@ class UsersController extends AppController $this->Flash->error(__("The OTP is incorrect or has expired")); $fieldsDescrStr = 'User (' . $user['id'] . '): ' . $user['email']. ' wrong TOTP token'; $this->User->extralog($user, "login_fail", $fieldsDescrStr, ''); + $this->Bruteforce->insert($user['email']); } } else { // GET Request, just show the form From cb74ad507f0aa2709cf7e9ad1bdb9053e932be58 Mon Sep 17 00:00:00 2001 From: Christophe Vandeplas Date: Thu, 25 May 2023 23:28:14 +0200 Subject: [PATCH 10/13] chg: [security] OTP support for HOTP --- app/Controller/AppController.php | 4 +- app/Controller/Component/ACLComponent.php | 3 +- app/Controller/UsersController.php | 81 ++++++++++++++++------- app/Model/AppModel.php | 1 + app/Model/Server.php | 12 ++-- app/View/Users/hotp.ctp | 24 +++++++ app/View/Users/{totp.ctp => otp.ctp} | 10 +-- app/View/Users/view.ctp | 3 +- 8 files changed, 99 insertions(+), 39 deletions(-) create mode 100644 app/View/Users/hotp.ctp rename app/View/Users/{totp.ctp => otp.ctp} (65%) diff --git a/app/Controller/AppController.php b/app/Controller/AppController.php index 25e971bb6..9315329cf 100755 --- a/app/Controller/AppController.php +++ b/app/Controller/AppController.php @@ -310,7 +310,7 @@ class AppController extends Controller $this->__accessMonitor($user); } else { - $preAuthActions = array('login', 'register', 'getGpgPublicKey', 'logout401', 'totp'); + $preAuthActions = array('login', 'register', 'getGpgPublicKey', 'logout401', 'otp'); if (!empty(Configure::read('Security.email_otp_enabled'))) { $preAuthActions[] = 'email_otp'; } @@ -602,7 +602,7 @@ class AppController extends Controller } // Check if user must create TOTP secret, force them to be on that page as long as needed. - if (!$user['totp'] && Configure::read('Security.totp_required') && !$this->_isControllerAction(['users' => ['terms', 'change_pw', 'logout', 'login', 'totp_new']])) { // TOTP is mandatory for users, prevent login until the user has configured their TOTP + if (!$user['totp'] && Configure::read('Security.otp_required') && !$this->_isControllerAction(['users' => ['terms', 'change_pw', 'logout', 'login', 'totp_new']])) { // TOTP is mandatory for users, prevent login until the user has configured their TOTP $this->redirect(array('controller' => 'users', 'action' => 'totp_new', 'admin' => false)); return false; } diff --git a/app/Controller/Component/ACLComponent.php b/app/Controller/Component/ACLComponent.php index 94582d5a4..e4ab9083f 100644 --- a/app/Controller/Component/ACLComponent.php +++ b/app/Controller/Component/ACLComponent.php @@ -745,7 +745,8 @@ class ACLComponent extends Component 'downloadTerms' => array('*'), 'edit' => array('self_management_enabled'), 'email_otp' => array('*'), - 'totp' => array('*'), + 'otp' => array('*'), + 'hotp' => array('*'), 'totp_new' => array('*'), 'totp_delete' => array('perm_admin'), 'searchGpgKey' => array('*'), diff --git a/app/Controller/UsersController.php b/app/Controller/UsersController.php index 8bf3dd196..1ca4adc8a 100644 --- a/app/Controller/UsersController.php +++ b/app/Controller/UsersController.php @@ -29,7 +29,7 @@ class UsersController extends AppController parent::beforeFilter(); // what pages are allowed for non-logged-in users - $allowedActions = array('login', 'logout', 'getGpgPublicKey', 'logout401', 'totp'); + $allowedActions = array('login', 'logout', 'getGpgPublicKey', 'logout401', 'otp'); if(!empty(Configure::read('Security.email_otp_enabled'))) { $allowedActions[] = 'email_otp'; } @@ -1185,7 +1185,7 @@ class UsersController extends AppController } $unauth_user = $this->User->find('first', [ 'conditions' => ['User.email' => $this->request->data['User']['email']], - 'fields' => ['User.password', 'User.totp'], + 'fields' => ['User.password', 'User.totp', 'User.hotp_counter'], 'recursive' => -1, ]); if ($unauth_user) { @@ -1200,8 +1200,8 @@ class UsersController extends AppController if ($unauth_user['User']['totp'] && !$unauth_user['User']['disabled'] && class_exists('\OTPHP\TOTP')) { $user = $this->Auth->identify($this->request, $this->response); if ($user && !$user['disabled']) { - $this->Session->write('totp_user', $user); - return $this->redirect('totp'); + $this->Session->write('otp_user', $user); + return $this->redirect('otp'); } } } @@ -1757,9 +1757,9 @@ class UsersController extends AppController } } - public function totp() + public function otp() { - $user = $this->Session->read('totp_user'); + $user = $this->Session->read('otp_user'); if (empty($user)) { $this->redirect('login'); } @@ -1770,22 +1770,50 @@ class UsersController extends AppController throw new ForbiddenException('You have reached the maximum number of login attempts. Please wait ' . $expire . ' seconds and try again.'); } $secret = $user['totp']; - $otp = \OTPHP\TOTP::create($secret); - $otp_now = $otp->now(); - if (trim($this->request->data['User']['otp']) == $otp_now) { - // we invalidate the previously generated OTP - // We login the user with CakePHP + $totp = \OTPHP\TOTP::create($secret); + $hotp = \OTPHP\HOTP::create($secret); + if ($totp->verify(trim($this->request->data['User']['otp']))) { + // OTP is correct, we login the user with CakePHP + $this->Auth->login($user); + $this->_postlogin(); + } elseif (isset($user['hotp_counter']) && $hotp->verify(trim($this->request->data['User']['otp']), $user['hotp_counter'])) { + // HOTP is correct, update the counter and login + $this->User->id = $user['id']; + $this->User->saveField('hotp_counter', $user['hotp_counter']+1); $this->Auth->login($user); $this->_postlogin(); } else { $this->Flash->error(__("The OTP is incorrect or has expired")); - $fieldsDescrStr = 'User (' . $user['id'] . '): ' . $user['email']. ' wrong TOTP token'; + $fieldsDescrStr = 'User (' . $user['id'] . '): ' . $user['email']. ' wrong OTP token'; $this->User->extralog($user, "login_fail", $fieldsDescrStr, ''); $this->Bruteforce->insert($user['email']); } - } else { - // GET Request, just show the form } + // GET Request or wrong OTP, just show the form + $this->set('totp', $user['totp']? true : false); + $this->set('hotp_counter', $user['hotp_counter']); + } + + public function hotp() + { + if (!class_exists('\OTPHP\HOTP')) { + $this->Flash->error(__("The required PHP libraries to support OTP are not installed. Please contact your administrator to address this.")); + $this->redirect($this->referer()); + } + + $user = $this->User->find('first', array( + 'recursive' => -1, + 'conditions' => array('User.id' => $this->Auth->user('id')), + 'fields' => array( + 'totp', 'email', 'id', 'hotp_counter' + ) + )); + $hotp = \OTPHP\HOTP::create($user['User']['totp'], $user['User']['hotp_counter']); + $hotp_codes = []; + for ($i=$user['User']['hotp_counter']; $i < $user['User']['hotp_counter']+50 ; $i++) { + $hotp_codes[$i] = $hotp->at($i); + } + $this->set('hotp_codes', $hotp_codes); } public function totp_new() @@ -1816,25 +1844,26 @@ class UsersController extends AppController $this->redirect($this->referer()); } - $secret = $this->Session->read('totp_secret'); // Reload secret from session. + $secret = $this->Session->read('otp_secret'); // Reload secret from session. if ($secret) { - $otp = \OTPHP\TOTP::create($secret); + $totp = \OTPHP\TOTP::create($secret); } else { - $otp = \OTPHP\TOTP::create(); - $secret = $otp->getSecret(); - $this->Session->write('totp_secret', $secret); // Store in session, this is to keep the same QR code even if the page refreshes. + $totp = \OTPHP\TOTP::create(); + $secret = $totp->getSecret(); + $this->Session->write('otp_secret', $secret); // Store in session, this is to keep the same QR code even if the page refreshes. } if ($this->request->is('post') && isset($this->request->data['User']['otp'])) { - $otp_now = $otp->now(); - if (trim($this->request->data['User']['otp']) == $otp_now) { + if ($totp->verify(trim($this->request->data['User']['otp']))) { // we know the user can generate TOTP tokens, save the new TOTP to the database $this->User->id = $user['User']['id']; $this->User->saveField('totp', $secret); - $this->_refreshAuth(); + $this->User->saveField('hotp_counter', 0); + $this->_refreshAuth(); $this->Flash->info(__('The OTP is correct and now active for your account.')); $fieldsDescrStr = 'User (' . $user['User']['id'] . '): ' . $user['User']['email']. ' TOTP token created'; $this->User->extralog($this->Auth->user(), "update", $fieldsDescrStr, ''); - $this->redirect(array('controller' => 'events', 'action'=> 'index')); + // redirect to a page that gives the next 50 HOTP + $this->redirect(array('controller' => 'users', 'action'=> 'hotp')); } else { $this->Flash->error(__("The OTP is incorrect or has expired.")); } @@ -1847,8 +1876,10 @@ class UsersController extends AppController new \BaconQrCode\Renderer\Image\SvgImageBackEnd() ); $writer = new \BaconQrCode\Writer($renderer); - $qrcode = $writer->writeString('otpauth://totp/' . Configure::read('MISP.org') . ' MISP (' . $user['User']['email'] . ')?secret=' . $secret); - $writer = preg_replace('/^.+\n/', '', $qrcode); // ignore first setLabel($user['User']['email']); + $totp->setIssuer(Configure::read('MISP.org') . ' MISP'); + $qrcode = $writer->writeString($totp->getProvisioningUri()); + $qrcode = preg_replace('/^.+\n/', '', $qrcode); // ignore first set('qrcode', $qrcode); $this->set('secret', $secret); diff --git a/app/Model/AppModel.php b/app/Model/AppModel.php index 64e638a2f..97a5c1ce3 100644 --- a/app/Model/AppModel.php +++ b/app/Model/AppModel.php @@ -1954,6 +1954,7 @@ class AppModel extends Model $sqlArray[] = "UPDATE `over_correlating_values` SET `value` = LOWER(`value`) COLLATE utf8mb4_unicode_ci;"; case 110: $sqlArray[] = "ALTER TABLE `users` ADD `totp` varchar(255) DEFAULT NULL;"; + $sqlArray[] = "ALTER TABLE `users` ADD `hotp_counter` int(11) DEFAULT NULL;"; break; case 'fixNonEmptySharingGroupID': $sqlArray[] = 'UPDATE `events` SET `sharing_group_id` = 0 WHERE `distribution` != 4;'; diff --git a/app/Model/Server.php b/app/Model/Server.php index d2c843b8b..0c5d4ba59 100644 --- a/app/Model/Server.php +++ b/app/Model/Server.php @@ -2154,7 +2154,7 @@ class Server extends AppModel return true; } - public function otpBeforeHook($setting, $value) + public function email_otpBeforeHook($setting, $value) { if ($value && !empty(Configure::read('MISP.disable_emailing'))) { return __('Emailing is currently disabled. Enabling OTP without e-mailing being configured would lock all users out.'); @@ -2162,7 +2162,7 @@ class Server extends AppModel return true; } - public function totpBeforeHook($setting, $value) + public function otpBeforeHook($setting, $value) { if ($value && (!class_exists('\OTPHP\TOTP') || !class_exists('\BaconQrCode\Writer'))) { return __('The TOTP and QR code generation libraries are not installed. Enabling OTP without those libraries installed would lock all users out.'); @@ -6397,12 +6397,12 @@ class Server extends AppModel 'type' => 'boolean', 'null' => true, ], - 'totp_required' => array( + 'otp_required' => array( 'level' => 2, - 'description' => __('Require authentication with TOTP. Users that do not have TOTP configured will be forced to create a token at first login. You cannot use it in combination with external authentication plugins.'), + 'description' => __('Require authentication with OTP. Users that do not have (T/H)OTP configured will be forced to create a token at first login. You cannot use it in combination with external authentication plugins.'), 'value' => false, 'test' => 'testBool', - 'beforeHook' => 'totpBeforeHook', + 'beforeHook' => 'otpBeforeHook', 'type' => 'boolean', 'null' => true ), @@ -6411,7 +6411,7 @@ class Server extends AppModel 'description' => __('Enable two step authentication with a OTP sent by email. Requires e-mailing to be enabled. Warning: You cannot use it in combination with external authentication plugins.'), 'value' => false, 'test' => 'testBool', - 'beforeHook' => 'otpBeforeHook', + 'beforeHook' => 'email_otpBeforeHook', 'type' => 'boolean', 'null' => true ), diff --git a/app/View/Users/hotp.ctp b/app/View/Users/hotp.ctp new file mode 100644 index 000000000..f2c5a607d --- /dev/null +++ b/app/View/Users/hotp.ctp @@ -0,0 +1,24 @@ +
+

+

Make sure you print these out.');?>

+ 
 $value) {
+      if ($key < 10) print(" ");
+      print("$key: $value");
+      if ($i == 5) {
+        print("\n");
+        $i = 1;
+      } else {
+        print("    ");
+        $i++;
+      }
+
+    }
+    ?>
+
+
+element('/genericElements/SideMenu/side_menu', array('menuList' => 'globalActions', 'menuItem' => 'view')); + diff --git a/app/View/Users/totp.ctp b/app/View/Users/otp.ctp similarity index 65% rename from app/View/Users/totp.ctp rename to app/View/Users/otp.ctp index 8d84c42ec..eb44dd5f4 100644 --- a/app/View/Users/totp.ctp +++ b/app/View/Users/otp.ctp @@ -2,11 +2,13 @@
-

+

element('/genericElements/Form/genericForm', array( "form" => $this->Form, "data" => array( @@ -14,13 +16,13 @@ echo $this->element('/genericElements/Form/genericForm', array( "fields" => array( array( "field" => "otp", - "label" => __("One Time Password"), + "label" => $label, "type" => "text", "placeholder" => __("Enter your OTP here"), - ), + ) ), "submit" => array ( - "action" => "totp", + "action" => "otp", ), ))); ?> diff --git a/app/View/Users/view.ctp b/app/View/Users/view.ctp index a1310976e..c8348620c 100755 --- a/app/View/Users/view.ctp +++ b/app/View/Users/view.ctp @@ -24,7 +24,8 @@ $boolean = sprintf( $isTotp ? 'label label-success label-padding' : 'label label-important label-padding', $isTotp ? __('Yes') : __('No')); $totpHtml = $boolean; -$totpHtml .= ($isTotp ? '' : $this->Html->link(__('Generate'), array('action' => 'totp_new', $user['User']['id']))); +$totpHtml .= (!$isTotp && !$admin_view ? $this->Html->link(__('Generate'), array('action' => 'totp_new')) : ''); +$totpHtml .= ($isTotp && !$admin_view ? $this->Html->link(__('View paper tokens'), array('action' => 'hotp', $user['User']['id'])): ''); $totpHtml .= ($admin_view && $isTotp ? ' ' . $this->Form->postLink(__('Delete'), array('action' => 'totp_delete', $user['User']['id'])) : ''); From 3097dc106e71e5122888641d52cbca318ac09100 Mon Sep 17 00:00:00 2001 From: iglocska Date: Wed, 31 May 2023 15:11:51 +0200 Subject: [PATCH 11/13] fix: [totp field check] causes exception if update is not executed yet and the field isn't added - without the login the update doesn't execute - chicken & egg issue --- app/Controller/AppController.php | 2 +- app/Controller/UsersController.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/Controller/AppController.php b/app/Controller/AppController.php index 9315329cf..a96a98617 100755 --- a/app/Controller/AppController.php +++ b/app/Controller/AppController.php @@ -602,7 +602,7 @@ class AppController extends Controller } // Check if user must create TOTP secret, force them to be on that page as long as needed. - if (!$user['totp'] && Configure::read('Security.otp_required') && !$this->_isControllerAction(['users' => ['terms', 'change_pw', 'logout', 'login', 'totp_new']])) { // TOTP is mandatory for users, prevent login until the user has configured their TOTP + if (empty($user['totp']) && Configure::read('Security.otp_required') && !$this->_isControllerAction(['users' => ['terms', 'change_pw', 'logout', 'login', 'totp_new']])) { // TOTP is mandatory for users, prevent login until the user has configured their TOTP $this->redirect(array('controller' => 'users', 'action' => 'totp_new', 'admin' => false)); return false; } diff --git a/app/Controller/UsersController.php b/app/Controller/UsersController.php index 1ca4adc8a..655faa0a7 100644 --- a/app/Controller/UsersController.php +++ b/app/Controller/UsersController.php @@ -1197,7 +1197,7 @@ class UsersController extends AppController $this->Auth->constructAuthenticate(); } // user has TOTP token, check creds and redirect to TOTP validation - if ($unauth_user['User']['totp'] && !$unauth_user['User']['disabled'] && class_exists('\OTPHP\TOTP')) { + if (!empty($unauth_user['User']['totp']) && !$unauth_user['User']['disabled'] && class_exists('\OTPHP\TOTP')) { $user = $this->Auth->identify($this->request, $this->response); if ($user && !$user['disabled']) { $this->Session->write('otp_user', $user); From 8d596784e3509057a04353c4496a3bb548e0dc49 Mon Sep 17 00:00:00 2001 From: iglocska Date: Wed, 31 May 2023 15:12:54 +0200 Subject: [PATCH 12/13] fix: [privileges] only site admins can remove totp for a user - leads to potential privilege check circumvention otherwise (org admin deleting site admin's totp key) - also, removal should be a nuclear option --- app/Controller/Component/ACLComponent.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/Controller/Component/ACLComponent.php b/app/Controller/Component/ACLComponent.php index e4ab9083f..e1c9dc8a6 100644 --- a/app/Controller/Component/ACLComponent.php +++ b/app/Controller/Component/ACLComponent.php @@ -748,7 +748,7 @@ class ACLComponent extends Component 'otp' => array('*'), 'hotp' => array('*'), 'totp_new' => array('*'), - 'totp_delete' => array('perm_admin'), + 'totp_delete' => array('perm_site_admin'), 'searchGpgKey' => array('*'), 'fetchGpgKey' => array('*'), 'histogram' => array('*'), From acf3e41e9de047feee80fd79cc4bd2fe17808a3a Mon Sep 17 00:00:00 2001 From: iglocska Date: Wed, 31 May 2023 15:13:56 +0200 Subject: [PATCH 13/13] fix: [removing totp] was a postlink, causing unprompted removal - use a GET to display a modal with the prompt --- app/View/Users/view.ctp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/app/View/Users/view.ctp b/app/View/Users/view.ctp index c8348620c..cf3b2f546 100755 --- a/app/View/Users/view.ctp +++ b/app/View/Users/view.ctp @@ -26,9 +26,15 @@ $isTotp ? __('Yes') : __('No')); $totpHtml = $boolean; $totpHtml .= (!$isTotp && !$admin_view ? $this->Html->link(__('Generate'), array('action' => 'totp_new')) : ''); $totpHtml .= ($isTotp && !$admin_view ? $this->Html->link(__('View paper tokens'), array('action' => 'hotp', $user['User']['id'])): ''); -$totpHtml .= ($admin_view && $isTotp ? ' ' . $this->Form->postLink(__('Delete'), array('action' => 'totp_delete', $user['User']['id'])) : ''); - +if ($admin_view && $isSiteAdmin && $isTotp) { + $totpHtml .= sprintf( + '%s', + h($baseurl), + h($user['User']['id']), + __('Delete') + ); +} $table_data = [ array('key' => __('ID'), 'value' => $user['User']['id']), array(