Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Security Fix for Session Fixation - huntr.dev #1112

Closed
wants to merge 4 commits into from
Closed

Security Fix for Session Fixation - huntr.dev #1112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
57cd2b7
Remove pesistent sessions across all browsers/devices after pasword c…
alromh87 Sep 23, 2020
2b93f30
Show invalid bounty
alromh87 Sep 24, 2020
8508a8c
Merge pull request #2 from alromh87/RateLimit
JamieSlome Oct 9, 2020
5c379e5
Merge pull request #1 from alromh87/FixSessionFixation
JamieSlome Oct 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion app/sprinkles/account/config/default.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,18 @@
'check_username_request' => null,
'password_reset_request' => null,
'registration_attempt' => null,
'sign_in_attempt' => null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

N.B.: The sign_in_attempt is null on purpose, as it's enabled only on production environment.

'sign_in_attempt' => [
'method' => 'ip',
'interval' => 3600,
'delays' => [
4 => 5,
5 => 10,
6 => 20,
7 => 40,
8 => 80,
9 => 600,
],
],
'verification_request' => null,
],

Expand Down
28 changes: 24 additions & 4 deletions app/sprinkles/account/src/Controller/AccountController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1182,13 +1182,19 @@ public function setPassword(Request $request, Response $response, $args)
/** @var \UserFrosting\Sprinkle\Account\Authenticate\Authenticator $authenticator */
$authenticator = $this->ci->authenticator;

// Log out any existing user, and create a new session
if ($authenticator->check()) {
$authenticator->logout();
// Remove persistent sessions across all browsers/devices, and create a new session
$user = $passwordReset->user;

// Make sure logout will have a valid user
if (!$authenticator->check()) {
$authenticator->login($user);
}

// Remove persistent sessions
$authenticator->logout(true);

// Auto-login the user (without "remember me")
$user = $passwordReset->user;
// Create a new session
$authenticator->login($user);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is not optimized : Login-in the user to log him out again, to log him in again is not an optimized process.

Note with UF5, the behavior of this page has changed. First, the page is guarded by the GuestGuard middleware, making sure nobody is logged in to access it. It then sends the user back to the login page instead of login him in automatically.


$ms->addMessageTranslated('success', 'WELCOME', $user->export());
Expand Down Expand Up @@ -1301,6 +1307,20 @@ public function settings(Request $request, Response $response, $args)

$currentUser->save();

// If user changed his password, remove persistent sessions across all browsers/devices, and create a new session
if (isset($data['password'])) {

/** @var \UserFrosting\Sprinkle\Account\Authenticate\Authenticator $authenticator */
$authenticator = $this->ci->authenticator;

// Keep user to re-login after persistent session cleanup
$user = $currentUser;
$authenticator->logout(true);

// Auto-login the user (without "remember me")
$authenticator->login($user);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. Login-in the user to log him out again, to log him in again is not an optimized process.

}

// Create activity record
$this->ci->userActivityLogger->info("User {$currentUser->user_name} updated their account settings.", [
'type' => 'update_account_settings',
Expand Down