This proposed fix for #15278 hashes the password reset token securely, and returns the same results every time the hash is created with the same data. I chose the ssh3-512 algorithm as it seems to be a decent balance between a secure hash and the time it takes to generate the hash.
I'm still not totally happy with using a hash algorithm that is optimized to be calculated as fast as possible on modern hardware instead of one intended for cryptographic use and optimized to be rather slow like bcrypt or argon2. But calling them directly outside of
password_hash() doesn't seem to be intended in PHP.
Also this is a bit out of the scope of this issue, but I got the feeling that a password reset token is more a topic of cryptographically signing the data that needs verification instead of hashing it.
Okay, I looked into the password reset process a bit more and am now even more confused as to why we need this hash at all.
I always assumed the password reset was not stored server side at all, but Matomo actually stores the information about the password server side:
But if we are already doing this, then we could simplify the password reset by a lot without any insecure hashes, weird shuffeling around of characters and many ways to mess this up in a security critical way.
Instead we could just generate a long random string from a cryptographically secure source, store it in the DB (with metadata) as done now and send it to the user. And on clicking on the reset link we can compare them if they are the same and accept it if it is.
Maybe I am missing something important, but this seems more secure and a lot harder to mess up to me.
@Findus23 The token isn't stored. It's tried to be regenerated based on the given user and the possible reset time range. See https://github.com/matomo-org/matomo/blob/7e82e80d7aa44714a412a0b6fd6cb53164b8ab6d/plugins/Login/PasswordResetter.php#L248-L262
@sgiehl But the
keySuffix is stored, which is where the actual randomness comes from (I think all other things passed to the hash can be guessed by an attacker)
I'm not sure if changing the hash algorithm for that specific part has any advantages or not.
Common::hash, which was used before, actually uses the
hash_algorithm configured in config. This defaults to
whirlpool. Not sure if changing that to something more modern would break anything, but maybe it might be better than changing it only for the password resetter maybe?
@sgiehl that circles back to my first implementation attempt. The thing with the config option is it's for a slightly different purpose, and could (and really should) be set to an algorithm that has varying results for the same input.
I guess currently is expected behavior that
Common::hash shouldn't have varying results. So it might be safe to change the default hash algo in configuration maybe. But besides that we could also completely refactor the password resetter. Not sure if would have any disadvantages / security concerns to store the hash in the database, so there is no need to actually regenerate it 🤔
Seems we're already storing parts of the token in
savePasswordResetInfo (keySuffix). Could we instead store the full token in there? Not in plain text but eg use
password_verify for the token ? It's actually not ideal that keySuffix is stored in plain text.
In my opinion it be fine if any sent password reset link before the Matomo update to this version will break if they try to click after the Matomo update. AKA any link generated from keySuffix will no longer work. But since links are only valid for 24 hours + they are clicked usually right after requesting the link it should be fine.
If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.