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.

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

Make password reset use a secure hash to create reset token #17961

Merged
merged 2 commits into from Sep 8, 2021
Merged

Conversation

mwithheld
Copy link
Contributor

Fix issue #15278 Secure Hash is not secure. Use passwordHelper->hash() which uses the Password::preferredAlgorithm() hash.

@justinvelluppillai justinvelluppillai added the Needs Review PRs that need a code review label Sep 3, 2021
@justinvelluppillai
Copy link
Contributor

Thanks @mwithheld. I'm not sure why the tests didn't run on this PR but to make them pass it looks like PasswordResetterTest needs updating to allow for the new characters possible in passwordHelper->hash function. Eg replace preg_match('/resetToken=[\s]*3D([a-zA-Z0-9=\s]+)<\/p>/', $body, $matches); with preg_match('/resetToken=[\s]*3D([^<]+)<\/p>/', $body, $matches); or similar.

@mwithheld
Copy link
Contributor Author

Good find -- I totally get it and will tweak that regex. I can see how to trigger the error manually, but can you tell me what tests brought this to your attention? I see no test failures when I run ./console tests:run /opt/bitnami/matomo/plugins/Login/tests/Integration/PasswordResetterTest.php, for example, and have so far been unable to run all tests (due to OmniFixture problems).

@justinvelluppillai
Copy link
Contributor

@mwithheld that command produces the following results for me, all related to that regex not finding the token from the email.

FAILURES!
Tests: 7, Assertions: 9, Failures: 5.

So you are running the right tests, I'm not sure why they are passing if you are running them against the change you made.

@mwithheld
Copy link
Contributor Author

Got it - my file sync to docker was not working the way I thought it was. I've reproduced the 5 failures, and confirmed your suggested change to the test fixes the issue. A commit is incoming.

@mwithheld
Copy link
Contributor Author

mwithheld commented Sep 7, 2021 via email

@justinvelluppillai
Copy link
Contributor

It's ok if your branch is behind a little, it looks like it will still merge.

Your PR is not running tests because travis seems to be flagging it as "Abuse detected". Maybe you could try logging in to travis directly as suggested here https://travis-ci.community/t/some-pr-are-not-analyzed-with-message-abuse-detected/1191/8 and see if that fixes it for future PRs?

Copy link
Contributor

@justinvelluppillai justinvelluppillai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mwithheld for your work on this PR!

@justinvelluppillai justinvelluppillai added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Sep 8, 2021
@justinvelluppillai justinvelluppillai added this to the 4.5.0 milestone Sep 8, 2021
@justinvelluppillai justinvelluppillai linked an issue Sep 8, 2021 that may be closed by this pull request
@justinvelluppillai justinvelluppillai merged commit 7e82e80 into matomo-org:4.x-dev Sep 8, 2021
@mwithheld
Copy link
Contributor Author

My pleasure to work with you :) I've logged into Travis and hope that cleans up the problem - would sure be helpful to see the test results.

@mwithheld mwithheld deleted the 15278 branch September 8, 2021 01:16
@justinvelluppillai justinvelluppillai changed the title 15278 Make SecureHash() use a secure hash Make password reset use a secure hash to create reset token Sep 8, 2021
@sgiehl sgiehl mentioned this pull request Sep 8, 2021
11 tasks
@sgiehl
Copy link
Member

sgiehl commented Sep 8, 2021

I will revert this change, as we can't use a hash algorithm that has varying results for the same input with the current implementation.
When resetting the password we are trying to regenerate the reset token and compare it with the given one.
This currently fails, and thus the password reset is broken. Would have been visible in a UI tests, if tests would have run 🙈
See

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;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SecureHash is not secure
3 participants