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

Allow selecting password_hash algorithm #17199

Merged
merged 5 commits into from Mar 14, 2021
Merged

Conversation

Findus23
Copy link
Member

@Findus23 Findus23 commented Feb 7, 2021

Some people might want their Matomo passwords to be hashed with a slower, more computationally complex algorithm than bcrypt to make sure that in case attackers ever get access to the matomo_user table, bruteforcing the hashes needs a lot of computational power even for more insecure passwords.
Argon2 allows this (and is supported since PHP 7.2) and also provides three options to tune the CPU and memory usage of the hash calculation. Increasing this value might increase load on the server during login, but this isn't something that should happen very often on a typical Matomo instance.

Also one day we could change the default to argon2 (like Nextcloud did), but this feel more like a breaking change than just allowing customization.

Just changing the setting works out of the box as Matomo already checks on login if the stored hash uses the correct algorithm and hashes and stores the password again if not. So after changing the settings, the next time a user loggs in their hash is updated.

As I am not so familar with PHP and the codesyle, this should be more seen as a proof of concept than the definitive implementation.
Also I tried to keep it conceptually similar to https://github.com/nextcloud/server/blob/master/lib/private/Security/Hasher.php

Review

  • Functional review done
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@Findus23 Findus23 added c: Security For issues that make Matomo more secure. Please report issues through HackerOne and not in Github. RFC Indicates the issue is a request for comments where the author is looking for feedback. labels Feb 7, 2021
Copy link
Member

@diosmosis diosmosis left a comment

Choose a reason for hiding this comment

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

LGTM, there are some code formatting issues (if blocks aren't formatted like the rest of matomo), but code looks good and the change makes sense. 👍

@Findus23
Copy link
Member Author

Findus23 commented Feb 8, 2021

Ah, I am not so familiar with the whole braces positioning discussion (I mostly write in Python which doesn't have this issue 🙂 ), so I might have ticked a few to many "in new line" boxes.
It should be looking better now.

@tsteur
Copy link
Member

tsteur commented Feb 8, 2021

@Findus23 quick question: Do you know how much it would make things slower to simply use the highest settings by default? Not sure how often we use the password_verify but might be only on log in etc and maybe we could try to simply use the strongest setting by default so we don't need a config for it? Just a random thought (I'm not much into these options)

@Findus23
Copy link
Member Author

Findus23 commented Feb 9, 2021

There are no highest settings, all three argon2 options are open ended. The nextcloud docs also mention that using more threads than the server has CPUs makes no sense (as it slows things down a lot). And memory is of course limited by the memory limit.

As to why we maybe shouldn't just hardcode argon2 without an option to use bcrypt: The nextcloud team added them afterwards as quite a few people reported a high CPU usage (as is intended with argon2), but their server couldn't handle the load (https://github.com/nextcloud/server/issues/17241). Also it seems like some CPUs are much slower with argon2 (https://github.com/nextcloud/server/issues/22886).
So having a fallback for those few people should be helpful. Even though they are probably fewer with Matomo as on one hand fewer people run it on very-low-end servers like Raspberry Pis and on the other hand it might be that Nextcloud needs to run password_hash on every API request, which Matomo doesn't.

And there is no perfect setting for everyone as the whole point is to slow down the calculation of the hash by a specific time and depending on the CPU speed of the server this might be a bit more or less. (e.g. KeepassXC, which of course can afford higher complexity sets the parameter to a value so that one hash takes one second on the current CPU).

@tsteur
Copy link
Member

tsteur commented Feb 9, 2021

Thanks for clarifying @Findus23

@diosmosis diosmosis added this to the 4.3.0 milestone Mar 14, 2021
@diosmosis diosmosis merged commit 226f8dc into 4.x-dev Mar 14, 2021
@diosmosis diosmosis deleted the password-hash-algorithms branch March 14, 2021 21:49
@sgiehl
Copy link
Member

sgiehl commented Mar 15, 2021

@diosmosis seems you have accidentally added the screenshot to git directly instead of git lfs. I'll change that on 4.x-dev branch directly.

@diosmosis
Copy link
Member

@sgiehl 👍 thanks, I guess I forgot to set lfs up on my new machine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Security For issues that make Matomo more secure. Please report issues through HackerOne and not in Github. RFC Indicates the issue is a request for comments where the author is looking for feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants