15278 Hash password reset token securely #18022

mwithheld · 2021-09-16T17:41:12Z

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.

tsteur · 2021-09-17T00:05:11Z

plugins/Login/PasswordResetter.php

+    /**
+     * Hash algorithm used to create the password reset token.
+     */
+    const TOKEN_HASH_ALGO = 'sha3-512';


fyi would maybe need to check if this hash algo is available and if not fall back to a different one

According to PHP hash_algos(), support for sha3-512 was added in PHP7.1, and the minimum requirement for Matomo is PHP7.2.

Findus23 · 2021-09-17T08:18:24Z

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 · 2021-09-17T08:32:49Z

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:

matomo/plugins/Login/PasswordResetter.php

Lines 509 to 521 in d2dd657

    
               /** 
        
                * Gets password hash stored in password reset info. 
        
                * 
        
                * @param string $login The user login to check for. 
        
                * @return string|false The hashed password or false if no reset info exists. 
        
                */ 
        
               private function getPasswordToResetTo($login) 
        
               { 
        
                   $optionName = self::getPasswordResetInfoOptionName($login); 
        
                   $optionValue = Option::get($optionName); 
        
                   $optionValue = json_decode($optionValue, $isAssoc = true); 
        
                   return $optionValue; 
        
               }

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 · 2021-09-17T08:48:22Z

@Findus23 The token isn't stored. It's tried to be regenerated based on the given user and the possible reset time range. See

matomo/plugins/Login/PasswordResetter.php

Lines 248 to 262 in 7e82e80

    
           public function isTokenValid($token, $user, $keySuffix) 
        
           { 
        
               $now = time(); 
        
               // token valid for 24 hrs (give or take, due to the coarse granularity in our strftime format string) 
        
               for ($i = 0; $i <= 24; $i++) { 
        
                   $generatedToken = $this->generatePasswordResetToken($user, $keySuffix, $now + $i * 60 * 60); 
        
                   if ($generatedToken === $token) { 
        
                       return true; 
        
                   } 
        
               } 
        
               // fails if token is invalid, expired, password already changed, other user information has changed, ... 
        
               return false; 
        
           }

Findus23 · 2021-09-17T09:11:23Z

@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 · 2021-09-20T14:05:34Z

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 · 2021-09-20T14:39:46Z

@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 · 2021-09-24T15:05:37Z

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 · 2021-09-26T19:23:42Z

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 · 2021-09-26T19:25:20Z

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 · 2021-10-11T01:51:50Z

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

github-actions · 2021-11-23T01:45:57Z

This PR was last updated more than one month ago, maybe it's time to close it. Please check if there is anything we still can do or close this PR. ping @matomo-org/core-reviewers

15278 Hash password reset token securely

4a6db1f

tsteur reviewed Sep 17, 2021

View reviewed changes

github-actions bot added the Stale The label used by the Close Stale Issues action label Oct 11, 2021

github-actions bot added the Stale for long The label used by the Close Stale Issues action label Nov 23, 2021

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

15278 Hash password reset token securely #18022

15278 Hash password reset token securely #18022

mwithheld commented Sep 16, 2021

tsteur Sep 17, 2021

mwithheld Sep 17, 2021

Findus23 commented Sep 17, 2021

Findus23 commented Sep 17, 2021

sgiehl commented Sep 17, 2021

Findus23 commented Sep 17, 2021

sgiehl commented Sep 20, 2021

mwithheld commented Sep 20, 2021

sgiehl commented Sep 24, 2021

tsteur commented Sep 26, 2021

tsteur commented Sep 26, 2021

github-actions bot commented Oct 11, 2021

github-actions bot commented Nov 23, 2021

15278 Hash password reset token securely #18022

15278 Hash password reset token securely #18022

Conversation

mwithheld commented Sep 16, 2021

Description:

tsteur Sep 17, 2021

Choose a reason for hiding this comment

mwithheld Sep 17, 2021

Choose a reason for hiding this comment

Findus23 commented Sep 17, 2021

Findus23 commented Sep 17, 2021

sgiehl commented Sep 17, 2021

Findus23 commented Sep 17, 2021

sgiehl commented Sep 20, 2021

mwithheld commented Sep 20, 2021

sgiehl commented Sep 24, 2021

tsteur commented Sep 26, 2021

tsteur commented Sep 26, 2021

github-actions bot commented Oct 11, 2021

github-actions bot commented Nov 23, 2021