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

Add configurable password strength check on database password #9782

Merged
merged 13 commits into from
Jan 6, 2024

Conversation

egglessness
Copy link
Contributor

Fixes: #8190

KeepassXC does not provide a way to enforce the minimum strength of the database password, allowing users to pick easy to crack passwords.

Although I see the point of @droidmonkey (#8190), I also believe that quality checks on the master password may be useful, especially when one wants to advocate the usage of this tool to non-security aware users.

For this reason, I added a new setting in the configuration file to enforce the minimum quality that a database password must have in order to be accepted, as follows:

[Security]
DatabasePasswordMinimumQuality=3

The values of DatabasePasswordMinimumQuality map to the values of PasswordHealth::Quality:

  • 0: Bad
  • 1: Poor
  • 2: Weak
  • 3: Good
  • 4: Excellent

This setting is set to 0 by default, meaning that the behaviour of KeepassXC won't change when creating/changing the password database. However, it can be enabled with the appropriate value when needed.

Screenshots

keepassxc

Testing strategy

Manual testing

Type of change

  • ✅ New feature (change that adds functionality)

@droidmonkey
Copy link
Member

Looks good to me! @phoerious

@michaelk83
Copy link

quality checks on the master password may be useful, especially when one wants to advocate the usage of this tool to non-security aware users.
This setting is set to 0 by default

A security aware user would use the available strength indicator and set a strong password anyway. A security unaware user would not be aware of this setting either. They would need someone to set it for them. So I see very little utility to this if the default is 0.

If this affects only the master password, maybe bettter set it to 3 by default? Or hardcode it: simply don't accept a master password that is less than Good.

@droidmonkey
Copy link
Member

droidmonkey commented Aug 23, 2023

Has to be a setting and it also has to be overrideable at the point of alert. For example, all my test databases have the password a haha. I agree with a default of 3.

@egglessness
Copy link
Contributor Author

egglessness commented Aug 23, 2023

So I see very little utility to this if the default is 0.

I agree. I was a bit conservative about this new feature so I proposed it as an opt-in, but surely setting the default to 3 is more secure by default.

So, in the last changes, database passwords are rejected unless they are Good. Users can override this feature by adding the DatabasePasswordMinimumQuality to the config file.

@phoerious
Copy link
Member

We were also considering a warning when the password is less than optimal. That could be added the same way with a default of at least 3.

@egglessness
Copy link
Contributor Author

egglessness commented Aug 24, 2023

We were also considering a warning when the password is less than optimal. That could be added the same way with a default of at least 3.

Okay. With the last changes, a warning for less than Good passwords is showed by default (hardcoded), letting users to continue anyway if they want:
keepassxc2

By setting the DatabasePasswordMinimumQuality flag in the config, this control is enforced and users are prevented from continuing with the weak password:
keepassxc

@egglessness
Copy link
Contributor Author

I think now it should be ok!

I was having some issues in passing tests since the new warning dialog wasn't expected. I changed QMessageBox with your implementation of MessageBox so I could use setNextAnswer in the tests. Now it works on all platforms.

Sorry for the tons of commits! 😅

@egglessness
Copy link
Contributor Author

Hi, will this be included in the next version of KeePassXC?

@droidmonkey
Copy link
Member

Most likely, we aren't in a rush to release the next version.

@droidmonkey
Copy link
Member

Will get this in very soon, sorry for the wait

@droidmonkey droidmonkey merged commit d44486c into keepassxreboot:develop Jan 6, 2024
11 checks passed
@WernerMue
Copy link

don't see it in Version 2.7.7 or am I missing something?

Thanks

@droidmonkey
Copy link
Member

This feature was not backported to 2.7.7

@WernerMue
Copy link

we would have needed this to get approval for distribution within the company. When is this planned for please?

@droidmonkey
Copy link
Member

As of right now, 2.8.0, but we may end up doing a 2.7.8.

droidmonkey pushed a commit that referenced this pull request Apr 29, 2024
* Set default value of DatabasePasswordMinimumQuality to 3 (do not accept a master password that is less than Good)

* Add custom message box button "Continue with weak password"
@WernerMue
Copy link

don't see this feature in change log for 2.7.8, I guess it did not make it yet?

@droidmonkey
Copy link
Member

It did, just missed the text

@WernerMue
Copy link

great, thanks for that!

It is a bit confusing that with enforced password handling you will first get the prompt stating "week password...do you really want to ....." and only after you click on "yes" you get the "bad password" message, this could already have been the initial message.

But this is only nice to have....thanks again!

@itsenaf
Copy link

itsenaf commented May 27, 2024

I am sorry to ask, but I dont get it to work. Version 2.7.8.

Added "DatabasePasswordMinimumQuality=3" to [Security] in keepassxc.ini located in Roaming folder.
Also tried "DatabasePasswordMinimumQuality=Good" and "DatabasePasswordMinimumQuality=true".

Warning for weak password appears, but after this is not being rejected.

@droidmonkey
Copy link
Member

It's on by default, and if you see the warning message, then it is working fine. There is a slight bug when editing encryption settings, the warning appears erroneously.

@itsenaf
Copy link

itsenaf commented May 27, 2024

I want that this message appears:
image
Like "WernerMe" mentioned above :).

@droidmonkey
Copy link
Member

That was not implemented by choice

@itsenaf
Copy link

itsenaf commented May 27, 2024

OK, I was a little bit confused due to comment of WernerMue.
So there is still no possibility to prevent continuing with a weak password.
For future releases, I would be happy to have such option in config file.

Thanks for assist and your application.

@droidmonkey
Copy link
Member

droidmonkey commented May 27, 2024

Sorry I was mistaken, it is implemented. The setting needs to be:

[Security]
DatabasePasswordMinimumQuality=2

Where 2 corresponds to Weak, can be anything less than 4

You will need to restart the application after manually editing the config file. Also, you need to acknowledge the weak password dialog first with this implementation.

@itsenaf
Copy link

itsenaf commented May 28, 2024

I tried this on 3 different devices now. I am not able to make this function work.
Installation: KeePassXC - Version 2.7.8 Revision: f6757d3
Just added to a new installation keepassxc.ini file:
[Security]
DatabasePasswordMinimumQuality=2
Restarted application and also device. Set database password to "a" and got warning about weak password.
Click "Continue" -> works. Unfortunately no expected error :(

@WernerMue
Copy link

I remember I had something similar and had to add it to the second keepass.ini file in local also, not only roaming appdata directory. Thought that I messed something up while testing and did not care, but maybe it is worth a try

@egglessness
Copy link
Contributor Author

Hi @itsenaf,

To properly set the restriction of the minimum password quality for the database, you should add the following lines into the local configuration file (not in roaming) in C:\Users\<your_username>\AppData\Local\KeePassXC\keepassxc.ini:

[Security]
DatabasePasswordMinimumQuality=2

If you want, you can also use this Powershell script I made some time ago to enforce specific settings into the configuration of KeePassXC.

@itsenaf
Copy link

itsenaf commented May 28, 2024

Worked! Thank you so much. Also for your script.

@itsenaf
Copy link

itsenaf commented Jun 17, 2024

I found a bug related to this function.
When you enabled the function in config and try to setup challenge-response with yubikey on existing password safe, it wont let you do that.
Yes, this password safe match the password requirements, since I created it just before with active requirement.
Workaround: Configure challenge-response at the beginning while creating new safe. Or disable the function in config.
keepassbug

@droidmonkey
Copy link
Member

This is fixed on develop branch

@Learninginfo
Copy link

Hello everyone. I am still confused how to implement or configure this. Can you give me a short description where to go and paste and to do

@Learninginfo
Copy link

I tried this on 3 different devices now. I am not able to make this function work. Installation: KeePassXC - Version 2.7.8 Revision: f6757d3 Just added to a new installation keepassxc.ini file: [Security] DatabasePasswordMinimumQuality=2 Restarted application and also device. Set database password to "a" and got warning about weak password. Click "Continue" -> works. Unfortunately no expected error :(

I still don't understand the process

@droidmonkey
Copy link
Member

Are you managing the application for an enterprise? If not, then don't worry about this. If so, read the first post it is very clear.

@Learninginfo
Copy link

Are you managing the application for an enterprise? If not, then don't worry about this. If so, read the first post it is very clear.

Yes I will be managing it for an Enterprise.

@egglessness
Copy link
Contributor Author

Are you managing the application for an enterprise? If not, then don't worry about this. If so, read the first post it is very clear.

Yes I will be managing it for an Enterprise.

Hi @Learninginfo,

Please, refer to this comment.

@Learninginfo
Copy link

Are you managing the application for an enterprise? If not, then don't worry about this. If so, read the first post it is very clear.

Yes I will be managing it for an Enterprise.

Hi @Learninginfo,

Please, refer to this comment.

I see this but am not sure how it works. I have to write a documentation how the admin of the Enterprise. will manage it

@droidmonkey droidmonkey added the pr: backported Pull request backported to previous release label Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: backported Pull request backported to previous release security user interface ux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Increase minimum master password length
7 participants