search

silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
721 stars 821 forks source link

Reduce default strength of EntropyPasswordValidator #11413

Open emteknetnz opened 1 month ago

emteknetnz commented 1 month ago

In the use symfony/validator work the new EntropyPasswordValidator was made the default PasswordValidator for CMS 6, however this appears to be a bit too strong compared to CMS 5

We set the EntropyPasswordValidator to have a default strength on PasswordStrength::STRENGH_STRONG. This means you now require a password of at least 16 characters, and 19 characters if using a passphrase

The default password validation in CMS 5 is only 8 characters. This goes up to 10 if using cwp-core or similar configuration was used, and you also needed some uppercase / punctuation characters in there too

Given that STRENGTH_STRONG is such a departure from CMS 5, it seems like we should either (in order of smallest to biggest change)

@madmatt Do you have any views on this?

Comparison of what passes the password validators

CMS 5 - silverstripe/installer

1234567 fail 7 chars
12345678 pass 8 chars

CMS 5 - silverstripe/installer with cwp/cwp-core config https://github.com/silverstripe/cwp-core/blob/3/_config/security.yml#L9

12345678 fail 8 chars
1234567890 fail 10 chars
1234567abc fail 10 chars
1234567abC pass 10 chars
1234567ab_ pass 10 chars

CMS 6 - PasswordStrength::STRENGTH_VERY_WEAK;

Symfony throws an Exception :-) Minimum score is actually STRENGTH_WEAK

CMS 6 - PasswordStrength::STRENGTH_WEAK;

the-quick fail 9 chars
Abcdef!@# fail 9 chars
tH3-q#iC= fail 9 chars
the-quick- fail 10 chars
Abcdef!@#$ pass 10 chars
tH3-q#iC=! pass 10 chars
the-quick-b pass 11 chars
123456789012345678 fail 18 chars
1234567890123456789 pass 19 chars

CMS 6 - PasswordStrength::STRENGTH_MEDIUM;

the-quick-br fail 12 chars
Abcdef!@#$%^ fail 12 chars
tH3-q#iC=!,0 fail 12 chars
the-quick-bro fail 13 chars
Abcdef!@#$%^& pass 13 chars
tH3-q#iC=!,0@ pass 13 chars
the-quick-brow pass 14 chars
123456789012345678901234 fail 24 chars
1234567890123456789012345 pass 25 chars

CMS 6 - PasswordStrength::STRENGTH_STRONG;

Abcdef!@#$%^&*( fail 15 chars
tH3-q#iC=!,0@v~ fail 15 chars
Abcdef!@#$%^&*() pass 16 chars
tH3-q#iC=!,0@v~* pass 16 chars
the-quick-brown-fo fail 18 chars
the-quick-brown-fox pass 19 chars
123456789012345678901234567890 fail 30 chars
1234567890123456789012345678901 pass 31 chars

CMS 6 - PasswordStrength::STRENGTH_VERY_STRONG;

Didn't test

Acceptance criteria

Video of POC

https://www.youtube.com/watch?v=HAFfEhTzcBo

PRs

madmatt commented 1 month ago

Couple of questions that I think I know the answer to but want to confirm:

  1. When upgrading, you're not forced to enter a new valid password in order to log in, right? It's just when you next go to change your password, reset your password or create a new user that you'd be required to do so?
  2. This would apply for all Member objects by default, right? For example both members with access to the CMS and those just registering as a website user (assuming a website allows registration)?
  3. This would be easy to override, via YML config?
  4. Can we emulate the old behaviour in some way easily, for those that want to opt out - without introducing jank like a custom validator or other annoying-thing-we-will-want-to-deprecate-later?
  5. Finally, how do we display validation failures - it's not necessarily as easy as saying "just make sure you have uppercase, lowercase, numbers and symbols and at least X characters" anymore as it's not a set of YML values, rather it's a strength rating, right? I guess we could type a user-friendly description to each of the different default strengths...

IMO I think it's worth using the jump to 6 to meaningfully improve security, so I'm more in favour of a jump to PasswordStrength::STRENGTH_STRONG as a default, as long as you can change it easily and we document it clearly in the upgrade guide. If it's a YML change to get back to (something close to) the old settings, even if it's not exactly the same, this seems like an easy decision for people to make, while still ensuring that new builds and those more security conscious sites get a safer experience out of the box.

It means we have to trust people upgrading will look at the docs - but I think that should be a given.

Overall, I'm in favour of going to STRENGTH_STRONG as a default so long as it's easy to revert - but I don't feel super passionate about it either. Call it 75% in favour, 25% against :)

GuySartorelli commented 1 month ago
  1. When upgrading, you're not forced to enter a new valid password in order to log in, right? It's just when you next go to change your password, reset your password or create a new user that you'd be required to do so?

Correct - there is no retrospective validation of passwords. Password validation only happens when you're setting a new password.

  1. This would apply for all Member objects by default, right? For example both members with access to the CMS and those just registering as a website user (assuming a website allows registration)?

Yup, unless there's some custom logic changing how that works.

  1. This would be easy to override, via YML config?

Very easy, yes

  1. Can we emulate the old behaviour in some way easily, for those that want to opt out - without introducing jank like a custom validator or other annoying-thing-we-will-want-to-deprecate-later?

The old behaviour is retained in the RulesPasswordValidator which you can set via YAML config or via PHP code.

  1. Finally, how do we display validation failures - it's not necessarily as easy as saying "just make sure you have uppercase, lowercase, numbers and symbols and at least X characters" anymore as it's not a set of YML values, rather it's a strength rating, right? I guess we could type a user-friendly description to each of the different default strengths...

The validation error message in ConfirmedPasswordField for CMS 6 is "The password strength is too low. Please use a stronger password." That's the same as the message from symfony's validation logic, which is what the EntropyPasswordValidator uses.

There are a variety of messages in the RulesPasswordValidator depending on what validation fails.

emteknetnz commented 1 month ago

I agree that we should meaningfully increase security, though I think I'd still prefer STRENGTH_MEDIUM as a default simply because requiring more than 16 chars for a basic style password feels like it goes past some sort of psychological barrier for me. We should probably avoid STRENGTH_WEAK if for no other reason that it has the word WEAK in there which makes justifying it hard.

Ideally we'd have a UX component in the password field that provides feedback to the user about password strength as you type, it could be as simple as a line of text below the input element that says "Password strength: weak".

madmatt commented 1 month ago

This sounds like a good compromise to me. Increase security but without making it insanely difficult for the average user. I'm a 👍🏼 to using STRENGTH_MEDIUM.

Thanks for the detailed feedback @GuySartorelli, all sounds great to me, and awesome that there's the RulesPasswordValidator that you can fallback to. In that case, so long as we document that in the upgrade guide as a fallback option if you want to retain your previous rules, then that sorts out any issues with increasing the default requirement.

It's also worth thinking about our overall strategy around passwords here - one of the reasons for long / complex passwords is to avoid brute force attacks. That's not as relevant for Silverstripe because of the lockout system that only allows you a small number of tries within 15minutes before getting locked out - meaning that brute forcing is essentially impossible for passwords a lot shorter than 16 characters.

GuySartorelli commented 1 month ago

Brute force attacks can also be used if someone gets a dump of the database (though granted the attack strategy changes a little in that scenario), so any password strategy should always take them into account.

madmatt commented 1 month ago

Yeah, that is certainly true and is a valid consideration (nice shout @GuySartorelli, thanks!), although there's a lot worse that attackers are likely to do with a full database dump from any site that isn't purely public information. I reckon MEDIUM strikes a reasonable balance - and people can always choose to make it harder as well.

emteknetnz commented 1 month ago

Thanks for the feedback, sounds like we should set it to MEDIUM, which is the default level in symfony/validator

I've made a proof of concept for the UX feedback, PRs linked in description

Here's a video of the POC https://www.youtube.com/watch?v=HAFfEhTzcBo

GuySartorelli commented 1 month ago

Overall what you've shown in the video looks neat. Won't make sense when a different validation callback is passed into ConfirmedPasswordField or when a different password validator is applied to Member.