"Weak password" warning incorrectly shown after adding (or removing) a key file from database

[zxsldfjsalkdf]

Overview

I set up a new database file for testing purposes initially without using a key file. My master password was generated with KeePassXC using 10 random words from the default word list to ensure very strong entropy. Later I decide to create and add a key file to the database, and when I went to save changes and click "OK" at the bottom I get an incorrect prompt saying my master password is too weak, which I know is false.

Steps to Reproduce

1. From menu at the top of the application: go to Database > Database Security
2. Add a key file if one does not already exist and save changes, Removing a key file and saving changes also triggers this bug.

Additional information that may help in reproducing:

• Encryption Algorithm was set to ChaCha20 during database creation phase, all other settings were left at default values.
• Master password was generated from within KeePassXC's default word list, the master password only contains words, no symbols (besides spaces separating the words), no numbers. 10 words in length.
• The key file was generated using KeePassXC.

Expected Behavior

If the master password is known to be -without a doubt- very strong then there is not supposed to be a "weak password" prompt when saving changes after adding or removing a key file from the database.

Actual Behavior

After adding or removing a key file from the database and saving changes, a prompt incorrectly shows up saying my password is weak.

Context

KeePassXC - Version 2.7.8
Revision: f6757d3

Operating System: Windows 11 Home - (23H2, OS Build 22631.3527)

[0xdeadbeer]

A few of my additional findings:

• it does not matter what encryption algorithm you used
• nor what kind of password you have, whether you generated it or not

I suspect that it comes down to this:

keepassxc/src/gui/dbsettings/DatabaseSettingsWidgetDatabaseKey.cpp

Lines 181 to 205 in da90319

	if (!m_passwordEditWidget->isEmpty()
	&& m_passwordEditWidget->getPasswordQuality() < PasswordHealth::Quality::Good) {
	auto dialogResult = MessageBox::warning(this,
	tr("Weak password"),
	tr("This is a weak password! For better protection of your secrets, "
	"you should choose a stronger password."),
	MessageBox::ContinueWithWeakPass | MessageBox::Cancel,
	MessageBox::Cancel);
	if (dialogResult == MessageBox::Cancel) {
	return false;
	}
	}
	// If enforced in the config file, deny users from continuing with a weak password
	auto minQuality =
	static_cast<PasswordHealth::Quality>(config()->get(Config::Security_DatabasePasswordMinimumQuality).toInt());
	if (!m_passwordEditWidget->isEmpty() && m_passwordEditWidget->getPasswordQuality() < minQuality) {
	MessageBox::critical(this,
	tr("Weak password"),
	tr("You must enter a stronger password to protect your database."),
	MessageBox::Ok,
	MessageBox::Ok);
	return false;
	}

To my surprise, m_passwordEditWidget->isEmpty() does not return true if the user hasn't clicked on "Change Password" button in the Database Security Settings.. an additional check with m_passwordEditWidget->visiblePage() == KeyComponentWidget::Page::Edit is I hope enough to safe-guard it..? Like so:

diff --git a/src/gui/dbsettings/DatabaseSettingsWidgetDatabaseKey.cpp b/src/gui/dbsettings/DatabaseSettingsWidgetDatabaseKey.cpp
index 1de8e6a9..0c26bccb 100644
--- a/src/gui/dbsettings/DatabaseSettingsWidgetDatabaseKey.cpp
+++ b/src/gui/dbsettings/DatabaseSettingsWidgetDatabaseKey.cpp
@@ -178,7 +178,8 @@ bool DatabaseSettingsWidgetDatabaseKey::save()
     }
 
     // Show warning if database password is weak
-    if (!m_passwordEditWidget->isEmpty()
+    bool isNewPasswordDirty = !m_passwordEditWidget->isEmpty() && m_passwordEditWidget->visiblePage() == KeyComponentWidget::Page::Edit;
+    if (isNewPasswordDirty
         && m_passwordEditWidget->getPasswordQuality() < PasswordHealth::Quality::Good) {
         auto dialogResult = MessageBox::warning(this,
                                                 tr("Weak password"),
@@ -195,7 +196,7 @@ bool DatabaseSettingsWidgetDatabaseKey::save()
     // If enforced in the config file, deny users from continuing with a weak password
     auto minQuality =
         static_cast<PasswordHealth::Quality>(config()->get(Config::Security_DatabasePasswordMinimumQuality).toInt());
-    if (!m_passwordEditWidget->isEmpty() && m_passwordEditWidget->getPasswordQuality() < minQuality) {
+    if (isNewPasswordDirty && m_passwordEditWidget->getPasswordQuality() < minQuality) {
         MessageBox::critical(this,
                              tr("Weak password"),
                              tr("You must enter a stronger password to protect your database."),

[droidmonkey]

That code won't compile. simply call isVisible() on the password widget.

[0xdeadbeer]

Strange, it compiles on my machine..? Anyhow, after some checking to me it seems that calling m_passwordEditWidget->isVisible() at that point in time returns true even if the user does not click on "Change Password"..? Even weirder is the fact that the visible property seems to be false at that point when inspected through Gamma Ray.. ->isVisible() still returns true though.. will look further into the source code the coming days