Change the remember cookie to store a MAC of the users password hash instead of their real password hash #58107
Conversation
…ad of their real password hash
|
FWIW, this could also require a GDPR notice for anything logging cookie data after SSL termination, since it is effectively password hash exfiltration :-| |
|
and of course the tests pass locally but not in CI... |
|
I'm not sure if cba18c3 is the right way to invent an app secret to use in tests; please let me know if there's a better way. |
|
I'd note that just as in #19843, a side effect of deploying this is that all users will be logged out. It would be possible to provide a fallback on the checking side to keep existing cookies working, but I'm not sure it's worth it.
|
That is to be considered a positive effect: a security bug that corrects itself shall have no fallback option. |
Synchro
changed the title
|
Thanks for the PR! Getting up to speed here, but am still a bit confused. All Laravel cookies are already encrypted and signed. I created a new Laravel application using our starter kit. I login with "Remember me" checkbox checked. I inspect cookies and see a
If you base64_decode that you get: That is a JSON structure generated by the Laravel encryption class. You now need to decrypt that server side using the So, the password hash is already encrypted and signed with the I'm a bit weary of a breaking change to log out every active Laravel application user when cookie encryption is enabled by default for all Laravel applications. I would rather just make the change to not allow cookie encryption to be disabled for this specific cookie. |
|
Yes, I’m aware of that, and mentioned it in the PR. The problem was my customer had disabled cookie encryption, and wasn’t aware that this exposure existed. The reason the hash is there is to provide automatic invalidation if the password has been changed, but introducing a catastrophic security risk to achieve that seems like a net loss when it is easily done without that risk. So the solution this provides is to ensure that the hash is not exposed even if cookie encryption is disabled, which is also a classic defence-in-depth measure that the original implementation fails to do; one measure failing should not result in major security issues, as this did. |
|
@Synchro I see. I think there is likely a way to write this PR so that both paths are supported, and thus existing users aren't logged out, but new remember me cookies are issued using your HMAC approach, allowing a graceful roll out of the change. |
|
Yes, I mentioned that possibility, but the cost of doing that is the addition of code that will likely never be deleted, even though the problem will self-correct after the cookie expires anyway. It will involve restoring the old mechanism to check values if the new one isn’t there; I’ll add a commit to do that this weekend. |
|
I managed to squeeze that in. |
| */ | ||
| public function hashPasswordForCookie($passwordHash) | ||
| { | ||
| $container = Container::getInstance(); |
There was a problem hiding this comment.
This is pretty hacky. The data should be passed through the session guard, rather than it calling out to the container. The container isn't even a dependency of the auth package, so this would break people installing this package directly.
There was a problem hiding this comment.
Sorry, I wasn't sure how to reliably get hold of the app secret, thanks for dealing with it @taylorotwell.
6 participants
During a pentest of a customer's deployed app, I inspected the
remember_web_*cookie that it had set. It contained an encoded string, which I decoded and was shocked to find that it contained the real server-side bcrypt hash of the user's password. I set up the hash for offline cracking, which succeeded after only about 2 hours due to a lax password policy.Looking at the SessionGuard history, it seems that the hash was included to provide a means for these session tokens to be invalidated automatically if the user changes their password (thus changing the hash). I can see that being useful, but it comes at the price of introducing an unnecessary security risk. Cookie encryption (which I gather is enabled by default, but in this case it had been disabled) does avoid the risk, but defence in depth applies; the same functionality can be implemented without any such risk.
The right way to do this is not to use the password hash directly, but a MAC derived from it (a hash is not good enough), keyed using the app's secret. It uses the app secret instead of a session fingerprint because we want it to match all active sessions. This retains all the same useful automatic invalidation if the password changes, but without exposing anything of use to an attacker, even if cookie encryption is disabled.
This PR introduces one new public method to provide the MAC, and then uses it in the cookie creation and session checking code, and includes updated tests to match.