Skip to content

Commit

Permalink
security(session): invalidate other sessions on user password change
Browse files Browse the repository at this point in the history
When a user changes their password all other sessions of that user
should be invalidated.

https://cwe.mitre.org/data/definitions/613.html
  • Loading branch information
jeabakker committed Oct 7, 2022
1 parent a5eca41 commit d8a860c
Show file tree
Hide file tree
Showing 5 changed files with 107 additions and 1 deletion.
56 changes: 56 additions & 0 deletions engine/classes/ElggSession.php
Expand Up @@ -267,6 +267,62 @@ public function removeLoggedInUser() {
$this->remove('guid');
_elgg_services()->sessionCache->clear();
}

/**
* Set a user specific token in the session for the currently logged in user
*
* This will invalidate the session on a password change of the logged in user
*
* @param \ElggUser $user the user to set the token for (default: logged in user)
*
* @return void
* @since 3.3.25
*/
public function setUserToken(\ElggUser $user = null): void {
if (!$user instanceof \ElggUser) {
$user = $this->getLoggedInUser();
}
if (!$user instanceof \ElggUser) {
return;
}

$this->set('__user_token', $this->generateUserToken($user));
}

/**
* Validate the user token stored in the session
*
* @param \ElggUser $user the user to check for
*
* @return void
* @throws SecurityException
* @since 3.3.25
*/
public function validateUserToken(\ElggUser $user): void {
$session_token = $this->get('__user_token');
$user_token = $this->generateUserToken($user);

if ($session_token !== $user_token) {
throw new SecurityException(elgg_echo('session_expired'));
}
}

/**
* Generate a token for a specific user
*
* @param \ElggUser $user the user to generate the token for
*
* @return string
* @since 3.3.25
*/
protected function generateUserToken(\ElggUser $user): string {
$hmac = _elgg_services()->hmac->getHmac([
$user->time_created,
$user->guid,
], 'sha256', $user->password_hash);

return $hmac->getToken();
}

/**
* Get current ignore access setting.
Expand Down
5 changes: 5 additions & 0 deletions engine/classes/ElggUser.php
Expand Up @@ -495,6 +495,11 @@ public function canComment($user_guid = 0, $default = null) {
*/
public function setPassword($password) {
$this->setMetadata('password_hash', _elgg_services()->passwords->generateHash($password));
if ($this->guid === elgg_get_logged_in_user_guid()) {
// update the session user token, so this session remains valid
// other sessions for this user will be invalidated
_elgg_services()->session->setUserToken();
}
}

/**
Expand Down
13 changes: 13 additions & 0 deletions engine/lib/sessions.php
Expand Up @@ -265,6 +265,7 @@ function login(\ElggUser $user, $persistent = false) {
// use elgg_get_logged_in_user_entity().
$session = elgg()->session;
$session->setLoggedInUser($user);
$session->setUserToken();

// re-register at least the core language file for users with language other than site default
elgg()->translator->registerTranslations(\Elgg\Project\Paths::elgg() . 'languages/');
Expand Down Expand Up @@ -390,6 +391,8 @@ function _elgg_session_boot(ServiceProvider $services) {
$user = $services->persistentLogin->bootSession();
if ($user) {
$services->persistentLogin->updateTokenUsage($user);
// make sure the session is valid as there is no login
$session->setUserToken($user);
}
}

Expand All @@ -402,6 +405,16 @@ function _elgg_session_boot(ServiceProvider $services) {
logout();
return false;
}

// make sure the user hasn't changed in between requests
// e.g. password reset
try {
$session->validateUserToken($user);
} catch (\SecurityException $e) {
register_error($e->getMessage());
logout();
return false;
}
}

$services->timer->end([__FUNCTION__]);
Expand Down
Expand Up @@ -156,6 +156,7 @@ public function testCanSetCustomUserClassDuringBootSequence() {
]);

$app->_services->session->set('guid', $user->guid);
$app->_services->session->setUserToken($user); // normally this is done during login()

$user->invalidateCache();

Expand Down
33 changes: 32 additions & 1 deletion engine/tests/phpunit/unit/ElggSessionUnitTest.php
Expand Up @@ -14,7 +14,8 @@ public function up() {
}

public function down() {

_elgg_services()->session->removeLoggedInUser();
_elgg_services()->session->invalidate();
}

public function testStart() {
Expand Down Expand Up @@ -61,4 +62,34 @@ public function testCanSetLoggedInUser() {
$this->assertNull($session->getLoggedInUser());
}

public function testUserTokenValidation() {
$user = $this->createUser();
$session = \ElggSession::getMock();

$session->setUserToken($user);
$session->validateUserToken($user);

// change the user password
$user->setPassword('some new password');

// token isn't valid anymore
$this->expectException(\SecurityException::class);
$session->validateUserToken($user);
}

public function testUserTokenValidationLoggedIn() {
$user = $this->createUser();
$session = _elgg_services()->session;

$session->setLoggedInUser($user);
$session->setUserToken();

$session->validateUserToken($user);

// change the user password
$user->setPassword('some new password');

// session should remain valid
$session->validateUserToken($user);
}
}

0 comments on commit d8a860c

Please sign in to comment.