Significantly enhance hardware key robustness

* Significantly improve user experience when using hardware keys on databases in both GUI and CLI modes. Prevent locking up the YubiKey USB interface for prolonged periods of time. Allows for other apps to use the key concurrently with KeePassXC. * Improve messages displayed to user when finding keys and when user interaction is required. Output specific error messages when handling hardware keys during database read/write. * Only poll for keys when previously used or upon user request. Prevent continuously polling keys when accessing the UI such as switching tabs and minimize/maximize. * Add support for using multiple hardware keys simultaneously. Keys are identified by their serial number which prevents using the wrong key during open and save operations. * Fixes #4400 * Fixes #4065 * Fixes #1050 * Fixes #1215 * Fixes #3087 * Fixes #1088 * Fixes #1869
keepassxreboot · May 11, 2020 · 64367d2 · 64367d2
1 parent dcff507
commit 64367d2
Show file tree

Hide file tree

Showing 32 changed files with 664 additions and 665 deletions.
diff --git a/src/cli/Command.cpp b/src/cli/Command.cpp
@@ -19,6 +19,7 @@
 #include <cstdlib>
 #include <utility>
 
+#include <QFileInfo>
 #include <QMap>
 
 #include "Command.h"
@@ -72,8 +73,8 @@ const QCommandLineOption Command::NoPasswordOption =
 const QCommandLineOption Command::YubiKeyOption =
     QCommandLineOption(QStringList() << "y"
                                      << "yubikey",
-                       QObject::tr("Yubikey slot used to encrypt the database."),
-                       QObject::tr("slot"));
+                       QObject::tr("Yubikey slot and optional serial used to access the database (e.g., 1:7370001)."),
+                       QObject::tr("slot[:serial]"));
 
 namespace
 {
@@ -121,7 +122,15 @@ QString Command::getDescriptionLine()
 
 QString Command::getHelpText()
 {
-    return buildParser(this)->helpText().replace("[options]", name + " [options]");
+    auto help = buildParser(this)->helpText();
+    // Fix spacing of options parameter
+    help.replace(QStringLiteral("[options]"), name + QStringLiteral(" [options]"));
+    // Remove application directory from command line example
+    auto appname = QFileInfo(QCoreApplication::applicationFilePath()).fileName();
+    auto regex = QRegularExpression(QStringLiteral(" .*%1").arg(QRegularExpression::escape(appname)));
+    help.replace(regex, appname.prepend(" "));
+
+    return help;
 }
 
 QSharedPointer<QCommandLineParser> Command::getCommandLineParser(const QStringList& arguments)

diff --git a/src/cli/Utils.cpp b/src/cli/Utils.cpp
@@ -17,6 +17,10 @@
 
 #include "Utils.h"
 
+#ifdef WITH_XC_YUBIKEY
+#include "keys/YkChallengeResponseKeyCLI.h"
+#endif
+
 #ifdef Q_OS_WIN
 #include <windows.h>
 #else
@@ -159,25 +163,32 @@ namespace Utils
 
 #ifdef WITH_XC_YUBIKEY
         if (!yubiKeySlot.isEmpty()) {
+            unsigned int serial = 0;
+            int slot;
+
             bool ok = false;
-            int slot = yubiKeySlot.toInt(&ok, 10);
+            auto parts = yubiKeySlot.split(":");
+            slot = parts[0].toInt(&ok);
+
             if (!ok || (slot != 1 && slot != 2)) {
-                err << QObject::tr("Invalid YubiKey slot %1").arg(yubiKeySlot) << endl;
+                err << QObject::tr("Invalid YubiKey slot %1").arg(parts[0]) << endl;
                 return {};
             }
 
-            QString errorMessage;
-            bool blocking = YubiKey::instance()->checkSlotIsBlocking(slot, errorMessage);
-            if (!errorMessage.isEmpty()) {
-                err << errorMessage << endl;
-                return {};
+            if (parts.size() > 1) {
+                serial = parts[1].toUInt(&ok, 10);
+                if (!ok) {
+                    err << QObject::tr("Invalid YubiKey serial %1").arg(parts[1]) << endl;
+                    return {};
+                }
             }
 
-            auto key = QSharedPointer<YkChallengeResponseKeyCLI>(new YkChallengeResponseKeyCLI(
-                slot,
-                blocking,
-                QObject::tr("Please touch the button on your YubiKey to unlock %1").arg(databaseFilename),
-                outputDescriptor));
+            // clang-format off
+            auto key = QSharedPointer<YkChallengeResponseKeyCLI>(
+                new YkChallengeResponseKeyCLI({serial, slot},
+                                              QObject::tr("Please touch the button on your YubiKey to continue…"),
+                                              errorDescriptor));
+            // clang-format on
             compositeKey->addChallengeResponseKey(key);
         }
 #else

diff --git a/src/cli/Utils.h b/src/cli/Utils.h
@@ -26,12 +26,6 @@
 #include "keys/PasswordKey.h"
 #include <QtCore/qglobal.h>
 
-#ifdef WITH_XC_YUBIKEY
-#include "keys/YkChallengeResponseKey.h"
-#include "keys/YkChallengeResponseKeyCLI.h"
-#include "keys/drivers/YubiKey.h"
-#endif
-
 namespace Utils
 {
     extern FILE* STDOUT;

diff --git a/src/core/Alloc.cpp b/src/core/Alloc.cpp
@@ -79,6 +79,8 @@ void operator delete[](void* ptr) noexcept
     ::operator delete(ptr);
 }
 
+// clang-format versions less than 10.0 refuse to put a space before "noexcept"
+// clang-format off
 /**
  * Custom insecure delete operator that does not zero out memory before
  * freeing a buffer. Can be used for better performance.
@@ -87,6 +89,7 @@ void operator delete(void* ptr, bool) noexcept
 {
     std::free(ptr);
 }
+// clang-format on
 
 void operator delete[](void* ptr, bool) noexcept
 {

diff --git a/src/core/Database.cpp b/src/core/Database.cpp
@@ -659,10 +659,11 @@ QByteArray Database::challengeResponseKey() const
 
 bool Database::challengeMasterSeed(const QByteArray& masterSeed)
 {
+    m_keyError.clear();
     if (m_data.key) {
         m_data.masterSeed->setHash(masterSeed);
         QByteArray response;
-        bool ok = m_data.key->challenge(masterSeed, response);
+        bool ok = m_data.key->challenge(masterSeed, response, &m_keyError);
         if (ok && !response.isEmpty()) {
             m_data.challengeResponseKey->setHash(response);
         } else if (ok && response.isEmpty()) {
@@ -703,6 +704,7 @@ bool Database::setKey(const QSharedPointer<const CompositeKey>& key,
                       bool transformKey)
 {
     Q_ASSERT(!m_data.isReadOnly);
+    m_keyError.clear();
 
     if (!key) {
         m_data.key.reset();
@@ -724,7 +726,7 @@ bool Database::setKey(const QSharedPointer<const CompositeKey>& key,
 
     if (!transformKey) {
         transformedMasterKey = QByteArray(oldTransformedMasterKey.rawKey());
-    } else if (!key->transform(*m_data.kdf, transformedMasterKey)) {
+    } else if (!key->transform(*m_data.kdf, transformedMasterKey, &m_keyError)) {
         return false;
     }
 
@@ -743,25 +745,9 @@ bool Database::setKey(const QSharedPointer<const CompositeKey>& key,
     return true;
 }
 
-bool Database::verifyKey(const QSharedPointer<CompositeKey>& key) const
+QString Database::keyError()
 {
-    Q_ASSERT(!m_data.key->isEmpty());
-
-    if (!m_data.challengeResponseKey->rawKey().isEmpty()) {
-        QByteArray result;
-
-        if (!key->challenge(m_data.masterSeed->rawKey(), result)) {
-            // challenge failed, (YubiKey?) removed?
-            return false;
-        }
-
-        if (m_data.challengeResponseKey->rawKey() != result) {
-            // wrong response from challenged device(s)
-            return false;
-        }
-    }
-
-    return (m_data.key->rawKey() == key->rawKey());
+    return m_keyError;
 }
 
 QVariantMap& Database::publicCustomData()

diff --git a/src/core/Database.h b/src/core/Database.h
@@ -119,9 +119,9 @@ class Database : public QObject
                 bool updateChangedTime = true,
                 bool updateTransformSalt = false,
                 bool transformKey = true);
+    QString keyError();
     QByteArray challengeResponseKey() const;
     bool challengeMasterSeed(const QByteArray& masterSeed);
-    bool verifyKey(const QSharedPointer<CompositeKey>& key) const;
     const QUuid& cipher() const;
     void setCipher(const QUuid& cipher);
     Database::CompressionAlgorithm compressionAlgorithm() const;
@@ -210,6 +210,7 @@ public slots:
     QPointer<FileWatcher> m_fileWatcher;
     bool m_modified = false;
     bool m_emitModified;
+    QString m_keyError;
 
     QList<QString> m_commonUsernames;
 

diff --git a/src/format/Kdbx3Reader.cpp b/src/format/Kdbx3Reader.cpp
@@ -55,7 +55,7 @@ bool Kdbx3Reader::readDatabaseImpl(QIODevice* device,
     }
 
     if (!db->challengeMasterSeed(m_masterSeed)) {
-        raiseError(tr("Unable to issue challenge-response."));
+        raiseError(tr("Unable to issue challenge-response: %1").arg(db->keyError()));
         return false;
     }
 

diff --git a/src/format/Kdbx3Writer.cpp b/src/format/Kdbx3Writer.cpp
@@ -42,7 +42,7 @@ bool Kdbx3Writer::writeDatabase(QIODevice* device, Database* db)
     QByteArray endOfHeader = "\r\n\r\n";
 
     if (!db->challengeMasterSeed(masterSeed)) {
-        raiseError(tr("Unable to issue challenge-response."));
+        raiseError(tr("Unable to issue challenge-response: %1").arg(db->keyError()));
         return false;
     }
 

diff --git a/src/format/Kdbx4Reader.cpp b/src/format/Kdbx4Reader.cpp
@@ -50,7 +50,7 @@ bool Kdbx4Reader::readDatabaseImpl(QIODevice* device,
 
     bool ok = AsyncTask::runAndWaitForFuture([&] { return db->setKey(key, false, false); });
     if (!ok) {
-        raiseError(tr("Unable to calculate master key"));
+        raiseError(tr("Unable to calculate master key: %1").arg(db->keyError()));
         return false;
     }
 

diff --git a/src/format/Kdbx4Writer.cpp b/src/format/Kdbx4Writer.cpp
@@ -53,7 +53,7 @@ bool Kdbx4Writer::writeDatabase(QIODevice* device, Database* db)
     QByteArray endOfHeader = "\r\n\r\n";
 
     if (!db->setKey(db->key(), false, true)) {
-        raiseError(tr("Unable to calculate master key"));
+        raiseError(tr("Unable to calculate master key: %1").arg(db->keyError()));
         return false;
     }
 

diff --git a/src/gui/ApplicationSettingsWidget.cpp b/src/gui/ApplicationSettingsWidget.cpp
@@ -390,6 +390,7 @@ void ApplicationSettingsWidget::saveSettings()
 
     if (!config()->get(Config::RememberLastKeyFiles).toBool()) {
         config()->remove(Config::LastKeyFiles);
+        config()->remove(Config::LastChallengeResponse);
         config()->remove(Config::LastDir);
     }