[5.7] Database-less Password Reset

[taylorotwell]

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]

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

[kduma]

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

[kevinruscoe]

Number OF minutes

[kevinruscoe]

Number OF minutes

[taylorotwell]

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

[taylorotwell]

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]

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

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]

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

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]

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

Ah good point.

[taylorotwell]

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

[tillkruss]

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]

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]

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]

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

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

[aso824]

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

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

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

[vpratfr]

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]

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]

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.

[Miguel-Serejo]

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]

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

[tillkruss]

https://paragonie.com/blog/2017/02/split-tokens-token-based-authentication-protocols-without-side-channels

[garygreen]

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

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

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]

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]

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

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]

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]

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