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

Password Reset Tokens Never Expire Vulnerability in Meteor Accounts using password. #11307

Closed
evolross opened this issue Jan 25, 2021 · 6 comments
Labels
good first issue Good first issue or something that should is nice to do. needs-reproduction We can't reproduce so it's blocked Project:Accounts:Password

Comments

@evolross
Copy link
Contributor

evolross commented Jan 25, 2021

Meteor Version: 1.10.2
Operating System: OS X & Galaxy

Upon calling Accounts.sendResetPasswordEmail a new reset password token is generated. Any tokens issued prior to this still work. This is considered a vulnerability on hacker one and weak security implementation.

I could see perhaps this being a usability issue if it takes a few minutes to receive a password reset email and the user is slamming the button thus causing each one that is sent to be invalidated and causing frustration once it arrives.

The obvious vulnerability is if an email account is compromised, an attacker could easily compromise a Meteor account with an old password reset token, then delete the reset email.

I believe in the current Meteor Accounts, password reset tokens expire by default after three days. It might be good to add a configurable setting to increase this to only a few minutes (for example) and/or add a setting to instantly invalidate previously issue password reset tokens upon issuance of a new token.

Easily reproduced in any Meteor install using Meteor Accounts and passwords.

@StorytellerCZ
Copy link
Collaborator

StorytellerCZ commented Jan 25, 2021

There is the loginExpirationInDays option param that can be adjusted to reduce the expiration time. So setting that to 1 will expire the token after 24 hours, but I agree that expiring the old token when a new is generated should also be added. Also after a token is used it should not be usable again...

@filipenevola filipenevola added good first issue Good first issue or something that should is nice to do. ready We've decided how to solve or implement it labels Feb 1, 2021
@bcluyse
Copy link

bcluyse commented Mar 26, 2021

Hi, I was taking a look into fixing this but I am unable to reproduce as the services.password.reset token is replaced when a new one is requested:

'services.password.reset': tokenRecord

and it also removes the services.password.reset token when the password has been reset:

$unset: { 'services.password.reset': 1 }

Am I overlooking something here or is this actually not an issue (anymore)?
Thanks

@StorytellerCZ
Copy link
Collaborator

@evolross Can you provide a reproduction?

@StorytellerCZ StorytellerCZ added needs-reproduction We can't reproduce so it's blocked and removed ready We've decided how to solve or implement it labels May 4, 2021
@StorytellerCZ
Copy link
Collaborator

I'm closing this since we don't have a reproduction, feel free to open this if this encounter this issue and if possible, please provide a reproduction.

@evolross
Copy link
Contributor Author

Finally tried to create a repo for this and upon a deeper investigation, the previous token indeed returns Token Expired as expected. My apologies for this one. I didn't fully investigate when my security researcher reported it.

They probably saw that it allows you to enter a new password but didn't test that it returns Token Expired upon actually trying to set it.

@StorytellerCZ
Copy link
Collaborator

No worries, I'm glad that everything checked out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good first issue or something that should is nice to do. needs-reproduction We can't reproduce so it's blocked Project:Accounts:Password
Projects
None yet
Development

No branches or pull requests

4 participants