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

[5.7] Database-less Password Reset #23706

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
@taylorotwell
Copy link
Member

taylorotwell commented Mar 26, 2018

This pull request implements user password resets using signed URLs instead of a database table. The signature can be used to verify that the link was created for a given user and also expiration time is implemented inside the URL as well. Would appreciate a review of the code to make sure everything looks OK.

taylorotwell added some commits Mar 26, 2018

@taylorotwell taylorotwell changed the title Database-less Password Reset [5.7] Database-less Password Reset Mar 26, 2018

@taylorotwell

This comment has been minimized.

Copy link
Member Author

taylorotwell commented Mar 26, 2018

This is just an initial pass at code. Will have to work on the tests.

@kduma

This comment has been minimized.

Copy link
Contributor

kduma commented Mar 26, 2018

Is there any possibility to invalidate link after first use to prevent reply attack?

/**
* The callback that should be used to build the mail message.
* The number minutes for which the reset link should be considered valid.

This comment has been minimized.

@kevdotbadger

kevdotbadger Mar 26, 2018

Contributor

Number OF minutes

/**
* The user provider implementation.
* The number minutes for which the reset link should be considered valid.

This comment has been minimized.

@kevdotbadger

kevdotbadger Mar 26, 2018

Contributor

Number OF minutes

@taylorotwell

This comment has been minimized.

Copy link
Member Author

taylorotwell commented Mar 26, 2018

@kduma not that I know of and that is definitely one concern I have with this.

@taylorotwell

This comment has been minimized.

Copy link
Member Author

taylorotwell commented Mar 27, 2018

Here are some reasons I'm hesitant to merge this PR, even though it is a "cool" idea:

  • URLs can be replayed to reset the token again.

  • Ex-employee could immediately generate password reset links (valid ones) for any account in the system until the encryption keys are rotated. Rotating these keys could be kind of a PITA if you're storing other encrypted data. In a large, high-profile application this becomes a huge source of pain and single point of failure.

  • Links can be essentially "guessed" forever; however, I'm not sure brute-forcing the keys generated by key:generate is realistic.

@tillkruss

This comment has been minimized.

Copy link
Member

tillkruss commented Mar 27, 2018

@taylorotwell, @kduma: If the password hash is included in the signature then the link expires as soon as the password is changed. See #17499. That's why I didn't make a PR with the "signed routes" feature, IMO this is a better approach.

However my PR didn't solve the "employee app key" issue, which is quite heavy, unless it's a small application. That should definitively go into the docs if this gets merged.

@thecrypticace

This comment has been minimized.

Copy link
Contributor

thecrypticace commented Mar 27, 2018

I would guess that if the user's password is included in the signature that item 1 has a high-probability of being a non-issue (unless the generated hash+salt by bcrypt or argon are identical — which I would guess is rather rare)

I also believe it would mean that item 2 is less of an issue because if the employee is able to construct the reset with the valid password hash then it is likely that they have access to the database (or it has been leaked — which is a whole other matter) in which case, if they sill have write access, it then becomes trivial to generate new password reset links. Or just change the password hash at that point.

Having said all this, the points you raise are entirely reasonable ones and I'm genuinely curious if these assumptions I've made are valid.

@kduma

This comment has been minimized.

Copy link
Contributor

kduma commented Mar 27, 2018

@taylorotwell how you managed to prevent reply attack in Forge 2FA implementation? Maybe that can be used here as well?

@thecrypticace, @tillkruss yeah, including user password hash in signature resolve problem with ex-employee.

@thecrypticace

This comment has been minimized.

Copy link
Contributor

thecrypticace commented Mar 27, 2018

2FA is time-based generated code that automatically rotates. The code changes every ~20s with a leeway (5 seconds I think).

Using a time-based algorithm means the link (or a code associated with it) would have to change every N seconds for some number N.

@kduma

This comment has been minimized.

Copy link
Contributor

kduma commented Mar 27, 2018

@thecrypticace TOTP codes are valid 30 seconds but there is sometimes a time window, so most of times the previous/next one or two codes would work (as a remedy for non-synchronized time) - this result in about 2 minutes of validity. Because of that, specification says that you shouldn’t accept same code twice.

@thecrypticace

This comment has been minimized.

Copy link
Contributor

thecrypticace commented Mar 27, 2018

Ah good point.

@taylorotwell

This comment has been minimized.

Copy link
Member Author

taylorotwell commented Mar 27, 2018

I dunno, our current implementation doesn't have any of these problems. Makes me nervous to change things to something less secure.

@tillkruss

This comment has been minimized.

Copy link
Member

tillkruss commented Mar 27, 2018

We can solve 1, but 2 seems like a step back. We could instead of dropping the database resets, offer crypto resets as an alternative for smaller sites/projects as long as we warn users about the potential vulnerability.

@vpratfr

This comment has been minimized.

Copy link

vpratfr commented Mar 27, 2018

Just wondering, what is the real issue with db resets tried to be solved here? Just removing the extra table?

Is that table really a problem? Is the lower security something that people are ready to give up against storing a table somewhere?

On my side, I see no worries with a table as it gives full control over resets, has no security issues, etc.

On the other side, a feature made optional and advertised as less secure seems something bound to be unused or marginally used (and thus something to have in an external package maybe?).

@m1guelpf

This comment has been minimized.

Copy link
Contributor

m1guelpf commented Mar 27, 2018

I really don't see the issue with point 2. If the employee has access to the app key, he has access to the env file, so he can also access the database.

@deleugpn

This comment has been minimized.

Copy link
Contributor

deleugpn commented Mar 27, 2018

@m1guelpf that's not necessarily true.

  • Employee resigns and copies the whole environment variables of the app.
  • After departure, company can easily change database credentials.
  • Ex-employee now no longer have access to the database, but still have the app key.
  • clean Laravel install using that key, he can generate signed URLs for the system he no longer have access to.

Changing the app key could be a pain point if you need to re-encrypt a lot of data.

@m1guelpf

This comment has been minimized.

Copy link
Contributor

m1guelpf commented Mar 27, 2018

@deleugpn The proposed solution of adding the existing password hash to the signed route would fix that scenario, wouldn't it?

@aso824

This comment has been minimized.

Copy link

aso824 commented Mar 27, 2018

@m1guelpf Okay, but again - what problem does it resolve that exists in current solution (password_resets table`)?

IMHO merging this PR doesn't have sense, as current solution is secure without doubt.

@m1guelpf

This comment has been minimized.

Copy link
Contributor

m1guelpf commented Mar 27, 2018

@aso824 The fact that something is working doesn't mean there isn't room for improvement. Having a migration in your app that creates a table only used by the framework isn't very clean.

@shirshak55

This comment has been minimized.

Copy link

shirshak55 commented Mar 27, 2018

@aso824 and we don't need to clean the key hourly :)

@vpratfr

This comment has been minimized.

Copy link

vpratfr commented Mar 27, 2018

One approach which could solve it:

  • generate a key when email has been checked and store that key along the user entry inside the user DB table (a column like the remember_token)
  • use that key instead of the app key to generate the url

It should solve your issues but still involves storing a key per user somewhere.

@milosdakic

This comment has been minimized.

Copy link

milosdakic commented Mar 27, 2018

Not sure if this helps or not.

  • The table is a good reference to see who has requested password rests
  • I don't think the table is limited to password resets only, could be used as part of an invite system for new users
@garygreen

This comment has been minimized.

Copy link
Contributor

garygreen commented Mar 27, 2018

I think if this get's merged it should be another alternative token manager/repository too. Then again, it makes you wonder what's the point in supporting two different ways of managing password resets in the core?

The one aim of this PR seems to be to avoid creating unnecessary writes to a database, so this would benefit HUGE websites who have many users requesting for password resets at any given time. But if you have a huge website then security becomes even more of concern. This smells very JWT to me, which has it's own security concerns.

Personally, I would prefer to see a cached implementation of the database token repository, which uses the cache system to store the password resets - that way users can choose if it's stored in the database, redis, etc.

@36864

This comment has been minimized.

Copy link
Contributor

36864 commented Mar 27, 2018

If this is to be implemented, I think it should be as an alternative to the existing method, not as a replacement.
This would be a pretty big breaking change that would require changing an existing database structure.

I also don't quite understand the need for it. What problem is this trying to solve? It just feels a bit like a solution looking for a problem.

If it does fix a problem that I'm just not aware of, I believe a the following points need to be addressed:

  • Reset tokens need to be easily revokable (swapping out your application's encryption key is not a practical solution for this);
  • Resetting a password must only be possible after a user explicitly asks to do so (Not seeing a way to ensure this without saving state);
  • There must be an easy way to temporarily prevent new tokens from being created and old tokens from being verified (Reactive security measure to prevent a brute-force attack)

I'll grant you that these tokens would not be easily cracked, but they are not impossible to crack. Give it a few years and you'll likely have legacy applications using this system that are suddenly very easy to break into. What's more, this new system would allow a brute-force attack to just keep trying tokens until it can eventually reset someone's password without knowledge of how many users are even in the system.

For small applications, this is a low risk vulnerability, as it's highly unlikely a randomly generated token will match a single user. For larger applications, the risk scales with the number of users.

The current system would require an additional step of generating a password reset request for each user, mitigating risk as not only does the attack require more time to first generate all the tokens (which would either require knowledge of the user list or be itself a brute-force attack at guessing emails) but also allows automated threat detection to temporarily halt generation of new tokens and invalidate all recently generated tokens when a high volume of requests is detected.

In summary:
This new system appears to be less secure than the current system, is not future-proof, and its only advantage seems to be getting rid of a single table. I don't consider that a positive trade-off.

@m1guelpf

This comment has been minimized.

Copy link
Contributor

m1guelpf commented Mar 27, 2018

@36864 The brute-force attack you're talking about isn't viable, as Laravel's rate-limit middleware can easily protect you from this. As a prevention measure, though, you could also hash the user's email with the request to make it more difficult to guess.

@garygreen

This comment has been minimized.

Copy link
Contributor

garygreen commented Mar 27, 2018

@tillkruss that's certainly an interesting read. One take away point from the timing attack article linked in that post:

It’s a lot of work to successfully exploit a timing attack. Therefore, attackers will generally look for easier and more common attacks such as SQLi, XSS, remote code execution, etc. From a practical standpoint, I wouldn’t worry about timing attacks until I was confident that the other potential vectors are secured.

I've never personally seen a timing attack, would be interested in seeing if there is any articles on high profile websites who have been exploited for a timing attack - I suspect most are exploited by other methods.

@taylorotwell

This comment has been minimized.

Copy link
Member Author

taylorotwell commented Mar 27, 2018

@m1guelpf i don't rate limiting is feasible when unauthenticated like password reset. It is basically just going off of IP address at that point which can be easily spoofed.

@laurencei

This comment has been minimized.

Copy link
Member

laurencei commented Mar 28, 2018

If people desperately want "database less" password resets - just use signed routes with a cached random identifier. Have the cache identifier expire after an hour or something.

i.e. app.com/password_reset/signed_url/cached_id

Even knowing the "signed route" is useless without a generated cached id - so it solves the ex-employee problem.

But I'm not sure this is much better than just using a DB - it's just "different".

@ircmaxell

This comment has been minimized.

Copy link

ircmaxell commented Mar 28, 2018

Just a few cents here from an outside security perspective. I haven't reviewed the code deeply, but the concept seems pretty straight forward. And while there are a few nice things to having a db table with the reset tokens (knowing how many times a user issued a reset, etc), they can be acomplished without having to put secret information in the DB. So overall I think this is at least an idea worth exploring...

I'll reply to the primary concerns:

URLs can be replayed to reset the token again.

Incuding the current password hash in the signature would quite elegantly resolve that problem...

Ex-employee could immediately generate password reset links (valid ones) for any account in the system until the encryption keys are rotated. Rotating these keys could be kind of a PITA if you're storing other encrypted data. In a large, high-profile application this becomes a huge source of pain and single point of failure.

I would make the posit here that employees having access to the keys is the problem here, not that they could use them. As pointed out in another reply, if they have access to the key, then the encryption is really relatively meaningless since it doesn't guard theft.

There are lots of Key Management Systems (KMS) available to make keys available to the app but unviewable by devs if this is a concern. Not sure if you have any integrations with any of them, but if this type of attack is a concern, then perhaps that would be a nice solution...?

Links can be essentially "guessed" forever; however, I'm not sure brute-forcing the keys generated by key:generate is realistic.

Assuming you have sufficient entropy in your keys (2^128) the chances of a successful guess without compromising the key is practically impossible. This then comes back to the prior point, which is how do you guard your keys...

@garygreen

This comment has been minimized.

Copy link
Contributor

garygreen commented Mar 28, 2018

@ircmaxell those are all fair points but honestly, all that just to get around the security implications of such a method? As mentioned by someone else:

It just feels a bit like a solution looking for a problem.

I echo this statement. The current database implementation isn't really that bothersome and it doesn't have any of the security concerns of this implementation - so what compelling reasons are there to change it?

@browner12

This comment has been minimized.

Copy link
Contributor

browner12 commented Mar 28, 2018

while I agree there's no pressing reason to get this removed, I think the table-free reset simplifies things a bit.

Due to the fact the create_password_resets_table.php migration lives outside of laravel/framework, it means when there are changes to the process, they require a more hands on manual upgrade. While changes to the migration file are few and far between, they are annoyances when they do occur.

By switching to this new process, we would essentially pull the whole password reset process inside of laravel/framework, abstract everything from the programmer, and updates would now be as simple as composer update.

@garygreen

This comment has been minimized.

Copy link
Contributor

garygreen commented Mar 28, 2018

it means when there are changes to the process, they require a more hands on manual upgrade

This is no different to any changes made in the framework - this is more of a documentation issue and doesn't provide a compelling reason to change.

and updates would now be as simple as composer update

If there is any changes to the signature or security hotfixes then any existing password resets would no longer be valid. Changes to the current system (which there have been none, so far) would be simpler.

I'm certainly not adverse to change or improvements, but I honestly believe this is one step forward, two steps back IMO.

@browner12

This comment has been minimized.

Copy link
Contributor

browner12 commented Mar 28, 2018

like I said, changes are infrequent, but they do happen. Since they live in laravel/laravel, this requires a manual update. And because they are migrations, things are even a little more hairy, because you cannot simply just edit the file.

laravel/laravel@4b2694c#diff-6381df0205738a7e9110cdac1c926341

laravel/laravel@be7b262#diff-6381df0205738a7e9110cdac1c926341

laravel/laravel@3ecc0e3#diff-6381df0205738a7e9110cdac1c926341

laravel/laravel@d6d98f9#diff-6381df0205738a7e9110cdac1c926341

@taylorotwell taylorotwell deleted the password-reset branch Sep 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment