[5.4] Secure password reset tokens against timing attacks and compromised DB

[HSPDev]

Before this patch, tokens and e-mails where stored in the database in clear text, and lookups where done using simple where()'s.

Problem number 1)
If the DB where somehow leaked, you could compromise any account with a password reset in the database. Password resets are practically randomly generated one-time passwords, and should be treated as passwords and at least obscured by SOME kind of hash function - so why not Bcrypt while we are at it?

See this link for a nice explanation: https://paragonie.com/blog/2016/09/untangling-forget-me-knot-secure-account-recovery-made-simple#secure-password-reset-tokens

Problem number 2)
This is more obscure but potentially a fatal flaw for the Laravel framework. A determined attacker could probably exploit it already.
The problem with using simple where() clauses in the lookup of email + cleartext token, is that MySQL is going to try and optimize this query.

MySQL lookups aren't meant to be constant time, and shouldn't be either; It's job is to be fast.

This however prevent a possible security flaw, as it will start by matching the e-mail field, and if it exists, it will begin matching the token.

If I try and recover using email [email protected] (which I know are in the password reset table because I ordered a reset myself), and I use the token AAAAAAAAAAAAAAAAAAAA, maybe 20 times, and average the response time, and then try BAAAAAAAAAAAAAAAAAAAA 20 times and average the response time, I will be able to make a fairly educated guess at when the string comparison stops in the MySQL database, and therefore deduce the cleartext token.
This will probably take some time, and the time limit on the reset tokens should definitely help combat this already (which is also why I don't see it as critical), BUT if you have a lot of tokens, maybe a slow or misconfigured server, and you are able to do this over and over again without being detected, I'm pretty sure you will get through eventually.
Even tho the tokens are 64 chars as far as I could deduce, it will only take maybe 20 guesses, for around 66 chars chars (a-Z0-9), and the total token length of 64 = 85.000 guesses. According to the birthday paradox, it will likely only take 42500 guesses most of the time, but even if the token expires, the attacker can just order a newly generated one, and try again. He only has to order a password reset AND guess the token for this reset within an hour ONCE, for it to succeed.

This is one of the reasons why we hash our passwords for normal user accounts, but I definitely think password reset tokens should get the same attention.

I hope my pullrequest didn't get too long, and I hope the style guides aren't totally off. It's my first PR but it really nagged me each time I looked at Laravels password resets.

[GrahamCampbell]

Thanks for the PR.

Timing attacks are mitigated by rate limiting, and expiration. That said, the rate limiting implementation in Laravel's core is weak I think. It's session/ip limited, rather than per use I think?

[HSPDev]

@GrahamCampbell Where is the password reset being rate limited? I can se normal logins being ratelimited by ThrottleLogins, and you can use ThrottleRequests in general. But I don't see it being applied to the Password reset controller anywhere?

[GrahamCampbell]

It's my first PR but it really nagged me each time I looked at Laravels password resets.

I'd have preferred if you'd read the security note on the readme though. We generally don't want to make vulnerability information public knowledge without a fix, similar to Symfony. It's a bit late to redact this now though, because it's PR rather than an issue.

We do appreciate you looking into this though for sure! I've let Taylor know about this PR.

[HSPDev]

@GrahamCampbell I honestly didn't think of it as a big vulnerability myself.. It started mostly as the nagging feeling regarding plaintext tokens in the database, but that requires full DB compromise to abuse... But the more I think about the timing attack and the more of the Laravel code I read, the more I hope to find somewhere that would completely negate it.
I have yet to do so... Oops...

[GrahamCampbell]

Where is the password reset being rate limited? I can se normal logins being rate limited by ThrottleLogins, and you can use ThrottleRequests in general. But I don't see it being applied to the Password reset controller anywhere?

Would be done by a rate limiting middleware typically, though, such a rate limiter is floored by the fact someone can just get lots of machines and break in, since as you pointed out, the number of tries is in the thousands, not billions. What we really would need is a per user rate limit, kept in the database. I think that's how Cartalyst's Sentry 2 used to implement this.

Your solution looks reasonable to me, probably better than adding an extra column to the user table, and it does address your 1st point too, though I think we should only consider point 2 a "security risk", point 1 has low probability.

[HSPDev]

@GrahamCampbell
Well, I can agree that both attack vectors are probably not very likely in the wild, but I could still fear them for targeted attacks. Defense in depth I guess.

The danger of rate-limting pr. user is that you can DDoS users. Doing it by IP as we do now, slows down attacks and requires many IP's that will each get slowed to a crawl pretty fast. Just securing it by using bcrypt as I've proposed is probably also the easiest.

Btw. my Github foo is not that strong, but the failing Travis build is caused by the Unit test and not the code itself. It only "mocks" the created_at attribute, and expects the query to use a where() for token also.. I missed that when making my changes. The StyleCI is complaining about whitespace. I will see if I can fix these up before going to bed. :|