CVE-2022-2820: Session Fixation (CWE-384)
value: Session Fixation in GitHub repository namelessmc/nameless prior to v2.0.2.


CWE description: Authenticating a user, or otherwise establishing a new user session, without invalidating any existing session identifier gives an attacker the opportunity to steal authenticated sessions.
CVSS v3 base score: 8.2
Published: 2022-08-15T11:21Z
Repository: https://github.com/namelessmc/nameless
Language: PHP
Files changed: core/classes/Core/User.php, core/init.php, core/installation/includes/upgrade_perform.php, core/migrations/20220807142446_add_user_session_activity_columns.php, core/migrations/20220807153708_delete_admin_session_table.php, modules/Core/hooks/DeleteUserHook.php, modules/Core/pages/user/settings.php
Commit date: 2022-08-12 22:29:09 -0600
Commit message: Rework user sessions system (#3000)

Co-authored-by: samerton <samerton@users.noreply.github.com>
Diff: diff --git a/core/classes/Core/User.php b/core/classes/Core/User.php
index ee4c890eb..8db27c050 100644
--- a/core/classes/Core/User.php
+++ b/core/classes/Core/User.php
@@ -4,11 +4,11 @@
  *
  * @package NamelessMC\Core
  * @author Samerton
  * @author Partydragen
  * @author Aberdeener
- * @version 2.0.0-pr13
+ * @version 2.0.2
  * @license MIT
  */
 class User {
 
     private static array $_user_cache = [];
@@ -71,18 +71,18 @@ class User {
         $this->_cookieName = Config::get('remember.cookie_name');
         $this->_admSessionName = Config::get('session.admin_name');
 
         if ($user === null) {
             if (Session::exists($this->_sessionName)) {
-                $user = Session::get($this->_sessionName);
-                if ($this->find($user, $field)) {
+                $hash = Session::get($this->_sessionName);
+                if ($this->find($hash, 'hash')) {
                     $this->_isLoggedIn = true;
                 }
             }
             if (Session::exists($this->_admSessionName)) {
-                $user = Session::get($this->_admSessionName);
-                if ($user == $this->data()->id && $this->find($user, $field)) {
+                $hash = Session::get($this->_admSessionName);
+                if ($this->find($hash, 'hash')) {
                     $this->_isAdmLoggedIn = true;
                 }
             }
         } else {
             $this->find($user, $field);
@@ -105,11 +105,15 @@ class User {
                 $this->_data = $cache['data'];
                 $this->_groups = $cache['groups'];
                 return true;
             }
 
-            $data = $this->_db->get('users', [$field, $value]);
+            if ($field != 'hash') {
+                $data = $this->_db->get('users', [$field, $value]);
+            } else {
+                $data = $this->_db->query('SELECT nl2_users.* FROM nl2_users LEFT JOIN nl2_users_session ON nl2_users.id = user_id WHERE hash = ? AND nl2_users_session.active = 1', [$value]);
+            }
 
             if ($data->count()) {
                 $this->_data = new UserData($data->first());
 
                 // Get user groups
@@ -295,43 +299,41 @@ class User {
      * Log the user in.
      *
      * @param string|null $username Their username (or email, depending on $method).
      * @param string|null $password Their password.
      * @param bool $remember Whether to keep them logged in or not.
-     * @param string $method What column to check for their details in. Can be either `username` or `email`.
+     * @param string $method What column to check for their details in. Can be either `username` or `email` or `oauth`.
      *
      * @return bool True/false on success or failure respectfully.
      */
     public function login(?string $username = null, ?string $password = null, bool $remember = false, string $method = 'email'): bool {
         return $this->_commonLogin($username, $password, $remember, $method, false);
     }
 
     private function _commonLogin(?string $username, ?string $password, bool $remember, string $method, bool $is_admin): bool {
         $sessionName = $is_admin ? $this->_admSessionName : $this->_sessionName;
-        if (!$username && !$password && $this->exists()) {
-            Session::put($sessionName, $this->data()->id);
+        if (!$username && $method == 'hash' && $this->exists()) {
+            // Logged in using hash from cookie
+            Session::put($sessionName, $password);
             if (!$is_admin) {
                 $this->_isLoggedIn = true;
             }
         } else if ($this->checkCredentials($username, $password, $method) === true) {
             // Valid credentials
-            Session::put($sessionName, $this->data()->id);
+            $hash = SecureRandom::alphanumeric();
+
+            $this->_db->insert('users_session', [
+                'user_id' => $this->data()->id,
+                'hash' => $hash,
+                'remember_me' => $remember,
+                'active' => 1,
+                'login_method' => $is_admin ? 'admin' : $method
+            ]);
 
-            if ($remember) {
-                $hash = SecureRandom::alphanumeric();
-                $table = $is_admin ? 'users_admin_session' : 'users_session';
-                $hashCheck = $this->_db->get($table, ['user_id', $this->data()->id]);
-
-                if (!$hashCheck->count()) {
-                    $this->_db->insert($table, [
-                        'user_id' => $this->data()->id,
-                        'hash' => $hash
-                    ]);
-                } else {
-                    $hash = $hashCheck->first()->hash;
-                }
+            Session::put($sessionName, $hash);
 
+            if ($remember) {
                 $expiry = $is_admin ? 3600 : Config::get('remember.cookie_expiry');
                 $cookieName = $is_admin ? ($this->_cookieName . '_adm') : $this->_cookieName;
                 Cookie::put($cookieName, $hash, $expiry, HttpUtils::getProtocol() === 'https', true);
             }
 
@@ -530,26 +532,40 @@ class User {
         }
 
         return false;
     }
 
+    /**
+     * Log the user out from all other sessions.
+     */
+    public function logoutAllOtherSessions(): void {
+        DB::getInstance()->query('UPDATE nl2_users_session SET `active` = 0 WHERE user_id = ? AND hash != ?', [
+            $this->data()->id,
+            Session::get(Config::get('session.session_name'))
+        ]);
+    }
+
     /**
      * Log the user out.
      * Deletes their cookies, sessions and database session entry.
      */
     public function logout(): void {
-        $this->_db->delete('users_session', ['user_id', $this->data()->id]);
+        $this->_db->update('users_session', [['user_id', $this->data()->id], ['hash', Session::get($this->_sessionName)]], [
+            'active' => 0
+        ]);
 
         Session::delete($this->_sessionName);
         Cookie::delete($this->_cookieName);
     }
 
     /**
      * Process logout if user is admin
      */
     public function admLogout(): void {
-        $this->_db->delete('users_admin_session', ['user_id', $this->data()->id]);
+        $this->_db->update('users_session', [['user_id', $this->data()->id], ['hash', Session::get($this->_admSessionName)]], [
+            'active' => 0
+        ]);
 
         Session::delete($this->_admSessionName);
         Cookie::delete($this->_cookieName . '_adm');
     }
 
diff --git a/core/init.php b/core/init.php
index edc98df05..5bcf48c67 100644
--- a/core/init.php
+++ b/core/init.php
@@ -1,10 +1,10 @@
 <?php
 /*
  *  Made by Samerton
  *  https://github.com/NamelessMC/Nameless/
- *  NamelessMC version 2.0.0-pr9
+ *  NamelessMC version 2.0.2
  *
  *  License: MIT
  *
  *  Initialisation file
  */
@@ -130,15 +130,15 @@ if ($page != 'install') {
     // User initialisation
     $user = new User();
     // Do they need logging in (checked remember me)?
     if (Cookie::exists(Config::get('remember.cookie_name')) && !Session::exists(Config::get('session.session_name'))) {
         $hash = Cookie::get(Config::get('remember.cookie_name'));
-        $hashCheck = DB::getInstance()->get('users_session', ['hash', $hash]);
+        $hashCheck = DB::getInstance()->get('users_session', [['hash', $hash], ['active', true]]);
 
         if ($hashCheck->count()) {
             $user = new User($hashCheck->first()->user_id);
-            $user->login();
+            $user->login(null, $hash, true, 'hash');
         }
     }
 
     // Check if we're in a subdirectory
     if (isset($directories)) {
diff --git a/core/installation/includes/upgrade_perform.php b/core/installation/includes/upgrade_perform.php
index 88624dd73..deface8ec 100644
--- a/core/installation/includes/upgrade_perform.php
+++ b/core/installation/includes/upgrade_perform.php
@@ -519,47 +519,10 @@ try {
     }
 } catch (Exception $e) {
     $errors[] = 'Unable to convert users: ' . $e->getMessage();
 }
 
-// User admin session -> user profile wall replies
-// User admin sessions
-try {
-    $old = $conn->get('nl1_users_admin_session', ['id', '<>', 0]);
-    if ($old->count()) {
-        $old = $old->results();
-
-        foreach ($old as $item) {
-            DB::getInstance()->insert('users_admin_session', [
-                'id' => $item->id,
-                'user_id' => $item->user_id,
-                'hash' => $item->hash
-            ]);
-        }
-    }
-} catch (Exception $e) {
-    $errors[] = 'Unable to convert user admin sessions: ' . $e->getMessage();
-}
-
-// User sessions
-try {
-    $old = $conn->get('nl1_users_session', ['id', '<>', 0]);
-    if ($old->count()) {
-        $old = $old->results();
-
-        foreach ($old as $item) {
-            DB::getInstance()->insert('users_session', [
-                'id' => $item->id,
-                'user_id' => $item->user_id,
-                'hash' => $item->hash
-            ]);
-        }
-    }
-} catch (Exception $e) {
-    $errors[] = 'Unable to convert user sessions: ' . $e->getMessage();
-}
-
 // Username history
 try {
     $old = $conn->get('nl1_users_username_history', ['id', '<>', 0]);
     if ($old->count()) {
         $old = $old->results();
diff --git a/core/migrations/20220807142446_add_user_session_activity_columns.php b/core/migrations/20220807142446_add_user_session_activity_columns.php
new file mode 100644
index 000000000..5a2e0a08f
--- /dev/null
+++ b/core/migrations/20220807142446_add_user_session_activity_columns.php
@@ -0,0 +1,22 @@
+<?php
+declare(strict_types=1);
+
+use Phinx\Migration\AbstractMigration;
+
+final class AddUserSessionActivityColumns extends AbstractMigration
+{
+    public function change(): void
+    {
+        $table = $this->table('nl2_users_session');
+        $table->addColumn('remember_me', 'boolean', ['default' => false]);
+        $table->addColumn('active', 'boolean', ['default' => false]);
+        $table->addColumn('device_name', 'string', ['length' => 256, 'null' => true, 'default' => null]);
+        $table->addColumn('last_seen', 'integer', ['length' => 11, 'null' => true, 'default' => null]);
+        $table->addColumn('login_method', 'string', ['length' => 32]);
+        $table->addIndex('hash', ['unique' => true]);
+        $table->update();
+
+        // Remove old data
+        $table->truncate();
+    }
+}
diff --git a/core/migrations/20220807153708_delete_admin_session_table.php b/core/migrations/20220807153708_delete_admin_session_table.php
new file mode 100644
index 000000000..4a7524c3f
--- /dev/null
+++ b/core/migrations/20220807153708_delete_admin_session_table.php
@@ -0,0 +1,14 @@
+<?php
+declare(strict_types=1);
+
+use Phinx\Migration\AbstractMigration;
+
+final class DeleteAdminSessionTable extends AbstractMigration
+{
+    public function change(): void
+    {
+        $table = $this->table('nl2_users_admin_session');
+        $table->drop();
+        $table->update();
+    }
+}
diff --git a/modules/Core/hooks/DeleteUserHook.php b/modules/Core/hooks/DeleteUserHook.php
index f8d76beeb..dee829e17 100644
--- a/modules/Core/hooks/DeleteUserHook.php
+++ b/modules/Core/hooks/DeleteUserHook.php
@@ -52,11 +52,10 @@ class DeleteUserHook {
             // Reports
             $db->delete('reports', ['reporter_id', $params['user_id']]);
             $db->delete('reports_comments', ['commenter_id', $params['user_id']]);
 
             // User sessions
-            $db->delete('users_admin_session', ['user_id', $params['user_id']]);
             $db->delete('users_session', ['user_id', $params['user_id']]);
 
             // Profile fields
             $db->delete('users_profile_fields', ['user_id', $params['user_id']]);
 
diff --git a/modules/Core/pages/user/settings.php b/modules/Core/pages/user/settings.php
index 3d0097e70..68d6c00b6 100644
--- a/modules/Core/pages/user/settings.php
+++ b/modules/Core/pages/user/settings.php
@@ -1,10 +1,10 @@
 <?php
 /*
  *  Made by Samerton
  *  https://github.com/NamelessMC/Nameless/
- *  NamelessMC version 2.0.0-pr13
+ *  NamelessMC version 2.0.2
  *
  *  License: MIT
  *
  *  UserCP settings
  */
@@ -78,10 +78,14 @@ if (isset($_GET['do'])) {
                             $user->update([
                                 'tfa_complete' => true,
                                 'tfa_enabled' => true,
                                 'tfa_type' => 1
                             ]);
+
+                            // Logout all other sessions for this user
+                            $user->logoutAllOtherSessions();
+
                             Session::delete('force_tfa_alert');
                             Session::flash('tfa_success', $language->get('user', 'tfa_successful'));
                             Redirect::to(URL::build('/user/settings'));
                         }
 
@@ -362,10 +366,13 @@ if (isset($_GET['do'])) {
                             $user->update([
                                 'password' => $new_password,
                                 'pass_method' => 'default'
                             ]);
 
+                            // Logout all other sessions for this user
+                            $user->logoutAllOtherSessions();
+
                             $success = $language->get('user', 'password_changed_successfully');
 
                         } else {
                             // Invalid current password
                             Session::flash('settings_error', $language->get('user', 'incorrect_password'));
@@ -408,11 +415,11 @@ if (isset($_GET['do'])) {
                                 $password = Input::get('password');
                                 if ($user->checkCredentials($user->data()->username, $password, 'username')) {
 
                                     // Update email
                                     $user->update([
-                                        'email' => Output::getClean($_POST['email'])
+                                        'email' => $_POST['email']
                                     ]);
 
                                     Session::flash('settings_success', $language->get('user', 'email_changed_successfully'));
                                     Redirect::to(URL::build('/user/settings'));
                                 }

Vulnerable code:  * @version 2.0.0-pr13
                $user = Session::get($this->_sessionName);
                if ($this->find($user, $field)) {
                $user = Session::get($this->_admSessionName);
                if ($user == $this->data()->id && $this->find($user, $field)) {
            $data = $this->_db->get('users', [$field, $value]);
     * @param string $method What column to check for their details in. Can be either `username` or `email`.
        if (!$username && !$password && $this->exists()) {
            Session::put($sessionName, $this->data()->id);
            Session::put($sessionName, $this->data()->id);
            if ($remember) {
                $hash = SecureRandom::alphanumeric();
                $table = $is_admin ? 'users_admin_session' : 'users_session';
                $hashCheck = $this->_db->get($table, ['user_id', $this->data()->id]);

                if (!$hashCheck->count()) {
                    $this->_db->insert($table, [
                        'user_id' => $this->data()->id,
                        'hash' => $hash
                    ]);
                } else {
                    $hash = $hashCheck->first()->hash;
                }
        $this->_db->delete('users_session', ['user_id', $this->data()->id]);
        $this->_db->delete('users_admin_session', ['user_id', $this->data()->id]);
 *  NamelessMC version 2.0.0-pr9
        $hashCheck = DB::getInstance()->get('users_session', ['hash', $hash]);
            $user->login();
// User admin session -> user profile wall replies
// User admin sessions
try {
    $old = $conn->get('nl1_users_admin_session', ['id', '<>', 0]);
    if ($old->count()) {
        $old = $old->results();

        foreach ($old as $item) {
            DB::getInstance()->insert('users_admin_session', [
                'id' => $item->id,
                'user_id' => $item->user_id,
                'hash' => $item->hash
            ]);
        }
    }
} catch (Exception $e) {
    $errors[] = 'Unable to convert user admin sessions: ' . $e->getMessage();
}

// User sessions
try {
    $old = $conn->get('nl1_users_session', ['id', '<>', 0]);
    if ($old->count()) {
        $old = $old->results();

        foreach ($old as $item) {
            DB::getInstance()->insert('users_session', [
                'id' => $item->id,
                'user_id' => $item->user_id,
                'hash' => $item->hash
            ]);
        }
    }
} catch (Exception $e) {
    $errors[] = 'Unable to convert user sessions: ' . $e->getMessage();
}

            $db->delete('users_admin_session', ['user_id', $params['user_id']]);
 *  NamelessMC version 2.0.0-pr13
                                        'email' => Output::getClean($_POST['email'])
Fixed code:  * @version 2.0.2
                $hash = Session::get($this->_sessionName);
                if ($this->find($hash, 'hash')) {
                $hash = Session::get($this->_admSessionName);
                if ($this->find($hash, 'hash')) {
            if ($field != 'hash') {
                $data = $this->_db->get('users', [$field, $value]);
            } else {
                $data = $this->_db->query('SELECT nl2_users.* FROM nl2_users LEFT JOIN nl2_users_session ON nl2_users.id = user_id WHERE hash = ? AND nl2_users_session.active = 1', [$value]);
            }
     * @param string $method What column to check for their details in. Can be either `username` or `email` or `oauth`.
        if (!$username && $method == 'hash' && $this->exists()) {
            // Logged in using hash from cookie
            Session::put($sessionName, $password);
            $hash = SecureRandom::alphanumeric();

            $this->_db->insert('users_session', [
                'user_id' => $this->data()->id,
                'hash' => $hash,
                'remember_me' => $remember,
                'active' => 1,
                'login_method' => $is_admin ? 'admin' : $method
            ]);
            Session::put($sessionName, $hash);
            if ($remember) {
    /**
     * Log the user out from all other sessions.
     */
    public function logoutAllOtherSessions(): void {
        DB::getInstance()->query('UPDATE nl2_users_session SET `active` = 0 WHERE user_id = ? AND hash != ?', [
            $this->data()->id,
            Session::get(Config::get('session.session_name'))
        ]);
    }

        $this->_db->update('users_session', [['user_id', $this->data()->id], ['hash', Session::get($this->_sessionName)]], [
            'active' => 0
        ]);
        $this->_db->update('users_session', [['user_id', $this->data()->id], ['hash', Session::get($this->_admSessionName)]], [
            'active' => 0
        ]);
 *  NamelessMC version 2.0.2
        $hashCheck = DB::getInstance()->get('users_session', [['hash', $hash], ['active', true]]);
            $user->login(null, $hash, true, 'hash');
<?php
declare(strict_types=1);

use Phinx\Migration\AbstractMigration;

final class AddUserSessionActivityColumns extends AbstractMigration
{
    public function change(): void
    {
        $table = $this->table('nl2_users_session');
        $table->addColumn('remember_me', 'boolean', ['default' => false]);
        $table->addColumn('active', 'boolean', ['default' => false]);
        $table->addColumn('device_name', 'string', ['length' => 256, 'null' => true, 'default' => null]);
        $table->addColumn('last_seen', 'integer', ['length' => 11, 'null' => true, 'default' => null]);
        $table->addColumn('login_method', 'string', ['length' => 32]);
        $table->addIndex('hash', ['unique' => true]);
        $table->update();

        // Remove old data
        $table->truncate();
    }
}
<?php
declare(strict_types=1);

use Phinx\Migration\AbstractMigration;

final class DeleteAdminSessionTable extends AbstractMigration
{
    public function change(): void
    {
        $table = $this->table('nl2_users_admin_session');
        $table->drop();
        $table->update();
    }
}
 *  NamelessMC version 2.0.2

                            // Logout all other sessions for this user
                            $user->logoutAllOtherSessions();

                            // Logout all other sessions for this user
                            $user->logoutAllOtherSessions();

                                        'email' => $_POST['email']