matomo 15278 Make SecureHash() use a secure hash

Fix issue https://github.com/issues/matomo-org/matomo/15278 Secure Hash is not secure. Use passwordHelper->hash() which uses the Password::preferredAlgorithm() hash.

Asked Jan 11 '22 18:01
avatar mwithheld
mwithheld

8 Answer:

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.

1
Answered Sep 06 '21 at 04:00
avatar  of justinvelluppillai
justinvelluppillai

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).

1
Answered Sep 07 '21 at 16:15
avatar  of mwithheld
mwithheld

@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.

1
Answered Sep 07 '21 at 21:45
avatar  of justinvelluppillai
justinvelluppillai

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.

1
Answered Sep 07 '21 at 22:47
avatar  of mwithheld
mwithheld

Yeah that is not part of my PR. It seems to be from rebasing on the 4.x-dev branch. 7f88a735 Fix multiple conversion selection in edit goal form (#17974) I have reset my branch to after my first commit, the added the correct second commit. Of course, now my branch is behind 4.x-dev by a handful of commits.

On Tue, Sep 7, 2021 at 4:08 PM Justin Velluppillai @.***> wrote:

@.**** commented on this pull request.

In plugins/Goals/angularjs/manage-goals/manage-goals.controller.js https://github.com/matomo-org/matomo/pull/17961#discussion_r703910526:

@@ -50,7 +50,7 @@ }

         self.goal.matchAttribute = matchAttribute;
  •        self.goal.allowMultiple = allowMultiple;
  •        self.goal.allowMultiple = allowMultiple == true ? "1" : "0";

has this change accidentally been included from another PR?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/matomo-org/matomo/pull/17961#pullrequestreview-748497514, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGCSOVQ6EZNIVAAY3Z7ZETUA2LQDANCNFSM5DK4QTMQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

1
Answered Sep 07 '21 at 23:19
avatar  of mwithheld
mwithheld

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?

1
Answered Sep 08 '21 at 01:12
avatar  of justinvelluppillai
justinvelluppillai

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.

1
Answered Sep 08 '21 at 01:16
avatar  of mwithheld
mwithheld

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 https://github.com/matomo-org/matomo/blob/7e82e80d7aa44714a412a0b6fd6cb53164b8ab6d/plugins/Login/PasswordResetter.php#L248-L262

1
Answered Sep 08 '21 at 15:27
avatar  of sgiehl
sgiehl