Fix challenge-response key data after Botan

* Fix #6420
* Refactor Challenge-Response key files to be more streamlined. Added a test to confirm raw key data is accurate.

src/CMakeLists.txt

@@ -181,7 +181,7 @@ set(keepassx_SOURCES
         keys/CompositeKey.cpp
         keys/FileKey.cpp
         keys/PasswordKey.cpp
-        keys/YkChallengeResponseKey.cpp
+        keys/ChallengeResponseKey.cpp
         streams/HashedBlockStream.cpp
         streams/HmacBlockStream.cpp
         streams/LayeredStream.cpp

src/cli/Utils.cpp

@@ -23,7 +23,7 @@
 #include "keys/FileKey.h"
 #include "keys/PasswordKey.h"
 #ifdef WITH_XC_YUBIKEY
-#include "keys/YkChallengeResponseKey.h"
+#include "keys/ChallengeResponseKey.h"
 #endif
 
 #ifdef Q_OS_WIN
@@ -174,7 +174,7 @@ namespace Utils
                 err << QObject::tr("Please touch the button on your YubiKey to continue…") << "\n\n" << flush;
             });
 
-            auto key = QSharedPointer<YkChallengeResponseKey>(new YkChallengeResponseKey({serial, slot}));
+            auto key = QSharedPointer<ChallengeResponseKey>(new ChallengeResponseKey({serial, slot}));
             compositeKey->addChallengeResponseKey(key);
 
             QObject::disconnect(conn);

src/gui/DatabaseOpenWidget.cpp

@@ -27,9 +27,9 @@
 #include "gui/Icons.h"
 #include "gui/MainWindow.h"
 #include "gui/MessageBox.h"
+#include "keys/ChallengeResponseKey.h"
 #include "keys/FileKey.h"
 #include "keys/PasswordKey.h"
-#include "keys/YkChallengeResponseKey.h"
 #include "touchid/TouchID.h"
 
 #include "config-keepassx.h"
@@ -337,7 +337,7 @@ QSharedPointer<CompositeKey> DatabaseOpenWidget::buildDatabaseKey()
     int selectionIndex = m_ui->challengeResponseCombo->currentIndex();
     if (selectionIndex > 0) {
         auto slot = m_ui->challengeResponseCombo->itemData(selectionIndex).value<YubiKeySlot>();
-        auto crKey = QSharedPointer<YkChallengeResponseKey>(new YkChallengeResponseKey(slot));
+        auto crKey = QSharedPointer<ChallengeResponseKey>(new ChallengeResponseKey(slot));
         databaseKey->addChallengeResponseKey(crKey);
 
         // Qt doesn't read custom types in settings so stuff into a QString

src/gui/databasekey/YubiKeyEditWidget.cpp

@@ -21,8 +21,8 @@
 
 #include "config-keepassx.h"
 #include "core/AsyncTask.h"
+#include "keys/ChallengeResponseKey.h"
 #include "keys/CompositeKey.h"
-#include "keys/YkChallengeResponseKey.h"
 
 YubiKeyEditWidget::YubiKeyEditWidget(QWidget* parent)
     : KeyComponentWidget(parent)
@@ -45,7 +45,7 @@ bool YubiKeyEditWidget::addToCompositeKey(QSharedPointer<CompositeKey> key)
 
     int selectionIndex = m_compUi->comboChallengeResponse->currentIndex();
     auto slot = m_compUi->comboChallengeResponse->itemData(selectionIndex).value<YubiKeySlot>();
-    key->addChallengeResponseKey(QSharedPointer<YkChallengeResponseKey>::create(slot));
+    key->addChallengeResponseKey(QSharedPointer<ChallengeResponseKey>::create(slot));
     return true;
 }
 

src/gui/databasekey/YubiKeyEditWidget.h

@@ -26,7 +26,7 @@ namespace Ui
     class YubiKeyEditWidget;
 }
 
-class YkChallengeResponseKey;
+class ChallengeResponseKey;
 
 class YubiKeyEditWidget : public KeyComponentWidget
 {

src/gui/dbsettings/DatabaseSettingsWidgetDatabaseKey.cpp

@@ -22,9 +22,9 @@
 #include "gui/databasekey/KeyFileEditWidget.h"
 #include "gui/databasekey/PasswordEditWidget.h"
 #include "gui/databasekey/YubiKeyEditWidget.h"
+#include "keys/ChallengeResponseKey.h"
 #include "keys/FileKey.h"
 #include "keys/PasswordKey.h"
-#include "keys/YkChallengeResponseKey.h"
 
 #include <QPushButton>
 #include <QSpacerItem>
@@ -91,7 +91,7 @@ void DatabaseSettingsWidgetDatabaseKey::load(QSharedPointer<Database> db)
 
 #ifdef WITH_XC_YUBIKEY
     for (const auto& key : m_db->key()->challengeResponseKeys()) {
-        if (key->uuid() == YkChallengeResponseKey::UUID) {
+        if (key->uuid() == ChallengeResponseKey::UUID) {
             m_yubiKeyEditWidget->setComponentAdded(true);
             hasAdditionalKeys = true;
         }
@@ -150,7 +150,7 @@ bool DatabaseSettingsWidgetDatabaseKey::save()
     }
 
     for (const auto& key : m_db->key()->challengeResponseKeys()) {
-        if (key->uuid() == YkChallengeResponseKey::UUID) {
+        if (key->uuid() == ChallengeResponseKey::UUID) {
             oldChallengeResponse = key;
         }
     }

src/keys/ChallengeResponseKey.cpp

@@ -16,30 +16,29 @@
  *  along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include "keys/YkChallengeResponseKey.h"
+#include "ChallengeResponseKey.h"
-#include "keys/drivers/YubiKey.h"
 
 #include "core/AsyncTask.h"
-#include "core/Tools.h"
-#include "crypto/CryptoHash.h"
-#include "crypto/Random.h"
 
-#include <QApplication>
+QUuid ChallengeResponseKey::UUID("e092495c-e77d-498b-84a1-05ae0d955508");
-#include <QEventLoop>
-#include <QFile>
-#include <QFutureWatcher>
-#include <QXmlStreamReader>
-#include <QtConcurrent>
 
-QUuid YkChallengeResponseKey::UUID("e092495c-e77d-498b-84a1-05ae0d955508");
+ChallengeResponseKey::ChallengeResponseKey(YubiKeySlot keySlot)
-
+    : Key(UUID)
-YkChallengeResponseKey::YkChallengeResponseKey(YubiKeySlot keySlot)
-    : ChallengeResponseKey(UUID)
     , m_keySlot(keySlot)
 {
 }
 
-bool YkChallengeResponseKey::challenge(const QByteArray& challenge)
+QByteArray ChallengeResponseKey::rawKey() const
+{
+    return QByteArray(m_key.data(), m_key.size());
+}
+
+QString ChallengeResponseKey::error() const
+{
+    return m_error;
+}
+
+bool ChallengeResponseKey::challenge(const QByteArray& challenge)
 {
     m_error.clear();
     auto result =

src/keys/ChallengeResponseKey.h

@@ -16,44 +16,36 @@
  *  along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#ifndef KEEPASSX_CHALLENGE_RESPONSE_KEY_H
+#ifndef KPXC_CHALLENGE_RESPONSE_KEY_H
-#define KEEPASSX_CHALLENGE_RESPONSE_KEY_H
+#define KPXC_CHALLENGE_RESPONSE_KEY_H
+
+#include "Key.h"
+#include "drivers/YubiKey.h"
 
 #include <QByteArray>
 #include <QUuid>
+
 #include <botan/secmem.h>
 
-class ChallengeResponseKey
+class ChallengeResponseKey : public Key
 {
 public:
-    explicit ChallengeResponseKey(const QUuid& uuid)
+    explicit ChallengeResponseKey(YubiKeySlot keySlot = {});
-        : m_uuid(uuid)
+    ~ChallengeResponseKey() override = default;
-    {
-    }
-    virtual ~ChallengeResponseKey() = default;
 
-    virtual bool challenge(const QByteArray& challenge) = 0;
+    QByteArray rawKey() const override;
 
-    Botan::secure_vector<char>& rawKey()
+    virtual bool challenge(const QByteArray& challenge);
-    {
+    QString error() const;
-        return m_key;
-    }
-    QUuid uuid() const
-    {
-        return m_uuid;
-    }
-    QString error() const
-    {
-        return m_error;
-    }
 
-protected:
+    static QUuid UUID;
-    QString m_error;
-    Botan::secure_vector<char> m_key;
 
 private:
     Q_DISABLE_COPY(ChallengeResponseKey);
-    QUuid m_uuid;
+
+    QString m_error;
+    Botan::secure_vector<char> m_key;
+    YubiKeySlot m_keySlot;
 };
 
-#endif // KEEPASSX_CHALLENGE_RESPONSE_KEY_H
+#endif // KPXC_CHALLENGE_RESPONSE_KEY_H

src/keys/CompositeKey.cpp

@@ -24,6 +24,7 @@
 #include "core/Global.h"
 #include "crypto/CryptoHash.h"
 #include "crypto/kdf/AesKdf.h"
+#include "keys/ChallengeResponseKey.h"
 
 QUuid CompositeKey::UUID("76a7ae25-a542-4add-9849-7c06be945b94");
 
@@ -143,7 +144,7 @@ bool CompositeKey::challenge(const QByteArray& seed, QByteArray& result, QString
             qWarning() << "Failed to issue challenge: " << key->error();
             return false;
         }
-        cryptoHash.addData(key->rawKey().data());
+        cryptoHash.addData(key->rawKey());
     }
 
     result = cryptoHash.result();

src/keys/CompositeKey.h

@@ -23,10 +23,11 @@
 #include <QSharedPointer>
 #include <QString>
 
-#include "crypto/kdf/Kdf.h"
-#include "keys/ChallengeResponseKey.h"
 #include "keys/Key.h"
 
+class Kdf;
+class ChallengeResponseKey;
+
 class CompositeKey : public Key
 {
 public:

src/keys/YkChallengeResponseKey.h

@@ -1,39 +0,0 @@
-/*
- *  Copyright (C) 2019 KeePassXC Team <team@keepassxc.org>
- *
- *  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
- *  the Free Software Foundation, either version 2 or (at your option)
- *  version 3 of the License.
- *
- *  This program is distributed in the hope that it will be useful,
- *  but WITHOUT ANY WARRANTY; without even the implied warranty of
- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *  GNU General Public License for more details.
- *
- *  You should have received a copy of the GNU General Public License
- *  along with this program.  If not, see <http://www.gnu.org/licenses/>.
- */
-
-#ifndef KEEPASSX_YK_CHALLENGERESPONSEKEY_H
-#define KEEPASSX_YK_CHALLENGERESPONSEKEY_H
-
-#include "core/Global.h"
-#include "keys/ChallengeResponseKey.h"
-#include "keys/drivers/YubiKey.h"
-
-class YkChallengeResponseKey : public ChallengeResponseKey
-{
-public:
-    static QUuid UUID;
-
-    explicit YkChallengeResponseKey(YubiKeySlot keySlot = {});
-    ~YkChallengeResponseKey() override = default;
-
-    bool challenge(const QByteArray& challenge) override;
-
-private:
-    YubiKeySlot m_keySlot;
-};
-
-#endif // KEEPASSX_YK_CHALLENGERESPONSEKEY_H

tests/TestYkChallengeResponseKey.cpp

@@ -22,8 +22,9 @@
 
 #include "core/Tools.h"
 #include "crypto/Crypto.h"
-#include "keys/YkChallengeResponseKey.h"
+#include "keys/ChallengeResponseKey.h"
 
+#include <QCryptographicHash>
 #include <QScopedPointer>
 #include <QSignalSpy>
 
@@ -50,7 +51,7 @@ void TestYubiKeyChallengeResponse::testDetectDevices()
     // Look at the information retrieved from the key(s)
     for (auto key : YubiKey::instance()->foundKeys()) {
         auto displayName = YubiKey::instance()->getDisplayName(key);
-        QVERIFY(displayName.contains("Challenge Response - Slot") || displayName.contains("Configured Slot -"));
+        QVERIFY(displayName.contains("Challenge-Response - Slot") || displayName.contains("Configured Slot -"));
         QVERIFY(displayName.contains(QString::number(key.first)));
         QVERIFY(displayName.contains(QString::number(key.second)));
     }
@@ -84,9 +85,11 @@ void TestYubiKeyChallengeResponse::testKeyChallenge()
         QSKIP("No YubiKey contains a slot in passive mode.");
     }
 
-    QScopedPointer<YkChallengeResponseKey> key(new YkChallengeResponseKey(pKey));
+    QScopedPointer<ChallengeResponseKey> key(new ChallengeResponseKey(pKey));
 
     QByteArray ba("UnitTest");
     QVERIFY(key->challenge(ba));
-    QCOMPARE(key->rawKey().size(), 20UL);
+    QCOMPARE(key->rawKey().size(), 20);
+    auto hash = QString(QCryptographicHash::hash(key->rawKey(), QCryptographicHash::Sha256).toHex());
+    QCOMPARE(hash, QString("2f7802c7112c301303526e7737b54d546c905076dca6e9538edf761a2264cd70"));
 }

tests/mock/MockChallengeResponseKey.cpp

@@ -18,22 +18,18 @@
 #include "MockChallengeResponseKey.h"
 
 MockChallengeResponseKey::MockChallengeResponseKey(const QByteArray& secret)
-    : ChallengeResponseKey(QUuid("aac5b480-cdc0-411e-9cb8-962062dcc1fd"))
+    : ChallengeResponseKey()
     , m_secret(secret)
 {
 }
 
-MockChallengeResponseKey::~MockChallengeResponseKey()
+QByteArray MockChallengeResponseKey::rawKey() const
 {
+    return m_challenge + m_secret;
 }
 
 bool MockChallengeResponseKey::challenge(const QByteArray& challenge)
 {
     m_challenge = challenge;
-
-    auto response = m_challenge + m_secret;
-    m_key.resize(response.size());
-    std::copy(response.begin(), response.end(), m_key.data());
-
     return true;
 }

tests/mock/MockChallengeResponseKey.h

@@ -28,13 +28,17 @@ class MockChallengeResponseKey : public ChallengeResponseKey
 {
 public:
     explicit MockChallengeResponseKey(const QByteArray& secret);
-    Q_DISABLE_COPY(MockChallengeResponseKey);
+    ~MockChallengeResponseKey() override = default;
-    ~MockChallengeResponseKey() override;
+
+    QByteArray rawKey() const override;
+
     bool challenge(const QByteArray& challenge) override;
 
 private:
     QByteArray m_challenge;
     QByteArray m_secret;
+
+    Q_DISABLE_COPY(MockChallengeResponseKey);
 };
 
 #endif // KEEPASSXC_MOCKCHALLENGERESPONSEKEY_H