Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SecureHash is not secure #15278

Open
mattab opened this issue Dec 17, 2019 · 4 comments · Fixed by #17961
Open

SecureHash is not secure #15278

mattab opened this issue Dec 17, 2019 · 4 comments · Fixed by #17961
Labels
c: Security For issues that make Matomo more secure. Please report issues through HackerOne and not in Github. Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement.
Milestone
For Prioritization

Comments

@mattab
Copy link
Member

mattab commented Dec 17, 2019

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:

matomo/plugins/Login/PasswordResetter.php

Line 305 in e922479

protected function generateSecureHash($hashIdentifier, $data)

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.
@mattab mattab added the c: Security For issues that make Matomo more secure. Please report issues through HackerOne and not in Github. label Dec 17, 2019
@mattab mattab added the Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement. label Mar 30, 2020
@mwithheld
Copy link
Contributor

mwithheld commented Sep 3, 2021 •
edited

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
Copy link
Contributor

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

This was referenced Sep 3, 2021
Closed
Merged
@justinvelluppillai justinvelluppillai linked a pull request Sep 8, 2021 that will close this issue
Make password reset use a secure hash to create reset token #17961
Merged
@sgiehl sgiehl reopened this Sep 8, 2021
@mwithheld
Copy link
Contributor

mwithheld commented Sep 16, 2021 •
edited

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
Copy link
Member

sgiehl commented Sep 16, 2021

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

@mwithheld mwithheld mentioned this issue Sep 16, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c: Security For issues that make Matomo more secure. Please report issues through HackerOne and not in Github. Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement.
Projects
None yet
Milestone
For Prioritization
Development

Successfully merging a pull request may close this issue.

Make password reset use a secure hash to create reset token
5 participants
@mattab @mwithheld @justinvelluppillai @sgiehl @innocraft-automation