CVE-2020-8890: Time-of-check Time-of-use (TOCTOU) Race Condition (CWE-367)
value: An issue was discovered in MISP before 2.4.121. It mishandled time skew (between the machine hosting the web server and the machine hosting the database) when trying to block a brute-force series of invalid requests.
CWE description: The product checks the state of a resource before using that resource, but the resource's state can change between the check and the use in a way that invalidates the results of the check. This can cause the product to perform invalid actions when the resource is in an unexpected state.
CVSS v3 base score: 5.9
Published: 2020-02-12T00:15Z
Repository: https://github.com/MISP/MISP
Language: PHP
Files changed: app/Controller/UsersController.php, app/Model/Bruteforce.php
Commit date: 2020-02-10 11:41:54 +0100
Commit message: fix: [security] Further fixes to the bruteforce handling

- resolved a potential failure of the subsystem when the MySQL and the webserver time settings are diverged
  - as reported by Dawid Czarnecki
- several tightenings of the checks to avoid potential foul play

Diff: diff --git a/app/Controller/UsersController.php b/app/Controller/UsersController.php
index c0fffba6caf..40fa352b174 100644
--- a/app/Controller/UsersController.php
+++ b/app/Controller/UsersController.php
@@ -1064,10 +1064,11 @@ class UsersController extends AppController
     {
         if ($this->request->is('post') || $this->request->is('put')) {
             $this->Bruteforce = ClassRegistry::init('Bruteforce');
             if (!empty($this->request->data['User']['email'])) {
                 if ($this->Bruteforce->isBlacklisted($_SERVER['REMOTE_ADDR'], $this->request->data['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 ' . Configure::read('SecureAuth.expire') . ' seconds and try again.');
                 }
             }
             // Check the length of the user's authkey
             $userPass = $this->User->find('first', array(
@@ -1114,11 +1115,11 @@ class UsersController extends AppController
             // don't display authError before first login attempt
             if (str_replace("//", "/", $this->webroot . $this->Session->read('Auth.redirect')) == $this->webroot && $this->Session->read('Message.auth.message') == $this->Auth->authError) {
                 $this->Session->delete('Message.auth');
             }
             // don't display "invalid user" before first login attempt
-            if ($this->request->is('post')) {
+            if ($this->request->is('post') || $this->request->is('put')) {
                 $this->Flash->error(__('Invalid username or password, try again'));
                 if (isset($this->request->data['User']['email'])) {
                     $this->Bruteforce->insert($_SERVER['REMOTE_ADDR'], $this->request->data['User']['email']);
                 }
             }
diff --git a/app/Model/Bruteforce.php b/app/Model/Bruteforce.php
index c1e8356f7e1..9f9463c7abe 100644
--- a/app/Model/Bruteforce.php
+++ b/app/Model/Bruteforce.php
@@ -7,21 +7,23 @@ class Bruteforce extends AppModel
 {
     public function insert($ip, $username)
     {
         $this->Log = ClassRegistry::init('Log');
         $this->Log->create();
-        $expire = time() + Configure::read('SecureAuth.expire');
+        $expire = Configure::check('SecureAuth.expire') ? Configure::read('SecureAuth.expire') : 300;
+        $amount = Configure::check('SecureAuth.amount') ? Configure::read('SecureAuth.amount') : 5;
+        $expire = time() + $expire;
         $expire = date('Y-m-d H:i:s', $expire);
         $bruteforceEntry = array(
             'ip' => $ip,
-            'username' => $username,
+            'username' => trim(strtolower($username)),
             'expire' => $expire
         );
         $this->save($bruteforceEntry);
         $title = 'Failed login attempt using username ' . $username . ' from IP: ' . $_SERVER['REMOTE_ADDR'] . '.';
         if ($this->isBlacklisted($ip, $username)) {
-            $title .= 'This has tripped the bruteforce protection after  ' . Configure::read('SecureAuth.amount') . ' failed attempts. The user is now blacklisted for ' . Configure::read('SecureAuth.expire') . ' seconds.';
+            $title .= 'This has tripped the bruteforce protection after  ' . $amount . ' failed attempts. The user is now blacklisted for ' . $expire . ' seconds.';
         }
         $log = array(
                 'org' => 'SYSTEM',
                 'model' => 'User',
                 'model_id' => 0,
@@ -34,14 +36,15 @@ class Bruteforce extends AppModel
 
     public function clean()
     {
         $dataSourceConfig = ConnectionManager::getDataSource('default')->config;
         $dataSource = $dataSourceConfig['datasource'];
+        $expire = date('Y-m-d H:i:s', time());
         if ($dataSource == 'Database/Mysql') {
-            $sql = 'DELETE FROM bruteforces WHERE `expire` <= NOW();';
+            $sql = 'DELETE FROM bruteforces WHERE `expire` <= "' . $expire . '";';
         } elseif ($dataSource == 'Database/Postgres') {
-            $sql = 'DELETE FROM bruteforces WHERE expire <= NOW();';
+            $sql = 'DELETE FROM bruteforces WHERE expire <= "' . $expire . '";';
         }
         $this->query($sql);
     }
 
     public function isBlacklisted($ip, $username)

Vulnerable code:             if ($this->request->is('post')) {
        $expire = time() + Configure::read('SecureAuth.expire');
            'username' => $username,
            $title .= 'This has tripped the bruteforce protection after  ' . Configure::read('SecureAuth.amount') . ' failed attempts. The user is now blacklisted for ' . Configure::read('SecureAuth.expire') . ' seconds.';
            $sql = 'DELETE FROM bruteforces WHERE `expire` <= NOW();';
            $sql = 'DELETE FROM bruteforces WHERE expire <= NOW();';
Fixed code:                     $expire = Configure::check('SecureAuth.expire') ? Configure::read('SecureAuth.expire') : 300;
            if ($this->request->is('post') || $this->request->is('put')) {
        $expire = Configure::check('SecureAuth.expire') ? Configure::read('SecureAuth.expire') : 300;
        $amount = Configure::check('SecureAuth.amount') ? Configure::read('SecureAuth.amount') : 5;
        $expire = time() + $expire;
            'username' => trim(strtolower($username)),
            $title .= 'This has tripped the bruteforce protection after  ' . $amount . ' failed attempts. The user is now blacklisted for ' . $expire . ' seconds.';
        $expire = date('Y-m-d H:i:s', time());
            $sql = 'DELETE FROM bruteforces WHERE `expire` <= "' . $expire . '";';
            $sql = 'DELETE FROM bruteforces WHERE expire <= "' . $expire . '";';