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.

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

Security Fix for Session Fixation - huntr.dev #1112

Conversation

huntr-helper
Copy link

https://huntr.dev/users/alromh87 has fixed the Session Fixation vulnerability 🔨. alromh87 has been awarded $25 for fixing the vulnerability through the huntr bug bounty program 💵. Think you could fix a vulnerability like this?

Get involved at https://huntr.dev/

Q | A
Version Affected | ALL
Bug Fix | YES
Original Pull Request | 418sec#1
Vulnerability README | https://github.com/418sec/huntr/blob/staging/bounties/packagist/userfrosting/userfrosting/2/vulnerability.json

User Comments:

📊 Metadata *

Bounty URL: https://www.huntr.dev/bounties/2-packagist-userfrosting%2Fuserfrosting

⚙️ Description *

Persistent sessions are invalidated after password change.

💻 Technical Description *

Userfost uses gbirke/rememberme for persistent sessions so cleanAllTriplets is called via logout in Authenticator class after password change.

🐛 Proof of Concept (PoC) *

  1. Setup UserFrosting and crate user
  2. Login with new user from two different browsers enabling 'Keep me signed in'
  3. Change password on one session
  4. Persistent session on the other browser will still be valid

🔥 Proof of Fix (PoF) *

After fix persistent session will be invalidated and user will have to login again.

userfrostingPWDPOF

👍 User Acceptance Testing (UAT)

Password can be changed normally

@lcharette lcharette added this to the 5.0.1 milestone Nov 25, 2023
@lcharette lcharette modified the milestones: 5.0.1, 5.1.0 Dec 12, 2023
Copy link
Member

@lcharette lcharette left a comment

Choose a reason for hiding this comment

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

I'm a bit late, but I've tested this on UF5. It simply doesn't work as intended. Sure, the "remember me" token will be removed from the database, but all PHP sessions will still be active. If you freshly login both browser and call "logout" on the first, the second browser will still be logged in. In fact, it's exactly the same as "login out" manually from the first browser without using "remember me"

However, I do agree there is a need for a function to logout a specific user from all devices.

// 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.

@@ -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.

$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.

@lcharette
Copy link
Member

I'm closing this as it's old, an opening #1245 to keep track of the issue.

@lcharette lcharette closed this Mar 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants