Address PR comments

This commit is contained in:
Jonathan White
2025-07-12 12:47:46 -04:00
parent 4e2c06b943
commit 67b550bb6e
13 changed files with 86 additions and 73 deletions

View File

@@ -353,6 +353,9 @@ void ApplicationSettingsWidget::loadSettings()
#ifdef Q_OS_LINUX #ifdef Q_OS_LINUX
// Remembering quick unlock is not supported on Linux // Remembering quick unlock is not supported on Linux
m_secUi->quickUnlockRememberCheckBox->setVisible(false); 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 #endif
for (const ExtraPage& page : asConst(m_extraPages)) { for (const ExtraPage& page : asConst(m_extraPages)) {

View File

@@ -174,11 +174,8 @@
</item> </item>
<item> <item>
<widget class="QCheckBox" name="quickUnlockRememberCheckBox"> <widget class="QCheckBox" name="quickUnlockRememberCheckBox">
<property name="toolTip">
<string>Quick unlock can only be remembered when using Touch ID or Windows Hello</string>
</property>
<property name="text"> <property name="text">
<string>Remember quick unlock after database is closed (Touch ID / Windows Hello only)</string> <string>Remember quick unlock after database is closed</string>
</property> </property>
</widget> </widget>
</item> </item>

View File

@@ -484,7 +484,7 @@
<enum>Qt::RightToLeft</enum> <enum>Qt::RightToLeft</enum>
</property> </property>
<property name="text"> <property name="text">
<string>Enable Quick Unlock</string> <string>Quick Unlock</string>
</property> </property>
<property name="checked"> <property name="checked">
<bool>true</bool> <bool>true</bool>
@@ -609,7 +609,7 @@
<property name="sizeHint" stdset="0"> <property name="sizeHint" stdset="0">
<size> <size>
<width>20</width> <width>20</width>
<height>8</height> <height>4</height>
</size> </size>
</property> </property>
</spacer> </spacer>
@@ -621,6 +621,12 @@
</property> </property>
<item> <item>
<widget class="QPushButton" name="resetQuickUnlockButton"> <widget class="QPushButton" name="resetQuickUnlockButton">
<property name="minimumSize">
<size>
<width>0</width>
<height>20</height>
</size>
</property>
<property name="text"> <property name="text">
<string>Reset</string> <string>Reset</string>
</property> </property>
@@ -636,7 +642,7 @@
</property> </property>
<property name="sizeHint" stdset="0"> <property name="sizeHint" stdset="0">
<size> <size>
<width>6</width> <width>4</width>
<height>20</height> <height>20</height>
</size> </size>
</property> </property>
@@ -644,6 +650,12 @@
</item> </item>
<item> <item>
<widget class="QPushButton" name="closeQuickUnlockButton"> <widget class="QPushButton" name="closeQuickUnlockButton">
<property name="minimumSize">
<size>
<width>0</width>
<height>20</height>
</size>
</property>
<property name="text"> <property name="text">
<string>Close Database</string> <string>Close Database</string>
</property> </property>
@@ -742,7 +754,6 @@
</customwidget> </customwidget>
</customwidgets> </customwidgets>
<tabstops> <tabstops>
<tabstop>quickUnlockButton</tabstop>
<tabstop>editPassword</tabstop> <tabstop>editPassword</tabstop>
<tabstop>keyFileLineEdit</tabstop> <tabstop>keyFileLineEdit</tabstop>
<tabstop>buttonBrowseFile</tabstop> <tabstop>buttonBrowseFile</tabstop>
@@ -750,7 +761,11 @@
<tabstop>hardwareKeyCombo</tabstop> <tabstop>hardwareKeyCombo</tabstop>
<tabstop>refreshHardwareKeys</tabstop> <tabstop>refreshHardwareKeys</tabstop>
<tabstop>addKeyFileLinkLabel</tabstop> <tabstop>addKeyFileLinkLabel</tabstop>
<tabstop>enableQuickUnlockCheckBox</tabstop>
<tabstop>buttonBox</tabstop> <tabstop>buttonBox</tabstop>
<tabstop>quickUnlockButton</tabstop>
<tabstop>resetQuickUnlockButton</tabstop>
<tabstop>closeQuickUnlockButton</tabstop>
</tabstops> </tabstops>
<resources/> <resources/>
<connections/> <connections/>

View File

@@ -1,5 +1,5 @@
/* /*
* Copyright (C) 2023 KeePassXC Team <team@keepassxc.org> * Copyright (C) 2025 KeePassXC Team <team@keepassxc.org>
* *
* This program is free software: you can redistribute it and/or modify * 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 * it under the terms of the GNU General Public License as published by
@@ -20,13 +20,17 @@
#include "crypto/CryptoHash.h" #include "crypto/CryptoHash.h"
#include "crypto/Random.h" #include "crypto/Random.h"
#include "crypto/SymmetricCipher.h" #include "crypto/SymmetricCipher.h"
#include "crypto/kdf/Argon2Kdf.h"
#include <QInputDialog> #include <QInputDialog>
#include <QRegularExpression> #include <QRegularExpression>
#define MIN_PIN_LENGTH 4 namespace
#define MAX_PIN_LENGTH 8 {
#define MAX_PIN_ATTEMPTS 3 constexpr int MIN_PIN_LENGTH = 6;
constexpr int MAX_PIN_LENGTH = 10;
constexpr int MAX_PIN_ATTEMPTS = 3;
} // namespace
bool PinUnlock::isAvailable() const bool PinUnlock::isAvailable() const
{ {
@@ -47,7 +51,7 @@ bool PinUnlock::setKey(const QUuid& dbUuid, const QByteArray& data)
pin = QInputDialog::getText( pin = QInputDialog::getText(
nullptr, nullptr,
QObject::tr("Quick Unlock Pin Entry"), 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, QLineEdit::Password,
{}, {},
&ok); &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); CryptoHash hash(CryptoHash::Sha256);
hash.addData(pin.toLatin1()); 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 // 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; SymmetricCipher cipher;
if (!cipher.init(SymmetricCipher::Aes256_GCM, SymmetricCipher::Encrypt, key, iv)) { if (!cipher.init(SymmetricCipher::Aes256_GCM, SymmetricCipher::Encrypt, key, iv)) {
m_error = QObject::tr("Failed to init KeePassXC crypto."); m_error = QObject::tr("Failed to init KeePassXC crypto.");
@@ -117,13 +126,18 @@ bool PinUnlock::getKey(const QUuid& dbUuid, QByteArray& data)
return false; 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); CryptoHash hash(CryptoHash::Sha256);
hash.addData(pin.toLatin1()); 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 // 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; const auto& keydata = pairData.second;
auto challenge = keydata.left(ivSize); auto challenge = keydata.left(ivSize);
auto encrypted = keydata.mid(ivSize); auto encrypted = keydata.mid(ivSize);
@@ -145,7 +159,7 @@ bool PinUnlock::getKey(const QUuid& dbUuid, QByteArray& data)
} }
data.clear(); data.clear();
m_error = QObject::tr("Maximum pin attempts have been reached."); m_error = QObject::tr("Too many pin attempts.");
reset(dbUuid); reset(dbUuid);
return false; return false;
} }
@@ -155,11 +169,6 @@ bool PinUnlock::hasKey(const QUuid& dbUuid) const
return m_encryptedKeys.contains(dbUuid); return m_encryptedKeys.contains(dbUuid);
} }
bool PinUnlock::canRemember() const
{
return false;
}
void PinUnlock::reset(const QUuid& dbUuid) void PinUnlock::reset(const QUuid& dbUuid)
{ {
m_encryptedKeys.remove(dbUuid); m_encryptedKeys.remove(dbUuid);

View File

@@ -1,5 +1,5 @@
/* /*
* Copyright (C) 2023 KeePassXC Team <team@keepassxc.org> * Copyright (C) 2025 KeePassXC Team <team@keepassxc.org>
* *
* This program is free software: you can redistribute it and/or modify * 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 * 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 getKey(const QUuid& dbUuid, QByteArray& key) override;
bool hasKey(const QUuid& dbUuid) const override; bool hasKey(const QUuid& dbUuid) const override;
bool canRemember() const override;
void reset(const QUuid& dbUuid) override; void reset(const QUuid& dbUuid) override;
void reset() override; void reset() override;

View File

@@ -237,11 +237,6 @@ bool Polkit::getKey(const QUuid& dbUuid, QByteArray& key)
return false; return false;
} }
bool Polkit::canRemember() const
{
return false;
}
bool Polkit::hasKey(const QUuid& dbUuid) const bool Polkit::hasKey(const QUuid& dbUuid) const
{ {
if (!m_encryptedMasterKeys.contains(dbUuid)) { if (!m_encryptedMasterKeys.contains(dbUuid)) {

View File

@@ -36,8 +36,6 @@ public:
bool getKey(const QUuid& dbUuid, QByteArray& key) override; bool getKey(const QUuid& dbUuid, QByteArray& key) override;
bool hasKey(const QUuid& dbUuid) const override; bool hasKey(const QUuid& dbUuid) const override;
bool canRemember() const override;
void reset(const QUuid& dbUuid) override; void reset(const QUuid& dbUuid) override;
void reset() override; void reset() override;

View File

@@ -68,11 +68,3 @@ bool QuickUnlockManager::isNativeAvailable() const
{ {
return m_nativeInterface && m_nativeInterface->isAvailable(); return m_nativeInterface && m_nativeInterface->isAvailable();
} }
bool QuickUnlockManager::isRememberAvailable() const
{
if (isNativeAvailable()) {
return m_nativeInterface->canRemember();
}
return m_fallbackInterface->canRemember();
}

View File

@@ -36,8 +36,6 @@ public:
virtual bool getKey(const QUuid& dbUuid, QByteArray& key) = 0; virtual bool getKey(const QUuid& dbUuid, QByteArray& key) = 0;
virtual bool hasKey(const QUuid& dbUuid) const = 0; virtual bool hasKey(const QUuid& dbUuid) const = 0;
virtual bool canRemember() const = 0;
virtual void reset(const QUuid& dbUuid) = 0; virtual void reset(const QUuid& dbUuid) = 0;
virtual void reset() = 0; virtual void reset() = 0;
}; };
@@ -52,7 +50,6 @@ public:
QSharedPointer<QuickUnlockInterface> interface() const; QSharedPointer<QuickUnlockInterface> interface() const;
bool isNativeAvailable() const; bool isNativeAvailable() const;
bool isRememberAvailable() const;
private: private:
QSharedPointer<QuickUnlockInterface> m_nativeInterface; QSharedPointer<QuickUnlockInterface> m_nativeInterface;

View File

@@ -31,8 +31,6 @@ public:
bool getKey(const QUuid& dbUuid, QByteArray& passwordKey) override; bool getKey(const QUuid& dbUuid, QByteArray& passwordKey) override;
bool hasKey(const QUuid& dbUuid) const override; bool hasKey(const QUuid& dbUuid) const override;
bool canRemember() const override;
void reset(const QUuid& dbUuid = "") override; void reset(const QUuid& dbUuid = "") override;
void reset() override; void reset() override;
@@ -44,8 +42,6 @@ private:
static void deleteKeyEntry(const QString& accountName); static void deleteKeyEntry(const QString& accountName);
static QString databaseKeyName(const QUuid& dbUuid); static QString databaseKeyName(const QUuid& dbUuid);
QHash<QUuid, QByteArray> m_encryptedMasterKeys;
}; };
#endif // KEEPASSX_TOUCHID_H #endif // KEEPASSX_TOUCHID_H

View File

@@ -25,6 +25,8 @@ inline void debug(const char *message, ...)
} }
#endif #endif
static const auto s_touchIdKeyPrefix = QStringLiteral("KeepassXC_TouchID_Keys_");
inline std::string StatusToErrorMessage(OSStatus status) inline std::string StatusToErrorMessage(OSStatus status)
{ {
CFStringRef text = SecCopyErrorMessageString(status, NULL); CFStringRef text = SecCopyErrorMessageString(status, NULL);
@@ -73,8 +75,7 @@ void TouchID::deleteKeyEntry(const QString& accountName)
QString TouchID::databaseKeyName(const QUuid& dbUuid) QString TouchID::databaseKeyName(const QUuid& dbUuid)
{ {
static const QString keyPrefix = "KeepassXC_TouchID_Keys_"; return s_touchIdKeyPrefix + dbUuid.toString();
return keyPrefix + dbUuid.toString();
} }
QString TouchID::errorString() const QString TouchID::errorString() const
@@ -85,8 +86,35 @@ QString TouchID::errorString() const
void TouchID::reset() void TouchID::reset()
{ {
// TODO: Clear all credentials associated with KeePassXC // Query for all generic password items
m_encryptedMasterKeys.clear(); 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; 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!"); debug("TouchID::setKey - Success!");
return true; return true;
} }
@@ -363,11 +388,6 @@ bool TouchID::isAvailable() const
return isWatchAvailable() || isTouchIdAvailable() || isPasswordFallbackPossible(); return isWatchAvailable() || isTouchIdAvailable() || isPasswordFallbackPossible();
} }
bool TouchID::canRemember() const
{
return true;
}
/** /**
* Resets the inner state either for all or for the given database * Resets the inner state either for all or for the given database
*/ */

View File

@@ -112,7 +112,7 @@ namespace
{ {
try { try {
auto vault = PasswordVault(); 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) { } catch (winrt::hresult_error const& ex) {
} }
} }
@@ -234,11 +234,6 @@ bool WindowsHello::hasKey(const QUuid& dbUuid) const
return !loadCredential(dbUuid).isEmpty(); return !loadCredential(dbUuid).isEmpty();
} }
bool WindowsHello::canRemember() const
{
return true;
}
void WindowsHello::reset() void WindowsHello::reset()
{ {
resetCredentials(); resetCredentials();

View File

@@ -32,8 +32,6 @@ public:
bool getKey(const QUuid& dbUuid, QByteArray& key) override; bool getKey(const QUuid& dbUuid, QByteArray& key) override;
bool hasKey(const QUuid& dbUuid) const override; bool hasKey(const QUuid& dbUuid) const override;
bool canRemember() const override;
void reset(const QUuid& dbUuid) override; void reset(const QUuid& dbUuid) override;
void reset() override; void reset() override;