CVE-2023-4005: Insufficient Session Expiration (CWE-613)
value: Insufficient Session Expiration in GitHub repository fossbilling/fossbilling prior to 0.5.5.
CWE description: According to WASC, "Insufficient Session Expiration is when a web site permits an attacker to reuse old session credentials or session IDs for authorization."
CVSS v3 base score: 9.8
Published: 2023-07-31T01:15Z
Repository: https://github.com/fossbilling/fossbilling
Language: PHP
Files changed: src/modules/Client/Api/Admin.php, src/modules/Profile/Api/Admin.php, src/modules/Profile/Api/Client.php, src/modules/Profile/Service.php, src/modules/Staff/Service.php, tests/modules/Client/Api/AdminTest.php, tests/modules/Staff/ServiceTest.php
Commit date: 2023-07-20 13:23:44 +0100
Commit message: Invalidate existing sessions during PW reset (#1435)


Diff: diff --git a/src/modules/Client/Api/Admin.php b/src/modules/Client/Api/Admin.php
index 4ec352efe..2d83add82 100644
--- a/src/modules/Client/Api/Admin.php
+++ b/src/modules/Client/Api/Admin.php
@@ -352,10 +352,13 @@ class Admin extends \Api_Abstract
 
         $client->pass = $this->di['password']->hashIt($data['password']);
         $client->updated_at = date('Y-m-d H:i:s');
         $this->di['db']->store($client);
 
+        $profileService = $this->di['mod_service']('profile');
+        $profileService->invalidateSessions('client', $data['id']);
+
         $this->di['events_manager']->fire(['event' => 'onAfterAdminClientPasswordChange', 'params' => ['id' => $client->id, 'password' => $data['password']]]);
 
         $this->di['logger']->info('Changed client #%s password', $client->id);
 
         return true;
diff --git a/src/modules/Profile/Api/Admin.php b/src/modules/Profile/Api/Admin.php
index 257624da4..af347e965 100644
--- a/src/modules/Profile/Api/Admin.php
+++ b/src/modules/Profile/Api/Admin.php
@@ -114,12 +114,28 @@ class Admin extends \Api_Abstract
             throw new \Exception('Passwords do not match');
         }
 
         $staff = $this->getIdentity();
 
-        if(!$this->di['password']->verify($data['current_password'], $staff->pass)) {
+        if (!$this->di['password']->verify($data['current_password'], $staff->pass)) {
             throw new \Exception('Current password incorrect');
         }
 
+        $this->getService()->invalidateSessions();
         return $this->getService()->changeAdminPassword($staff, $data['new_password']);
     }
+
+    /**
+     * Used to destroy / invalidate all existing sessions for a given user
+     * @param array $data An array with the options.
+     *                    The array can contain the following sub-keys:
+     *                    - string|null $data['type'] The user type (admin or staff) (optional).
+     *                    - id|null $data['id'] The session ID (optional).
+     * @return bool 
+     */
+    public function destroy_sessions(array $data): bool
+    {
+        $data['type'] ??= null;
+        $data['id'] ??= null;
+        return $this->getService()->invalidateSessions($data['type'], $data['id']);
+    }
 }
diff --git a/src/modules/Profile/Api/Client.php b/src/modules/Profile/Api/Client.php
index 0c355d192..f731f95d7 100644
--- a/src/modules/Profile/Api/Client.php
+++ b/src/modules/Profile/Api/Client.php
@@ -118,14 +118,15 @@ class Client extends \Api_Abstract
             throw new \Exception('Passwords do not match');
         }
 
         $client = $this->getIdentity();
 
-        if(!$this->di['password']->verify($data['current_password'], $client->pass)) {
+        if (!$this->di['password']->verify($data['current_password'], $client->pass)) {
             throw new \Exception('Current password incorrect');
         }
 
+        $this->getService()->invalidateSessions();
         return $this->getService()->changeClientPassword($client, $data['new_password']);
     }
 
     /**
      * Clear session and logout.
@@ -134,6 +135,15 @@ class Client extends \Api_Abstract
      */
     public function logout()
     {
         return $this->getService()->logoutClient();
     }
+
+    /**
+     * Used to destroy / invalidate all existing sessions for the current client
+     * @return bool 
+     */
+    public function destroy_sessions(array $data): bool
+    {
+        return $this->getService()->invalidateSessions();
+    }
 }
diff --git a/src/modules/Profile/Service.php b/src/modules/Profile/Service.php
index 2eb2077c3..3af6f3a63 100644
--- a/src/modules/Profile/Service.php
+++ b/src/modules/Profile/Service.php
@@ -220,6 +220,82 @@ class Service implements InjectionAwareInterface
         $this->di['session']->destroy('client');
         $this->di['logger']->info('Logged out');
 
         return true;
     }
+
+    public function invalidateSessions(?string $type = null, ?int $id = null): bool
+    {
+        if (empty($type)) {
+            $auth = new \Box_Authorization($this->di);
+            if ($auth->isAdminLoggedIn()) {
+                $type = 'admin';
+            } elseif ($auth->isClientLoggedIn()) {
+                $type = 'client';
+            } else {
+                throw new \Box_Exception("Unable to invalidate sessions, nobody is logged in");
+            }
+        }
+
+        if (empty($id)) {
+            switch ($type) {
+                case 'admin':
+                    $admin = $this->di['session']->get('admin');
+                    $id = $admin['id'];
+                    break;
+                case 'client':
+                    $id = $this->di['session']->get('client_id');
+                    break;
+            }
+        }
+
+        if ($type !== 'admin' && $type !== 'client') {
+            throw new \Box_Exception("Unable to invalidate sessions, an invalid type was used");
+        }
+
+        $sessions = $this->getSessions();
+        foreach ($sessions as $session) {
+            $this->deleteSessionIfMatching($session, $type, $id);
+        }
+
+        return true;
+    }
+
+    private function getSessions(): array
+    {
+        $query = 'SELECT * FROM session WHERE content IS NOT NULL AND content <> ""';
+        $sessions = $this->di['db']->getAll($query);
+        return $sessions;
+    }
+
+    private function deleteSessionIfMatching(array $session, string $type, int $id): void
+    {
+        // Decode the data for the current session and then verify it is for the selected type
+        $data = base64_decode($session['content']);
+        $stringStart = ($type === 'admin') ? 'admin|' : 'client_id|';
+        if (!str_starts_with($data, $stringStart)) {
+            return;
+        }
+
+        // Now we strip off the starting portion so we can unserialize the data
+        $data = str_replace($stringStart, '', $data);
+
+        // Finally, perform the check depending on what type of session we are looking for and trash it if it's a match
+        if ($type === 'admin') {
+            $dataArray = unserialize($data);
+            if ($dataArray['id'] === $id) {
+                $this->trashSessionByArray($session);
+            }
+        } else {
+            if (unserialize($data) === $id) {
+                $this->trashSessionByArray($session);
+            }
+        }
+    }
+
+    private function trashSessionByArray(array $session): void
+    {
+        $bean = $this->di['db']->dispense('session');
+        $bean->import($session);
+        $this->di['db']->trash($bean);
+    }
 }
diff --git a/src/modules/Staff/Service.php b/src/modules/Staff/Service.php
index d22aea5cf..0faf9129f 100644
--- a/src/modules/Staff/Service.php
+++ b/src/modules/Staff/Service.php
@@ -441,10 +441,13 @@ class Service implements InjectionAwareInterface
 
         $model->pass = $this->di['password']->hashIt($password);
         $model->updated_at = date('Y-m-d H:i:s');
         $this->di['db']->store($model);
 
+        $profileService = $this->di['mod_service']('profile');
+        $profileService->invalidateSessions('admin', $model->id);
+
         $this->di['events_manager']->fire(['event' => 'onAfterAdminStaffPasswordChange', 'params' => ['id' => $model->id]]);
 
         $this->di['logger']->info('Changed staff member %s password', $model->id);
 
         return true;
diff --git a/tests/modules/Client/Api/AdminTest.php b/tests/modules/Client/Api/AdminTest.php
index 3975ede72..572aec48a 100644
--- a/tests/modules/Client/Api/AdminTest.php
+++ b/tests/modules/Client/Api/AdminTest.php
@@ -447,21 +447,25 @@ class AdminTest extends \BBTestCase
         $passwordMock = $this->getMockBuilder('\Box_Password')->getMock();
         $passwordMock->expects($this->atLeastOnce())
             ->method('hashIt')
             ->with($data['password']);
 
+        $profileService = $this->getMockBuilder('\Box\Mod\Profile\Service')->getMock();
+
         $di                   = new \Pimple\Container();
         $di['db']             = $dbMock;
         $di['events_manager'] = $eventMock;
         $di['logger']         = new \Box_Log();
         $di['password']       = $passwordMock;
         $validatorMock = $this->getMockBuilder('\FOSSBilling\Validate')->disableOriginalConstructor()->getMock();
         $validatorMock->expects($this->atLeastOnce())
             ->method('checkRequiredParamsForArray')
             ->will($this->returnValue(null));
         $di['validator'] = $validatorMock;
-
+        $di['mod_service'] = $di->protect(function () use ($profileService) {
+            return $profileService;
+        });
 
         $admin_Client = new \Box\Mod\Client\Api\Admin();
         $admin_Client->setDi($di);
 
         $result = $admin_Client->change_password($data);
diff --git a/tests/modules/Staff/ServiceTest.php b/tests/modules/Staff/ServiceTest.php
index 8de72682b..3daf88ef7 100644
--- a/tests/modules/Staff/ServiceTest.php
+++ b/tests/modules/Staff/ServiceTest.php
@@ -1025,16 +1025,21 @@ class ServiceTest extends \BBTestCase
 
         $passwordMock = $this->getMockBuilder('\Box_Password')->getMock();
         $passwordMock->expects($this->atLeastOnce())
             ->method('hashIt')
             ->with($plainTextPassword);
+        
+        $profileService = $this->getMockBuilder('\Box\Mod\Profile\Service')->getMock();
 
         $di                   = new \Pimple\Container();
         $di['events_manager'] = $eventsMock;
         $di['logger']         = $logMock;
         $di['db']             = $dbMock;
         $di['password']       = $passwordMock;
+        $di['mod_service'] = $di->protect(function () use ($profileService) {
+            return $profileService;
+        });
 
         $service = new \Box\Mod\Staff\Service();
         $service->setDi($di);
 
         $result = $service->changePassword($adminModel, $plainTextPassword);

Vulnerable code:         if(!$this->di['password']->verify($data['current_password'], $staff->pass)) {
        if(!$this->di['password']->verify($data['current_password'], $client->pass)) {

Fixed code:         $profileService = $this->di['mod_service']('profile');
        $profileService->invalidateSessions('client', $data['id']);

        if (!$this->di['password']->verify($data['current_password'], $staff->pass)) {
        $this->getService()->invalidateSessions();

    /**
     * Used to destroy / invalidate all existing sessions for a given user
     * @param array $data An array with the options.
     *                    The array can contain the following sub-keys:
     *                    - string|null $data['type'] The user type (admin or staff) (optional).
     *                    - id|null $data['id'] The session ID (optional).
     * @return bool 
     */
    public function destroy_sessions(array $data): bool
    {
        $data['type'] ??= null;
        $data['id'] ??= null;
        return $this->getService()->invalidateSessions($data['type'], $data['id']);
    }
        if (!$this->di['password']->verify($data['current_password'], $client->pass)) {
        $this->getService()->invalidateSessions();

    /**
     * Used to destroy / invalidate all existing sessions for the current client
     * @return bool 
     */
    public function destroy_sessions(array $data): bool
    {
        return $this->getService()->invalidateSessions();
    }

    public function invalidateSessions(?string $type = null, ?int $id = null): bool
    {
        if (empty($type)) {
            $auth = new \Box_Authorization($this->di);
            if ($auth->isAdminLoggedIn()) {
                $type = 'admin';
            } elseif ($auth->isClientLoggedIn()) {
                $type = 'client';
            } else {
                throw new \Box_Exception("Unable to invalidate sessions, nobody is logged in");
            }
        }

        if (empty($id)) {
            switch ($type) {
                case 'admin':
                    $admin = $this->di['session']->get('admin');
                    $id = $admin['id'];
                    break;
                case 'client':
                    $id = $this->di['session']->get('client_id');
                    break;
            }
        }

        if ($type !== 'admin' && $type !== 'client') {
            throw new \Box_Exception("Unable to invalidate sessions, an invalid type was used");
        }

        $sessions = $this->getSessions();
        foreach ($sessions as $session) {
            $this->deleteSessionIfMatching($session, $type, $id);
        }

        return true;
    }

    private function getSessions(): array
    {
        $query = 'SELECT * FROM session WHERE content IS NOT NULL AND content <> ""';
        $sessions = $this->di['db']->getAll($query);
        return $sessions;
    }

    private function deleteSessionIfMatching(array $session, string $type, int $id): void
    {
        // Decode the data for the current session and then verify it is for the selected type
        $data = base64_decode($session['content']);
        $stringStart = ($type === 'admin') ? 'admin|' : 'client_id|';
        if (!str_starts_with($data, $stringStart)) {
            return;
        }

        // Now we strip off the starting portion so we can unserialize the data
        $data = str_replace($stringStart, '', $data);

        // Finally, perform the check depending on what type of session we are looking for and trash it if it's a match
        if ($type === 'admin') {
            $dataArray = unserialize($data);
            if ($dataArray['id'] === $id) {
                $this->trashSessionByArray($session);
            }
        } else {
            if (unserialize($data) === $id) {
                $this->trashSessionByArray($session);
            }
        }
    }

    private function trashSessionByArray(array $session): void
    {
        $bean = $this->di['db']->dispense('session');
        $bean->import($session);
        $this->di['db']->trash($bean);
    }
        $profileService = $this->di['mod_service']('profile');
        $profileService->invalidateSessions('admin', $model->id);

        $profileService = $this->getMockBuilder('\Box\Mod\Profile\Service')->getMock();

        $di['mod_service'] = $di->protect(function () use ($profileService) {
            return $profileService;
        });
        
        $profileService = $this->getMockBuilder('\Box\Mod\Profile\Service')->getMock();
        $di['mod_service'] = $di->protect(function () use ($profileService) {
            return $profileService;
        });