@mattab opened this Issue on December 17th 2019 Member

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:


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 commented on September 3rd 2021 Contributor

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 commented on September 3rd 2021 Contributor

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

@mwithheld commented on September 16th 2021 Contributor

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](https://www.php.net/manual/en/function.hash.php#124509), and an [output length comparison is here](https://www.php.net/manual/en/function.hash.php#124917).

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

@sgiehl commented on September 16th 2021 Member

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

Powered by GitHub Issue Mirror