Show a clear error if no slots on hardware key(s) are configured (#11609

) Fixes #11543 Also fix delayed polling on window activation --------- Co-authored-by: w15dev <w15developer@proton.me> Co-authored-by: Jonathan White <support@dmapps.us>
keepassxreboot · Jan 3, 2025 · 9e29b5c · 9e29b5c
1 parent 2afec91
commit 9e29b5c
Show file tree

Hide file tree

Showing 13 changed files with 89 additions and 58 deletions.
diff --git a/share/translations/keepassxc_en.ts b/share/translations/keepassxc_en.ts
@@ -1664,6 +1664,10 @@ Are you sure you want to continue with this file?.</source>
         <source>&lt;a href=&quot;#&quot; style=&quot;text-decoration: underline&quot;&gt;I have a key file&lt;/a&gt;</source>
         <translation type="unfinished"></translation>
     </message>
+    <message>
+        <source>Hardware keys found, but no slots are configured.</source>
+        <translation type="unfinished"></translation>
+    </message>
 </context>
 <context>
     <name>DatabaseSettingWidgetMetaData</name>
@@ -10230,6 +10234,10 @@ Example: JBSWY3DPEHPK3PXP</source>
         <source>&lt;p&gt;If you own a &lt;a href=&quot;https://www.yubico.com/&quot;&gt;YubiKey&lt;/a&gt; or &lt;a href=&quot;https://onlykey.io&quot;&gt;OnlyKey&lt;/a&gt;, you can use it for additional security.&lt;/p&gt;&lt;p&gt;The key requires one of its slots to be programmed with &lt;a href=&quot;https://keepassxc.org/docs/#faq-yubikey-howto&quot;&gt;Challenge-Response&lt;/a&gt;.&lt;/p&gt;</source>
         <translation type="unfinished"></translation>
     </message>
+    <message>
+        <source>Hardware keys found, but no slots are configured</source>
+        <translation type="unfinished"></translation>
+    </message>
 </context>
 <context>
     <name>YubiKeyInterfacePCSC</name>

diff --git a/src/gui/DatabaseOpenWidget.cpp b/src/gui/DatabaseOpenWidget.cpp
@@ -102,7 +102,7 @@ DatabaseOpenWidget::DatabaseOpenWidget(QWidget* parent)
     m_ui->hardwareKeyProgress->setSizePolicy(sp);
 
 #ifdef WITH_XC_YUBIKEY
-    connect(m_deviceListener, SIGNAL(devicePlugged(bool, void*, void*)), this, SLOT(pollHardwareKey()));
+    connect(m_deviceListener, &DeviceListener::devicePlugged, this, [this] { pollHardwareKey(false, 500); });
     connect(YubiKey::instance(), SIGNAL(detectComplete(bool)), SLOT(hardwareKeyResponse(bool)), Qt::QueuedConnection);
 
     connect(YubiKey::instance(), &YubiKey::userInteractionRequest, this, [this] {
@@ -140,7 +140,12 @@ void DatabaseOpenWidget::toggleHardwareKeyComponent(bool state)
     m_ui->hardwareKeyProgress->setVisible(false);
     m_ui->hardwareKeyComponent->setVisible(state);
     m_ui->hardwareKeyCombo->setVisible(state && m_ui->hardwareKeyCombo->count() != 1);
+
     m_ui->noHardwareKeysFoundLabel->setVisible(!state && m_manualHardwareKeyRefresh);
+    m_ui->noHardwareKeysFoundLabel->setText(YubiKey::instance()->connectedKeys() > 0
+                                                ? tr("Hardware keys found, but no slots are configured.")
+                                                : tr("No hardware keys found."));
+
     if (!state) {
         m_ui->useHardwareKeyCheckBox->setChecked(false);
     }
@@ -520,7 +525,7 @@ bool DatabaseOpenWidget::browseKeyFile()
     return true;
 }
 
-void DatabaseOpenWidget::pollHardwareKey(bool manualTrigger)
+void DatabaseOpenWidget::pollHardwareKey(bool manualTrigger, int delay)
 {
     if (m_pollingHardwareKey) {
         return;
@@ -534,9 +539,6 @@ void DatabaseOpenWidget::pollHardwareKey(bool manualTrigger)
     m_pollingHardwareKey = true;
     m_manualHardwareKeyRefresh = manualTrigger;
 
-    // Add a delay, if this is an automatic trigger, to allow the USB device to settle as
-    // the device may not report a valid serial number immediately after plugging in
-    int delay = manualTrigger ? 0 : 500;
     QTimer::singleShot(delay, this, [] { YubiKey::instance()->findValidKeysAsync(); });
 }
 

diff --git a/src/gui/DatabaseOpenWidget.h b/src/gui/DatabaseOpenWidget.h
@@ -79,7 +79,7 @@ protected slots:
 private slots:
     bool browseKeyFile();
     void toggleHardwareKeyComponent(bool state);
-    void pollHardwareKey(bool manualTrigger = false);
+    void pollHardwareKey(bool manualTrigger = false, int delay = 0);
     void hardwareKeyResponse(bool found);
 
 private:

diff --git a/src/gui/databasekey/YubiKeyEditWidget.cpp b/src/gui/databasekey/YubiKeyEditWidget.cpp
@@ -173,7 +173,9 @@ void YubiKeyEditWidget::hardwareKeyResponse(bool found)
 
     if (!found) {
         m_compUi->yubikeyProgress->setVisible(false);
-        m_compUi->comboChallengeResponse->addItem(tr("No hardware keys detected"));
+        m_compUi->comboChallengeResponse->addItem(YubiKey::instance()->connectedKeys() > 0
+                                                      ? tr("Hardware keys found, but no slots are configured")
+                                                      : tr("No hardware keys detected"));
         m_isDetected = false;
         return;
     }

diff --git a/src/keys/drivers/YubiKey.cpp b/src/keys/drivers/YubiKey.cpp
@@ -75,8 +75,9 @@ bool YubiKey::findValidKeys()
 {
     QMutexLocker lock(&s_interfaceMutex);
 
-    m_usbKeys = YubiKeyInterfaceUSB::instance()->findValidKeys();
-    m_pcscKeys = YubiKeyInterfacePCSC::instance()->findValidKeys();
+    m_connectedKeys = 0;
+    m_usbKeys = YubiKeyInterfaceUSB::instance()->findValidKeys(m_connectedKeys);
+    m_pcscKeys = YubiKeyInterfacePCSC::instance()->findValidKeys(m_connectedKeys);
 
     return !m_usbKeys.isEmpty() || !m_pcscKeys.isEmpty();
 }
@@ -89,19 +90,17 @@ void YubiKey::findValidKeysAsync()
 YubiKey::KeyMap YubiKey::foundKeys()
 {
     QMutexLocker lock(&s_interfaceMutex);
-    KeyMap foundKeys;
-
-    for (auto i = m_usbKeys.cbegin(); i != m_usbKeys.cend(); ++i) {
-        foundKeys.insert(i.key(), i.value());
-    }
-
-    for (auto i = m_pcscKeys.cbegin(); i != m_pcscKeys.cend(); ++i) {
-        foundKeys.insert(i.key(), i.value());
-    }
+    KeyMap foundKeys = m_usbKeys;
+    foundKeys.unite(m_pcscKeys);
 
     return foundKeys;
 }
 
+int YubiKey::connectedKeys()
+{
+    return m_connectedKeys;
+}
+
 QString YubiKey::errorMessage()
 {
     QMutexLocker lock(&s_interfaceMutex);

diff --git a/src/keys/drivers/YubiKey.h b/src/keys/drivers/YubiKey.h
@@ -38,7 +38,8 @@ class YubiKey : public QObject
     Q_OBJECT
 
 public:
-    typedef QMap<YubiKeySlot, QString> KeyMap;
+    using KeyMap = QMap<YubiKeySlot, QString>;
+
     enum class ChallengeResult : int
     {
         YCR_ERROR = 0,
@@ -53,6 +54,7 @@ class YubiKey : public QObject
     void findValidKeysAsync();
 
     KeyMap foundKeys();
+    int connectedKeys();
 
     ChallengeResult challenge(YubiKeySlot slot, const QByteArray& challenge, Botan::secure_vector<char>& response);
     bool testChallenge(YubiKeySlot slot, bool* wouldBlock = nullptr);
@@ -93,6 +95,8 @@ class YubiKey : public QObject
     KeyMap m_usbKeys;
     KeyMap m_pcscKeys;
 
+    int m_connectedKeys = 0;
+
     Q_DISABLE_COPY(YubiKey)
 };
 

diff --git a/src/keys/drivers/YubiKeyInterface.h b/src/keys/drivers/YubiKeyInterface.h
@@ -32,7 +32,7 @@ class YubiKeyInterface : public QObject
 public:
     bool isInitialized() const;
 
-    virtual YubiKey::KeyMap findValidKeys() = 0;
+    virtual YubiKey::KeyMap findValidKeys(int& connectedKeys) = 0;
     virtual YubiKey::ChallengeResult
     challenge(YubiKeySlot slot, const QByteArray& challenge, Botan::secure_vector<char>& response) = 0;
     virtual bool testChallenge(YubiKeySlot slot, bool* wouldBlock) = 0;

diff --git a/src/keys/drivers/YubiKeyInterfacePCSC.cpp b/src/keys/drivers/YubiKeyInterfacePCSC.cpp
@@ -20,6 +20,8 @@
 #include "core/Tools.h"
 #include "crypto/Random.h"
 
+#include <QScopeGuard>
+
 // MSYS2 does not define these macros
 // So set them to the value used by pcsc-lite
 #ifndef MAX_ATR_SIZE
@@ -511,7 +513,6 @@ namespace
             return rv;
         });
     }
-
 } // namespace
 
 YubiKeyInterfacePCSC::YubiKeyInterfacePCSC()
@@ -542,7 +543,7 @@ YubiKeyInterfacePCSC* YubiKeyInterfacePCSC::instance()
     return m_instance;
 }
 
-YubiKey::KeyMap YubiKeyInterfacePCSC::findValidKeys()
+YubiKey::KeyMap YubiKeyInterfacePCSC::findValidKeys(int& connectedKeys)
 {
     m_error.clear();
     if (!isInitialized()) {
@@ -578,6 +579,8 @@ YubiKey::KeyMap YubiKeyInterfacePCSC::findValidKeys()
             continue;
         }
 
+        auto finally = qScopeGuard([hCard]() { SCardDisconnect(hCard, SCARD_LEAVE_CARD); });
+
         // Read the protocol and the ATR record
         char pbReader[MAX_READERNAME] = {0};
         SCUINT dwReaderLen = sizeof(pbReader);
@@ -605,6 +608,8 @@ YubiKey::KeyMap YubiKeyInterfacePCSC::findValidKeys()
             unsigned int serial = 0;
             getSerial(satr, serial);
 
+            ++connectedKeys;
+
             /* This variable indicates that the key is locked / timed out.
                 When using the key via NFC, the user has to re-present the key to clear the timeout.
                 Also, the key can be programmatically reset (see below).

diff --git a/src/keys/drivers/YubiKeyInterfacePCSC.h b/src/keys/drivers/YubiKeyInterfacePCSC.h
@@ -52,7 +52,7 @@ class YubiKeyInterfacePCSC : public YubiKeyInterface
 public:
     static YubiKeyInterfacePCSC* instance();
 
-    YubiKey::KeyMap findValidKeys() override;
+    YubiKey::KeyMap findValidKeys(int& connectedKeys) override;
 
     YubiKey::ChallengeResult
     challenge(YubiKeySlot slot, const QByteArray& challenge, Botan::secure_vector<char>& response) override;

diff --git a/src/keys/drivers/YubiKeyInterfaceUSB.cpp b/src/keys/drivers/YubiKeyInterfaceUSB.cpp
@@ -18,7 +18,6 @@
 
 #include "YubiKeyInterfaceUSB.h"
 
-#include "core/Tools.h"
 #include "crypto/Random.h"
 #include "thirdparty/ykcore/ykcore.h"
 #include "thirdparty/ykcore/ykstatus.h"
@@ -27,7 +26,15 @@ namespace
 {
     constexpr int MAX_KEYS = 4;
 
-    YK_KEY* openKey(int index)
+    void closeKey(YK_KEY* key)
+    {
+        yk_close_key(key);
+    }
+
+    using Yubikey = std::unique_ptr<YK_KEY, decltype(&closeKey)>;
+    using YubikeyStatus = std::unique_ptr<YK_STATUS, decltype(&ykds_free)>;
+
+    Yubikey openKey(int index)
     {
         static const int vids[] = {YUBICO_VID, ONLYKEY_VID};
         static const int pids[] = {YUBIKEY_PID,
@@ -42,12 +49,9 @@ namespace
                                    PLUS_U2F_OTP_PID,
                                    ONLYKEY_PID};
 
-        return yk_open_key_vid_pid(vids, sizeof(vids) / sizeof(vids[0]), pids, sizeof(pids) / sizeof(pids[0]), index);
-    }
-
-    void closeKey(YK_KEY* key)
-    {
-        yk_close_key(key);
+        return Yubikey{
+            yk_open_key_vid_pid(vids, sizeof(vids) / sizeof(vids[0]), pids, sizeof(pids) / sizeof(pids[0]), index),
+            &closeKey};
     }
 
     void printError()
@@ -69,24 +73,25 @@ namespace
         return serial;
     }
 
-    YK_KEY* openKeySerial(unsigned int serial)
+    Yubikey openKeySerial(unsigned int serial)
     {
         for (int i = 0; i < MAX_KEYS; ++i) {
-            auto* yk_key = openKey(i);
-            if (yk_key) {
+            auto key = openKey(i);
+            if (key) {
                 // If the provided serial number is 0, or the key matches the serial, return it
-                if (serial == 0 || getSerial(yk_key) == serial) {
-                    return yk_key;
+                if (serial == 0 || getSerial(key.get()) == serial) {
+                    return key;
                 }
-                closeKey(yk_key);
             } else if (yk_errno == YK_ENOKEY) {
                 // No more connected keys
                 break;
             }
             printError();
         }
-        return nullptr;
+
+        return Yubikey{nullptr, &closeKey};
     }
+
 } // namespace
 
 YubiKeyInterfaceUSB::YubiKeyInterfaceUSB()
@@ -114,7 +119,7 @@ YubiKeyInterfaceUSB* YubiKeyInterfaceUSB::instance()
     return m_instance;
 }
 
-YubiKey::KeyMap YubiKeyInterfaceUSB::findValidKeys()
+YubiKey::KeyMap YubiKeyInterfaceUSB::findValidKeys(int& connectedKeys)
 {
     m_error.clear();
     if (!isInitialized()) {
@@ -127,29 +132,30 @@ YubiKey::KeyMap YubiKeyInterfaceUSB::findValidKeys()
     for (int i = 0; i < MAX_KEYS; ++i) {
         auto yk_key = openKey(i);
         if (yk_key) {
-            auto serial = getSerial(yk_key);
+            auto serial = getSerial(yk_key.get());
             if (serial == 0) {
-                closeKey(yk_key);
                 continue;
             }
 
-            auto st = ykds_alloc();
-            yk_get_status(yk_key, st);
+            ++connectedKeys;
+
+            YubikeyStatus st{ykds_alloc(), &ykds_free};
+            yk_get_status(yk_key.get(), st.get());
             int vid, pid;
-            yk_get_key_vid_pid(yk_key, &vid, &pid);
+            yk_get_key_vid_pid(yk_key.get(), &vid, &pid);
 
             QString name = m_pid_names.value(pid, tr("Unknown"));
             if (vid == ONLYKEY_VID) {
                 name = QStringLiteral("OnlyKey %ver");
             }
             if (name.contains("%ver")) {
-                name = name.replace("%ver", QString::number(ykds_version_major(st)));
+                name = name.replace("%ver", QString::number(ykds_version_major(st.get())));
             }
 
             bool wouldBlock;
             for (int slot = 1; slot <= 2; ++slot) {
                 auto config = (slot == 1 ? CONFIG1_VALID : CONFIG2_VALID);
-                if (!(ykds_touch_level(st) & config)) {
+                if (!(ykds_touch_level(st.get()) & config)) {
                     // Slot is not configured
                     continue;
                 }
@@ -159,7 +165,7 @@ YubiKey::KeyMap YubiKeyInterfaceUSB::findValidKeys()
                     auto display = tr("%1 [%2] - Slot %3", "YubiKey NEO display fields")
                                        .arg(name, QString::number(serial), QString::number(slot));
                     keyMap.insert({serial, slot}, display);
-                } else if (performTestChallenge(yk_key, slot, &wouldBlock)) {
+                } else if (performTestChallenge(yk_key.get(), slot, &wouldBlock)) {
                     auto display =
                         tr("%1 [%2] - Slot %3, %4", "YubiKey display fields")
                             .arg(name,
@@ -170,9 +176,6 @@ YubiKey::KeyMap YubiKeyInterfaceUSB::findValidKeys()
                     keyMap.insert({serial, slot}, display);
                 }
             }
-
-            ykds_free(st);
-            closeKey(yk_key);
         } else if (yk_errno == YK_ENOKEY) {
             // No more keys are connected
             break;
@@ -197,11 +200,11 @@ YubiKey::KeyMap YubiKeyInterfaceUSB::findValidKeys()
 bool YubiKeyInterfaceUSB::testChallenge(YubiKeySlot slot, bool* wouldBlock)
 {
     bool ret = false;
-    auto* yk_key = openKeySerial(slot.first);
+    auto yk_key = openKeySerial(slot.first);
     if (yk_key) {
-        ret = performTestChallenge(yk_key, slot.second, wouldBlock);
+        ret = performTestChallenge(yk_key.get(), slot.second, wouldBlock);
     }
-    closeKey(yk_key);
+
     return ret;
 }
 
@@ -237,7 +240,7 @@ YubiKeyInterfaceUSB::challenge(YubiKeySlot slot, const QByteArray& challenge, Bo
         return YubiKey::ChallengeResult::YCR_ERROR;
     }
 
-    auto* yk_key = openKeySerial(slot.first);
+    auto yk_key = openKeySerial(slot.first);
     if (!yk_key) {
         // Key with specified serial number is not connected
         m_error =
@@ -246,9 +249,8 @@ YubiKeyInterfaceUSB::challenge(YubiKeySlot slot, const QByteArray& challenge, Bo
     }
 
     emit challengeStarted();
-    auto ret = performChallenge(yk_key, slot.second, true, challenge, response);
+    auto ret = performChallenge(yk_key.get(), slot.second, true, challenge, response);
 
-    closeKey(yk_key);
     emit challengeCompleted();
 
     return ret;

diff --git a/src/keys/drivers/YubiKeyInterfaceUSB.h b/src/keys/drivers/YubiKeyInterfaceUSB.h
@@ -35,7 +35,7 @@ class YubiKeyInterfaceUSB : public YubiKeyInterface
     static constexpr int YUBICO_USB_VID = YUBICO_VID;
     static constexpr int ONLYKEY_USB_VID = ONLYKEY_VID;
 
-    YubiKey::KeyMap findValidKeys() override;
+    YubiKey::KeyMap findValidKeys(int& connectedKeys) override;
 
     YubiKey::ChallengeResult
     challenge(YubiKeySlot slot, const QByteArray& challenge, Botan::secure_vector<char>& response) override;