Add Pin Quick Unlock and Remember Quick Unlock for Win/Mac #11520

droidmonkey · 2024-12-02T04:55:56Z

Added ability to remember quick unlock across KeePassXC sessions and database open/close. This relies on the public UUID feature introduced earlier in the develop branch to store the encrypted database credentials into the Windows Credential Store or the macOS Key Store. Remembering quick unlock is not supported on Linux due to the lack of a unified/standardized key store across distros.

Also introduce pin unlock as a fallback option when OS Native security tools are not available for use. The pin is set on first unlock and must be between 4 and 8 digits. The pin hash is used in conjunction with a random IV to encrypt database credentials in memory.

Shifted security setting to be an enable/disable quick unlock by default setting. Now that quick unlock is always available (via Pin fallback) it makes more sense to allow the user to choose what to do at time of unlock. The remember quick unlock setting is hidden on Linux since it is not functional.

Screenshots

Testing strategy

Tested on Windows, will be testing on other platforms to ensure no regression

Type of change

✅ New feature (change that adds functionality)
✅ Refactor (significant modification to existing code)

src/gui/DatabaseOpenWidget.cpp

michaelk83 · 2024-12-02T11:18:47Z

Remembering quick unlock is not supported on Linux due to the lack of a unified/standardized key store across distros.

There is the kernel keyring, which I think the existing Linux Quick Unlock code is already using. Not sure if it can store things across sessions, though.
https://man7.org/linux/man-pages/man2/add_key.2.html
https://man7.org/linux/man-pages/man7/persistent-keyring.7.html

droidmonkey · 2024-12-02T11:49:32Z

Those are not persistent credential storage. They get wiped on reboot or log off.

* Introduce QuickUnlockManager to fall back to pin unlock if OS native options are not available.

codecov · 2024-12-09T05:12:28Z

Codecov Report

Attention: Patch coverage is 28.94737% with 108 lines in your changes missing coverage. Please review.

Project coverage is 63.81%. Comparing base (f71cca4) to head (3766f17).

Files with missing lines	Patch %	Lines
src/quickunlock/PinUnlock.cpp	7.14%	78 Missing ⚠️
src/gui/DatabaseOpenWidget.cpp	63.89%	13 Missing ⚠️
src/quickunlock/Polkit.cpp	0.00%	11 Missing ⚠️
src/quickunlock/QuickUnlockInterface.cpp	66.67%	4 Missing ⚠️
src/gui/ApplicationSettingsWidget.cpp	66.67%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #11520      +/-   ##
===========================================
- Coverage    63.88%   63.81%   -0.07%     
===========================================
  Files          362      364       +2     
  Lines        38147    38233      +86     
===========================================
+ Hits         24368    24396      +28     
- Misses       13779    13837      +58

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

phoerious · 2025-01-04T20:39:23Z

src/quickunlock/PinUnlock.cpp

@@ -0,0 +1,171 @@
+/*
+ *  Copyright (C) 2023 KeePassXC Team <[email protected]>


phoerious · 2025-01-04T20:40:29Z

src/quickunlock/PinUnlock.cpp

+        pin = QInputDialog::getText(
+            nullptr,
+            QObject::tr("Quick Unlock Pin Entry"),
+            QObject::tr("Enter a %1 to %2 digit pin to use for quick unlock:").arg(MIN_PIN_LENGTH).arg(MAX_PIN_LENGTH),


There should be hyphens (%1- to %2-digit) and I guess PIN should be all-caps.

I prefer an en dash instead: 4–6 digit

That's an option, yes. But there should still be a hyphen between the number and "digit" (in addition to the en dash).

phoerious · 2025-01-04T20:42:54Z

src/quickunlock/PinUnlock.cpp

+    // Encrypt the data using AES-256-CBC
+    SymmetricCipher cipher;
+    if (!cipher.init(SymmetricCipher::Aes256_GCM, SymmetricCipher::Encrypt, key, iv)) {
+        m_error = QObject::tr("Failed to init KeePassXC crypto.");
+        return false;


Comment says CBC, but you're doing GCM.

Copy pasta, good catch, GCM is correct

phoerious · 2025-01-04T21:01:01Z

src/quickunlock/PinUnlock.cpp

+        auto pin = QInputDialog::getText(
+            nullptr,
+            QObject::tr("Quick Unlock Pin Entry"),
+            QObject::tr("Enter quick unlock pin (%1 of %2 attempts):").arg(pinAttempts).arg(MAX_PIN_ATTEMPTS),


phoerious · 2025-01-04T21:06:45Z

src/quickunlock/PinUnlock.cpp

+    }
+
+    data.clear();
+    m_error = QObject::tr("Maximum pin attempts have been reached.");


KISS: "Too many PIN attempts."

src/gui/DatabaseOpenDialog.cpp

phoerious · 2025-01-04T21:23:26Z

src/quickunlock/QuickUnlockInterface.cpp

+bool QuickUnlockManager::isRememberAvailable() const
 {
-    Q_UNUSED(dbUuid)
+    if (isNativeAvailable()) {
+        return m_nativeInterface->canRemember();
+    }
+    return m_fallbackInterface->canRemember();
 }


Looks like this isn't used anywhere?

Yah there was a purpose at some point but the settings shuffle made this unnecessary.

This would be needed for an explicit reset if the key cannot be remembered. We should definitely do that, even if it may technically not be neccessary with the way these are implemented now. We should not make any assumptions here whether an explicit reset is necessary or not.

phoerious · 2025-01-04T21:25:22Z

src/quickunlock/PinUnlock.cpp

+    QByteArray encrypted = data;
+    if (!cipher.finish(encrypted)) {
+        m_error = QObject::tr("Failed to encrypt key data.");
+        return false;
+    }
+
+    // Prepend the IV to the encrypted data
+    encrypted.prepend(iv);
+    // Store the encrypted data and pin attempts
+    m_encryptedKeys.insert(dbUuid, qMakePair(1, encrypted));


We should probably pad the serialised key to a specific length to avoid leaking length information. I'm in general only half happy with this whole thing, since the PIN attempt count matters only as long as KeePassXC stays in full control of the cipher text (at which point it might as well not be encrypted at all).

On the other hand, the entire database is in memory naked at this point, so none of this really matters probably.

At the very least, this map should be mlocked. All this applies to Polkit unlock as well.

Hmmm good idea, should maybe apply Argon2 to the pin hash to provide protection against brute force. Can just prepend the Argon2 parameters to the encrypted string.

I was thinking the same, though with a numerical four-digit PIN, Argon2 would add only very little security.

phoerious · 2025-01-04T21:29:39Z

src/quickunlock/PinUnlock.cpp

+        auto key = hash.result();
+
+        // Read the previously used challenge and encrypted data
+        auto ivSize = SymmetricCipher::defaultIvSize(SymmetricCipher::Aes256_GCM);


const auto.

phoerious · 2025-01-04T21:31:08Z

src/quickunlock/PinUnlock.h

@@ -0,0 +1,49 @@
+/*
+ *  Copyright (C) 2023 KeePassXC Team <[email protected]>


phoerious · 2025-01-04T21:35:05Z

src/gui/DatabaseOpenWidget.cpp

+DatabaseOpenWidget::~DatabaseOpenWidget()
+{
+    // Reset quick unlock if we are not remembering it
+    if (!config()->get(Config::Security_QuickUnlockRemember).toBool()) {
+        resetQuickUnlock();
+    }
+}


Also reset if remember isn't available (see above).

phoerious · 2025-01-04T21:43:06Z

src/gui/ApplicationSettingsWidgetSecurity.ui

+        <property name="text">
+         <string>Remember quick unlock after database is closed (Touch ID / Windows Hello only)</string>
        </property>


Remove the part in parentheses and show this option conditionally only when either of the two is available.

phoerious · 2025-01-04T21:44:30Z

I think the unlock screen layout got screwed up. There's way too much padding above "I have a key file". I also don't like the checkbox placement next to the Unlock button (why do we need that anyway, what's wrong with the way it was before?)

droidmonkey · 2025-01-04T21:59:48Z

I did tweak the layout of the unlock dialog, extra padding was on purpose, I felt it was too tight before. We can debate it.

The checkbox is necessary now so you can decide when to use Quick Unlock before unlocking. This matters on Linux and Mac where the user isn't prompted prior to setting up quick unlock (and also potentially remembering it). On Windows or when using PIN this doesn't matter because you can always cancel the Windows Hello or PIN dialog.

I am also open to changing the "Unlock" button to include an alternative drop down option of "Unlock Once" or "No Quick Unlock" or similar.

phoerious · 2025-01-04T22:05:36Z

The previous layout was deliberate. With the extra padding you're messing with the proximity and common region principles. The visual cohesion of related elements gets lost entirely and the element grouping looks loose and confusing. I'm strongly in favour of reverting the change.

I don't see why we need a checkbox here. If users don't want quick unlock, they can disable it in the settings. If they don't want quick unlock just now (probably a niche use case), a hybrid button dropdown could be an option. On the other hand, you could also just lock the database and then one-click cancel quick unlock (that said: why not add that as an option to the lock button instead of the unlock button?).

droidmonkey · 2025-01-04T22:19:01Z

I changed the application setting from "Enable Quick Unlock" to "Use Quick Unlock by default". Enabling the new setting checks the box when you open a database, otherwise the box is unchecked. Disabling quick unlock at the application level doesn't really make sense anymore. Are you disabling the OS version of quick unlock, fallback? What if you want to use it from some databases but not others? Most users want to use quick unlock and want it to be remembered (based on issue reports and feature requests).

Perhaps a compromise would be moving this checkbox to the database settings page and store the desire in database custom data that can be queried right after the database is unlocked before quick unlock is established? Then we can also potentially use a selection box: "Enable Quick Unlock (default)", "Use PIN unlock", "Disable Quick Unlock"

Some people hate OS integration so explicitly allowing PIN unlock could be a neat feature for them.

phoerious · 2025-01-04T22:23:16Z

I believe in critical environments you definitely don't want to have quick unlock. So there should be an easily-accessible feature to turn it off. It may make sense to have that on database level, but I would assume that more often than not the desire to turn it off depends more on the environment than on the database (e.g. have it on at home, but not on the laptop).

droidmonkey · 2025-01-04T22:23:59Z

Quick mockup

phoerious · 2025-01-04T22:28:56Z

Not a fan of that. "Use PIN unlock" doesn't make any sense to me. You're not unlocking with a PIN here. Only the next unlock will use a PIN. "Disable quick unlock" sounds like a permenant setting, but isn't. I'd rather add an option to the "lock" button to cancel quick unlock immediately. Arguably, that would still store the key temporarily in the system key chain, but that's probably not the biggest deal.

droidmonkey · 2025-01-04T22:31:39Z

I dunno if I actually did change the padding, here is a comparison of develop against this branch (develop on left):

phoerious · 2025-01-04T22:35:00Z

Then it got messed up somewhere else, need to bisect that. I don't have a develop build handy right now, but this is what it's supposed to look like. The contrast needs improving (that is actually pretty terrible), the padding doesn't. You could add a few pixels if you really feel the urge to, but your screenshot has way too much of a gap. Should be the same distance to the password field as the label above.

droidmonkey · 2025-01-04T22:43:57Z

It was me, another commit though: ba0fc3b #11321

phoerious · 2025-01-04T22:50:07Z

Yes, I just checked a snapshot build. The padding was indeed increased there.

jasperweiss · 2025-01-18T12:49:47Z

Correct me if I'm wrong but encrypting the memory with a pin hash seems like security theater. Even with 8 digits and a slow hash function this can be brute forced so quickly it's hardly an obstacle for an attacker who is able to read the memory contents.

Hardware backed credential storage with Windows Hello or TouchID provide meaningful protection for the key material so they're a proper substitute for constantly filling in the passphrase, whereas the pin based feature is really just a UI level protection which can already be achieved by locking your device when leaving it unattended.

I would at the very least make it a completely separate option from the hardware backed options and make it very clear to the user that it's purely a UI level protection. But without hardware backed storage options, I don't see the point to be honest. The pin pretends to be a middle ground between having to type in the passphrase and having it unlocked while in reality it doesn't provide meaningful protection and leaving it unlocked is much more convenient.

droidmonkey added feature: QuickUnlock pr: new feature Pull request that adds a new feature labels Dec 2, 2024

droidmonkey added this to the v2.8.0 milestone Dec 2, 2024

droidmonkey requested a review from phoerious December 2, 2024 04:55

github-advanced-security bot found potential problems Dec 2, 2024

View reviewed changes

src/gui/DatabaseOpenWidget.cpp Fixed Show fixed Hide fixed

droidmonkey added 2 commits December 7, 2024 16:47

Add support to remember quick unlock on Windows and macOS

0ee002d

Add Pin Quick Unlock option

8e4cba3

* Introduce QuickUnlockManager to fall back to pin unlock if OS native options are not available.

droidmonkey force-pushed the feature/remember-quickunlock branch from dfbec82 to 8e4cba3 Compare December 9, 2024 04:57

Fixup macos

3766f17

droidmonkey mentioned this pull request Dec 18, 2024

Always be able to use Mac's TouchID to unlock the database even the first time #11575

Open

phoerious requested changes Jan 4, 2025

View reviewed changes

phoerious reviewed Jan 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Pin Quick Unlock and Remember Quick Unlock for Win/Mac #11520

Add Pin Quick Unlock and Remember Quick Unlock for Win/Mac #11520

droidmonkey commented Dec 2, 2024 •

edited

Loading

michaelk83 commented Dec 2, 2024 •

edited

Loading

droidmonkey commented Dec 2, 2024

codecov bot commented Dec 9, 2024 •

edited

Loading

phoerious Jan 4, 2025

phoerious Jan 4, 2025

droidmonkey Jan 4, 2025 •

edited

Loading

phoerious Jan 4, 2025 •

edited

Loading

phoerious Jan 4, 2025

droidmonkey Jan 4, 2025 •

edited

Loading

phoerious Jan 4, 2025

phoerious Jan 4, 2025

phoerious Jan 4, 2025

droidmonkey Jan 4, 2025

phoerious Jan 4, 2025

phoerious Jan 4, 2025 •

edited

Loading

droidmonkey Jan 4, 2025 •

edited

Loading

phoerious Jan 4, 2025

phoerious Jan 4, 2025

phoerious Jan 4, 2025

phoerious Jan 4, 2025

phoerious Jan 4, 2025

phoerious commented Jan 4, 2025 •

edited

Loading

droidmonkey commented Jan 4, 2025 •

edited

Loading

phoerious commented Jan 4, 2025 •

edited

Loading

droidmonkey commented Jan 4, 2025 •

edited

Loading

phoerious commented Jan 4, 2025 •

edited

Loading

droidmonkey commented Jan 4, 2025 •

edited

Loading

phoerious commented Jan 4, 2025

droidmonkey commented Jan 4, 2025

phoerious commented Jan 4, 2025 •

edited

Loading

droidmonkey commented Jan 4, 2025 •

edited

Loading

phoerious commented Jan 4, 2025

jasperweiss commented Jan 18, 2025

		@@ -0,0 +1,171 @@
		/*
		* Copyright (C) 2023 KeePassXC Team <[email protected]>

		@@ -0,0 +1,49 @@
		/*
		* Copyright (C) 2023 KeePassXC Team <[email protected]>

Add Pin Quick Unlock and Remember Quick Unlock for Win/Mac #11520

Are you sure you want to change the base?

Add Pin Quick Unlock and Remember Quick Unlock for Win/Mac #11520

Conversation

droidmonkey commented Dec 2, 2024 • edited Loading

Screenshots

Testing strategy

Type of change

michaelk83 commented Dec 2, 2024 • edited Loading

droidmonkey commented Dec 2, 2024

codecov bot commented Dec 9, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droidmonkey Jan 4, 2025 • edited Loading

Choose a reason for hiding this comment

phoerious Jan 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droidmonkey Jan 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoerious Jan 4, 2025 • edited Loading

Choose a reason for hiding this comment

droidmonkey Jan 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoerious commented Jan 4, 2025 • edited Loading

droidmonkey commented Jan 4, 2025 • edited Loading

phoerious commented Jan 4, 2025 • edited Loading

droidmonkey commented Jan 4, 2025 • edited Loading

phoerious commented Jan 4, 2025 • edited Loading

droidmonkey commented Jan 4, 2025 • edited Loading

phoerious commented Jan 4, 2025

droidmonkey commented Jan 4, 2025

phoerious commented Jan 4, 2025 • edited Loading

droidmonkey commented Jan 4, 2025 • edited Loading

phoerious commented Jan 4, 2025

jasperweiss commented Jan 18, 2025

droidmonkey commented Dec 2, 2024 •

edited

Loading

michaelk83 commented Dec 2, 2024 •

edited

Loading

codecov bot commented Dec 9, 2024 •

edited

Loading

droidmonkey Jan 4, 2025 •

edited

Loading

phoerious Jan 4, 2025 •

edited

Loading

droidmonkey Jan 4, 2025 •

edited

Loading

phoerious Jan 4, 2025 •

edited

Loading

droidmonkey Jan 4, 2025 •

edited

Loading

phoerious commented Jan 4, 2025 •

edited

Loading

droidmonkey commented Jan 4, 2025 •

edited

Loading

phoerious commented Jan 4, 2025 •

edited

Loading

droidmonkey commented Jan 4, 2025 •

edited

Loading

phoerious commented Jan 4, 2025 •

edited

Loading

droidmonkey commented Jan 4, 2025 •

edited

Loading

phoerious commented Jan 4, 2025 •

edited

Loading

droidmonkey commented Jan 4, 2025 •

edited

Loading