@mwithheld opened this Pull Request on September 16th 2021 Contributor

Description:

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.

@Findus23 commented on September 17th 2021 Member

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.

@Findus23 commented on September 17th 2021 Member

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:
https://github.com/matomo-org/matomo/blob/d2dd6570ae4461db3cf2697e5b3adf9090645852/plugins/Login/PasswordResetter.php#L509-L521

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.

@sgiehl commented on September 17th 2021 Member

@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

@Findus23 commented on September 17th 2021 Member

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

@sgiehl commented on September 20th 2021 Member

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?

@mwithheld commented on September 20th 2021 Contributor

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

@sgiehl commented on September 24th 2021 Member

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 🤔

@tsteur commented on September 26th 2021 Member

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_hash and password_verify for the token ? It's actually not ideal that keySuffix is stored in plain text.

@tsteur commented on September 26th 2021 Member

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.

@github-actions[bot] commented on October 11th 2021 Contributor

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

Powered by GitHub Issue Mirror