SecureHash is not secure

[mattab]

As reported by @Findus23

Not really a vulnerability in itself, but also not secure and might cause issues if someone uses the function without checking in the future. Therefore, I want to document it here.

The function generateSecureHash here is really not secure:

https://github.com/matomo-org/matomo/blob/e92247972a99092eb300bcbc163492542017d1b5/plugins/Login/PasswordResetter.php#L305

It hashes the string with $this->hashData which again calls Common::hash which uses the whirlpool hash which is fast and not intended for cryptographic use cases.

The splitting of data is just distraction as with 50000000 Hashes per second on a simple GTX 1060 Ti there is no need to store rainbowtables.

I guess there is no reason to not use the secure slow hashes used for passwords also for password reset tokens.

[mwithheld]

Seems to me we could just update the hashData function as follows and it will then use the Password::preferredAlgorithm() hash.

    protected function hashData($data)
    {
        return $this->passwordHelper->hash($data);
    }

If that makes sense I can toss in a PR pretty quick.

[justinvelluppillai]

Thanks @mwithheld that'd be great if you fired up a PR for this.

[mwithheld]

Assuming the password reset hash should be secure and reproducible, how about using something like is used here:

namespace Piwik\Session\SaveHandler;
...
class DbTable
{
...
    const TOKEN_HASH_ALGO = 'sha512';
    ...
    private function hashSessionId($id)
    {
        $salt = SettingsPiwik::getSalt();
        return hash(self::TOKEN_HASH_ALGO, $id . $salt);
    }

For better security than sha512, we could choose another algorithm from the hash_algos() list, e.g. sha3-512. A speed comparison is here, and an output length comparison is here.

Proof of concept at https://onecompiler.com/php/3xbpcw5xn

[sgiehl]

Guess adding a more secure hashing for the reset token would be fine that way