From 67b550bb6e99195bd29c66b1f3cf83660f40bcac Mon Sep 17 00:00:00 2001 From: Jonathan White Date: Sat, 12 Jul 2025 12:47:46 -0400 Subject: [PATCH] Address PR comments --- src/gui/ApplicationSettingsWidget.cpp | 3 ++ src/gui/ApplicationSettingsWidgetSecurity.ui | 5 +-- src/gui/DatabaseOpenWidget.ui | 23 ++++++++-- src/quickunlock/PinUnlock.cpp | 47 ++++++++++++-------- src/quickunlock/PinUnlock.h | 6 +-- src/quickunlock/Polkit.cpp | 5 --- src/quickunlock/Polkit.h | 2 - src/quickunlock/QuickUnlockInterface.cpp | 8 ---- src/quickunlock/QuickUnlockInterface.h | 3 -- src/quickunlock/TouchID.h | 4 -- src/quickunlock/TouchID.mm | 44 +++++++++++++----- src/quickunlock/WindowsHello.cpp | 7 +-- src/quickunlock/WindowsHello.h | 2 - 13 files changed, 86 insertions(+), 73 deletions(-) diff --git a/src/gui/ApplicationSettingsWidget.cpp b/src/gui/ApplicationSettingsWidget.cpp index aa3fa2471..3f1f91cad 100644 --- a/src/gui/ApplicationSettingsWidget.cpp +++ b/src/gui/ApplicationSettingsWidget.cpp @@ -353,6 +353,9 @@ void ApplicationSettingsWidget::loadSettings() #ifdef Q_OS_LINUX // Remembering quick unlock is not supported on Linux m_secUi->quickUnlockRememberCheckBox->setVisible(false); +#else + // Only show this option if Touch ID or Windows Hello are available for use + m_secUi->quickUnlockRememberCheckBox->setVisible(getQuickUnlock()->isNativeAvailable()); #endif for (const ExtraPage& page : asConst(m_extraPages)) { diff --git a/src/gui/ApplicationSettingsWidgetSecurity.ui b/src/gui/ApplicationSettingsWidgetSecurity.ui index f708995a5..56a6f7343 100644 --- a/src/gui/ApplicationSettingsWidgetSecurity.ui +++ b/src/gui/ApplicationSettingsWidgetSecurity.ui @@ -174,11 +174,8 @@ - - Quick unlock can only be remembered when using Touch ID or Windows Hello - - Remember quick unlock after database is closed (Touch ID / Windows Hello only) + Remember quick unlock after database is closed diff --git a/src/gui/DatabaseOpenWidget.ui b/src/gui/DatabaseOpenWidget.ui index eef43dd27..e479bcac3 100644 --- a/src/gui/DatabaseOpenWidget.ui +++ b/src/gui/DatabaseOpenWidget.ui @@ -484,7 +484,7 @@ Qt::RightToLeft - Enable Quick Unlock + Quick Unlock true @@ -609,7 +609,7 @@ 20 - 8 + 4 @@ -621,6 +621,12 @@ + + + 0 + 20 + + Reset @@ -636,7 +642,7 @@ - 6 + 4 20 @@ -644,6 +650,12 @@ + + + 0 + 20 + + Close Database @@ -742,7 +754,6 @@ - quickUnlockButton editPassword keyFileLineEdit buttonBrowseFile @@ -750,7 +761,11 @@ hardwareKeyCombo refreshHardwareKeys addKeyFileLinkLabel + enableQuickUnlockCheckBox buttonBox + quickUnlockButton + resetQuickUnlockButton + closeQuickUnlockButton diff --git a/src/quickunlock/PinUnlock.cpp b/src/quickunlock/PinUnlock.cpp index 54d7ef1ee..bcf48defb 100644 --- a/src/quickunlock/PinUnlock.cpp +++ b/src/quickunlock/PinUnlock.cpp @@ -1,5 +1,5 @@ -/* - * Copyright (C) 2023 KeePassXC Team +/* + * Copyright (C) 2025 KeePassXC Team * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -20,13 +20,17 @@ #include "crypto/CryptoHash.h" #include "crypto/Random.h" #include "crypto/SymmetricCipher.h" +#include "crypto/kdf/Argon2Kdf.h" #include #include -#define MIN_PIN_LENGTH 4 -#define MAX_PIN_LENGTH 8 -#define MAX_PIN_ATTEMPTS 3 +namespace +{ + constexpr int MIN_PIN_LENGTH = 6; + constexpr int MAX_PIN_LENGTH = 10; + constexpr int MAX_PIN_ATTEMPTS = 3; +} // namespace bool PinUnlock::isAvailable() const { @@ -47,7 +51,7 @@ bool PinUnlock::setKey(const QUuid& dbUuid, const QByteArray& data) 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), + QObject::tr("Enter a %1–%2 digit pin to use for quick unlock:").arg(MIN_PIN_LENGTH).arg(MAX_PIN_LENGTH), QLineEdit::Password, {}, &ok); @@ -63,15 +67,20 @@ bool PinUnlock::setKey(const QUuid& dbUuid, const QByteArray& data) } } - // Hash the pin and use it as the key for the encryption + // Hash the pin then run it through Argon2 to derive the encryption key + QByteArray key(32, '\0'); + Argon2Kdf kdf(Argon2Kdf::Type::Argon2id); CryptoHash hash(CryptoHash::Sha256); hash.addData(pin.toLatin1()); - auto key = hash.result(); + if (!kdf.transform(hash.result(), key)) { + m_error = QObject::tr("Failed to derive key using Argon2"); + return false; + } // Generate a random IV - auto iv = Random::instance()->randomArray(SymmetricCipher::defaultIvSize(SymmetricCipher::Aes256_GCM)); + const auto iv = Random::instance()->randomArray(SymmetricCipher::defaultIvSize(SymmetricCipher::Aes256_GCM)); - // Encrypt the data using AES-256-CBC + // Encrypt the data using AES-256-GCM SymmetricCipher cipher; if (!cipher.init(SymmetricCipher::Aes256_GCM, SymmetricCipher::Encrypt, key, iv)) { m_error = QObject::tr("Failed to init KeePassXC crypto."); @@ -117,13 +126,18 @@ bool PinUnlock::getKey(const QUuid& dbUuid, QByteArray& data) return false; } - // Hash the pin and use it as the key for the encryption + // Hash the pin then run it through Argon2 to derive the encryption key + QByteArray key(32, '\0'); + Argon2Kdf kdf(Argon2Kdf::Type::Argon2id); CryptoHash hash(CryptoHash::Sha256); hash.addData(pin.toLatin1()); - auto key = hash.result(); + if (!kdf.transform(hash.result(), key)) { + m_error = QObject::tr("Failed to derive key using Argon2"); + return false; + } // Read the previously used challenge and encrypted data - auto ivSize = SymmetricCipher::defaultIvSize(SymmetricCipher::Aes256_GCM); + const auto ivSize = SymmetricCipher::defaultIvSize(SymmetricCipher::Aes256_GCM); const auto& keydata = pairData.second; auto challenge = keydata.left(ivSize); auto encrypted = keydata.mid(ivSize); @@ -145,7 +159,7 @@ bool PinUnlock::getKey(const QUuid& dbUuid, QByteArray& data) } data.clear(); - m_error = QObject::tr("Maximum pin attempts have been reached."); + m_error = QObject::tr("Too many pin attempts."); reset(dbUuid); return false; } @@ -155,11 +169,6 @@ bool PinUnlock::hasKey(const QUuid& dbUuid) const return m_encryptedKeys.contains(dbUuid); } -bool PinUnlock::canRemember() const -{ - return false; -} - void PinUnlock::reset(const QUuid& dbUuid) { m_encryptedKeys.remove(dbUuid); diff --git a/src/quickunlock/PinUnlock.h b/src/quickunlock/PinUnlock.h index c285ad8e9..acae32eeb 100644 --- a/src/quickunlock/PinUnlock.h +++ b/src/quickunlock/PinUnlock.h @@ -1,5 +1,5 @@ -/* - * Copyright (C) 2023 KeePassXC Team +/* + * Copyright (C) 2025 KeePassXC Team * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -34,8 +34,6 @@ public: bool getKey(const QUuid& dbUuid, QByteArray& key) override; bool hasKey(const QUuid& dbUuid) const override; - bool canRemember() const override; - void reset(const QUuid& dbUuid) override; void reset() override; diff --git a/src/quickunlock/Polkit.cpp b/src/quickunlock/Polkit.cpp index 740ab12a8..c9fa5f75c 100644 --- a/src/quickunlock/Polkit.cpp +++ b/src/quickunlock/Polkit.cpp @@ -237,11 +237,6 @@ bool Polkit::getKey(const QUuid& dbUuid, QByteArray& key) return false; } -bool Polkit::canRemember() const -{ - return false; -} - bool Polkit::hasKey(const QUuid& dbUuid) const { if (!m_encryptedMasterKeys.contains(dbUuid)) { diff --git a/src/quickunlock/Polkit.h b/src/quickunlock/Polkit.h index 2dd8bbf6c..7dfc2db7b 100644 --- a/src/quickunlock/Polkit.h +++ b/src/quickunlock/Polkit.h @@ -36,8 +36,6 @@ public: bool getKey(const QUuid& dbUuid, QByteArray& key) override; bool hasKey(const QUuid& dbUuid) const override; - bool canRemember() const override; - void reset(const QUuid& dbUuid) override; void reset() override; diff --git a/src/quickunlock/QuickUnlockInterface.cpp b/src/quickunlock/QuickUnlockInterface.cpp index 2e55dd41f..bd365ed28 100644 --- a/src/quickunlock/QuickUnlockInterface.cpp +++ b/src/quickunlock/QuickUnlockInterface.cpp @@ -68,11 +68,3 @@ bool QuickUnlockManager::isNativeAvailable() const { return m_nativeInterface && m_nativeInterface->isAvailable(); } - -bool QuickUnlockManager::isRememberAvailable() const -{ - if (isNativeAvailable()) { - return m_nativeInterface->canRemember(); - } - return m_fallbackInterface->canRemember(); -} diff --git a/src/quickunlock/QuickUnlockInterface.h b/src/quickunlock/QuickUnlockInterface.h index df55c01a8..6a999ac2a 100644 --- a/src/quickunlock/QuickUnlockInterface.h +++ b/src/quickunlock/QuickUnlockInterface.h @@ -36,8 +36,6 @@ public: virtual bool getKey(const QUuid& dbUuid, QByteArray& key) = 0; virtual bool hasKey(const QUuid& dbUuid) const = 0; - virtual bool canRemember() const = 0; - virtual void reset(const QUuid& dbUuid) = 0; virtual void reset() = 0; }; @@ -52,7 +50,6 @@ public: QSharedPointer interface() const; bool isNativeAvailable() const; - bool isRememberAvailable() const; private: QSharedPointer m_nativeInterface; diff --git a/src/quickunlock/TouchID.h b/src/quickunlock/TouchID.h index ef4f5ebf3..ce73e87f9 100644 --- a/src/quickunlock/TouchID.h +++ b/src/quickunlock/TouchID.h @@ -31,8 +31,6 @@ public: bool getKey(const QUuid& dbUuid, QByteArray& passwordKey) override; bool hasKey(const QUuid& dbUuid) const override; - bool canRemember() const override; - void reset(const QUuid& dbUuid = "") override; void reset() override; @@ -44,8 +42,6 @@ private: static void deleteKeyEntry(const QString& accountName); static QString databaseKeyName(const QUuid& dbUuid); - - QHash m_encryptedMasterKeys; }; #endif // KEEPASSX_TOUCHID_H diff --git a/src/quickunlock/TouchID.mm b/src/quickunlock/TouchID.mm index 60aa45cb7..d2e6a88ba 100644 --- a/src/quickunlock/TouchID.mm +++ b/src/quickunlock/TouchID.mm @@ -25,6 +25,8 @@ inline void debug(const char *message, ...) } #endif +static const auto s_touchIdKeyPrefix = QStringLiteral("KeepassXC_TouchID_Keys_"); + inline std::string StatusToErrorMessage(OSStatus status) { CFStringRef text = SecCopyErrorMessageString(status, NULL); @@ -73,8 +75,7 @@ void TouchID::deleteKeyEntry(const QString& accountName) QString TouchID::databaseKeyName(const QUuid& dbUuid) { - static const QString keyPrefix = "KeepassXC_TouchID_Keys_"; - return keyPrefix + dbUuid.toString(); + return s_touchIdKeyPrefix + dbUuid.toString(); } QString TouchID::errorString() const @@ -85,8 +86,35 @@ QString TouchID::errorString() const void TouchID::reset() { - // TODO: Clear all credentials associated with KeePassXC - m_encryptedMasterKeys.clear(); + // Query for all generic password items + CFMutableDictionaryRef query = makeDictionary(); + CFDictionarySetValue(query, kSecClass, kSecClassGenericPassword); + CFDictionarySetValue(query, kSecReturnAttributes, kCFBooleanTrue); + CFDictionarySetValue(query, kSecMatchLimit, kSecMatchLimitAll); + + CFTypeRef result = nullptr; + OSStatus status = SecItemCopyMatching(query, &result); + if (status != errSecSuccess || !result) { + LogStatusError("TouchID::deleteAllKeyEntriesWithPrefix - Error querying keychain", status); + CFRelease(query); + return; + } + + NSArray* items = (__bridge NSArray*)result; + for (NSDictionary* item in items) { + NSString* account = item[(id)kSecAttrAccount]; + if (account && [account hasPrefix:s_touchIdKeyPrefix.toNSString()]) { + // Build a query to delete this item + CFMutableDictionaryRef delQuery = makeDictionary(); + CFDictionarySetValue(delQuery, kSecClass, kSecClassGenericPassword); + CFDictionarySetValue(delQuery, kSecAttrAccount, (__bridge CFStringRef)account); + OSStatus delStatus = SecItemDelete(delQuery); + LogStatusError("TouchID::deleteAllKeyEntriesWithPrefix - Error deleting item", delStatus); + CFRelease(delQuery); + } + } + CFRelease(result); + CFRelease(query); } /** @@ -184,9 +212,6 @@ bool TouchID::setKey(const QUuid& dbUuid, const QByteArray& key, const bool igno return false; } - // memorize which database the stored key is for - // TODO: Do we need to store the db uuid's to do a full reset later? - //m_encryptedMasterKeys.insert(dbUuid, encryptedMasterKey); debug("TouchID::setKey - Success!"); return true; } @@ -363,11 +388,6 @@ bool TouchID::isAvailable() const return isWatchAvailable() || isTouchIdAvailable() || isPasswordFallbackPossible(); } -bool TouchID::canRemember() const -{ - return true; -} - /** * Resets the inner state either for all or for the given database */ diff --git a/src/quickunlock/WindowsHello.cpp b/src/quickunlock/WindowsHello.cpp index b1d7c94b2..7290ac73c 100644 --- a/src/quickunlock/WindowsHello.cpp +++ b/src/quickunlock/WindowsHello.cpp @@ -112,7 +112,7 @@ namespace { try { auto vault = PasswordVault(); - vault.Remove({s_winHelloKeyName, winrt::to_hstring(uuid.toString().toStdString()), L"blah"}); + vault.Remove({s_winHelloKeyName, winrt::to_hstring(uuid.toString().toStdString()), L"nodata"}); } catch (winrt::hresult_error const& ex) { } } @@ -234,11 +234,6 @@ bool WindowsHello::hasKey(const QUuid& dbUuid) const return !loadCredential(dbUuid).isEmpty(); } -bool WindowsHello::canRemember() const -{ - return true; -} - void WindowsHello::reset() { resetCredentials(); diff --git a/src/quickunlock/WindowsHello.h b/src/quickunlock/WindowsHello.h index 67f504bb2..0f6008590 100644 --- a/src/quickunlock/WindowsHello.h +++ b/src/quickunlock/WindowsHello.h @@ -32,8 +32,6 @@ public: bool getKey(const QUuid& dbUuid, QByteArray& key) override; bool hasKey(const QUuid& dbUuid) const override; - bool canRemember() const override; - void reset(const QUuid& dbUuid) override; void reset() override;