Ensure challenge-response key buffer is properly cleared.

The challenge-response key buffer is explicitly cleared
before the key transformation if no such key is configured
to ensure one is never injected into the hash even if the
database had a challenge-response key previously.

This patch also adds extensive tests for verifying that a
key change will not add any expired key material to the hash.

Fixes #4146

6 files changed, 232 insertions, 33 deletions

diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
index 1c0e5f7ed..fc27f48d3 100644
--- a/tests/CMakeLists.txt
+++ b/tests/CMakeLists.txt
@@ -104,7 +104,7 @@ add_unit_test(NAME testgroup SOURCES TestGroup.cpp
 add_unit_test(NAME testkdbx2 SOURCES TestKdbx2.cpp
         LIBS ${TEST_LIBRARIES})
 
-add_unit_test(NAME testkdbx3 SOURCES TestKeePass2Format.cpp FailDevice.cpp TestKdbx3.cpp
+add_unit_test(NAME testkdbx3 SOURCES TestKeePass2Format.cpp FailDevice.cpp mock/MockChallengeResponseKey.cpp TestKdbx3.cpp
         LIBS testsupport ${TEST_LIBRARIES})
 
 add_unit_test(NAME testkdbx4 SOURCES TestKeePass2Format.cpp FailDevice.cpp mock/MockChallengeResponseKey.cpp TestKdbx4.cpp
diff --git a/tests/TestKdbx3.cpp b/tests/TestKdbx3.cpp
index bf9a1ec8b..92fd2c752 100644
--- a/tests/TestKdbx3.cpp
+++ b/tests/TestKdbx3.cpp
@@ -31,6 +31,8 @@ QTEST_GUILESS_MAIN(TestKdbx3)
 
 void TestKdbx3::initTestCaseImpl()
 {
+    m_xmlDb->changeKdf(fastKdf(KeePass2::uuidToKdf(KeePass2::KDF_AES_KDBX3)));
+    m_kdbxSourceDb->changeKdf(fastKdf(KeePass2::uuidToKdf(KeePass2::KDF_AES_KDBX3)));
 }
 
 QSharedPointer<Database> TestKdbx3::readXml(const QString& path, bool strictMode, bool& hasError, QString& errorString)
diff --git a/tests/TestKdbx4.cpp b/tests/TestKdbx4.cpp
index 88352d825..51784f062 100644
--- a/tests/TestKdbx4.cpp
+++ b/tests/TestKdbx4.cpp
@@ -29,15 +29,25 @@
 #include "keys/PasswordKey.h"
 #include "mock/MockChallengeResponseKey.h"
 
-QTEST_GUILESS_MAIN(TestKdbx4)
+int main(int argc, char* argv[])
+{
+    QCoreApplication app(argc, argv);
+    QCoreApplication::setAttribute(Qt::AA_Use96Dpi, true);
+    QTEST_SET_MAIN_SOURCE_PATH
+
+    TestKdbx4Argon2 argon2Test;
+    TestKdbx4AesKdf aesKdfTest;
+    return QTest::qExec(&argon2Test, argc, argv) | QTest::qExec(&aesKdfTest, argc, argv);
+}
 
-void TestKdbx4::initTestCaseImpl()
+void TestKdbx4Argon2::initTestCaseImpl()
 {
     m_xmlDb->changeKdf(fastKdf(KeePass2::uuidToKdf(KeePass2::KDF_ARGON2)));
     m_kdbxSourceDb->changeKdf(fastKdf(KeePass2::uuidToKdf(KeePass2::KDF_ARGON2)));
 }
 
-QSharedPointer<Database> TestKdbx4::readXml(const QString& path, bool strictMode, bool& hasError, QString& errorString)
+QSharedPointer<Database>
+TestKdbx4Argon2::readXml(const QString& path, bool strictMode, bool& hasError, QString& errorString)
 {
     KdbxXmlReader reader(KeePass2::FILE_VERSION_4);
     reader.setStrictMode(strictMode);
@@ -47,7 +57,7 @@ QSharedPointer<Database> TestKdbx4::readXml(const QString& path, bool strictMode
     return db;
 }
 
-QSharedPointer<Database> TestKdbx4::readXml(QBuffer* buf, bool strictMode, bool& hasError, QString& errorString)
+QSharedPointer<Database> TestKdbx4Argon2::readXml(QBuffer* buf, bool strictMode, bool& hasError, QString& errorString)
 {
     KdbxXmlReader reader(KeePass2::FILE_VERSION_4);
     reader.setStrictMode(strictMode);
@@ -57,7 +67,7 @@ QSharedPointer<Database> TestKdbx4::readXml(QBuffer* buf, bool strictMode, bool&
     return db;
 }
 
-void TestKdbx4::writeXml(QBuffer* buf, Database* db, bool& hasError, QString& errorString)
+void TestKdbx4Argon2::writeXml(QBuffer* buf, Database* db, bool& hasError, QString& errorString)
 {
     KdbxXmlWriter writer(KeePass2::FILE_VERSION_4);
     writer.writeDatabase(buf, db);
@@ -65,11 +75,11 @@ void TestKdbx4::writeXml(QBuffer* buf, Database* db, bool& hasError, QString& er
     errorString = writer.errorString();
 }
 
-void TestKdbx4::readKdbx(QIODevice* device,
-                         QSharedPointer<const CompositeKey> key,
-                         QSharedPointer<Database> db,
-                         bool& hasError,
-                         QString& errorString)
+void TestKdbx4Argon2::readKdbx(QIODevice* device,
+                               QSharedPointer<const CompositeKey> key,
+                               QSharedPointer<Database> db,
+                               bool& hasError,
+                               QString& errorString)
 {
     KeePass2Reader reader;
     reader.readDatabase(device, key, db.data());
@@ -80,11 +90,11 @@ void TestKdbx4::readKdbx(QIODevice* device,
     QCOMPARE(reader.version(), KeePass2::FILE_VERSION_4);
 }
 
-void TestKdbx4::readKdbx(const QString& path,
-                         QSharedPointer<const CompositeKey> key,
-                         QSharedPointer<Database> db,
-                         bool& hasError,
-                         QString& errorString)
+void TestKdbx4Argon2::readKdbx(const QString& path,
+                               QSharedPointer<const CompositeKey> key,
+                               QSharedPointer<Database> db,
+                               bool& hasError,
+                               QString& errorString)
 {
     KeePass2Reader reader;
     reader.readDatabase(path, key, db.data());
@@ -95,7 +105,7 @@ void TestKdbx4::readKdbx(const QString& path,
     QCOMPARE(reader.version(), KeePass2::FILE_VERSION_4);
 }
 
-void TestKdbx4::writeKdbx(QIODevice* device, Database* db, bool& hasError, QString& errorString)
+void TestKdbx4Argon2::writeKdbx(QIODevice* device, Database* db, bool& hasError, QString& errorString)
 {
     if (db->kdf()->uuid() == KeePass2::KDF_AES_KDBX3) {
         db->changeKdf(fastKdf(KeePass2::uuidToKdf(KeePass2::KDF_ARGON2)));
@@ -110,7 +120,7 @@ void TestKdbx4::writeKdbx(QIODevice* device, Database* db, bool& hasError, QStri
 }
 
 Q_DECLARE_METATYPE(QUuid)
-void TestKdbx4::testFormat400()
+void TestKdbx4Argon2::testFormat400()
 {
     QString filename = QString(KEEPASSX_TEST_DATA_DIR).append("/Format400.kdbx");
     auto key = QSharedPointer<CompositeKey>::create();
@@ -135,7 +145,7 @@ void TestKdbx4::testFormat400()
     QCOMPARE(entry->attachments()->value("Format400"), QByteArray("Format400\n"));
 }
 
-void TestKdbx4::testFormat400Upgrade()
+void TestKdbx4Argon2::testFormat400Upgrade()
 {
     QFETCH(QUuid, kdfUuid);
     QFETCH(QUuid, cipherUuid);
@@ -193,7 +203,7 @@ void TestKdbx4::testFormat400Upgrade()
 }
 
 // clang-format off
-void TestKdbx4::testFormat400Upgrade_data()
+void TestKdbx4Argon2::testFormat400Upgrade_data()
 {
     QTest::addColumn<QUuid>("kdfUuid");
     QTest::addColumn<QUuid>("cipherUuid");
@@ -226,7 +236,7 @@ void TestKdbx4::testFormat400Upgrade_data()
 }
 // clang-format on
 
-void TestKdbx4::testUpgradeMasterKeyIntegrity()
+void TestKdbx4Argon2::testUpgradeMasterKeyIntegrity()
 {
     QFETCH(QString, upgradeAction);
     QFETCH(quint32, expectedVersion);
@@ -249,6 +259,7 @@ void TestKdbx4::testUpgradeMasterKeyIntegrity()
 
     QScopedPointer<Database> db(new Database());
     db->changeKdf(fastKdf(db->kdf()));
+    QCOMPARE(db->kdf()->uuid(), KeePass2::KDF_AES_KDBX3); // default is legacy AES-KDF
     db->setKey(compositeKey);
 
     // upgrade the database by a specific method
@@ -309,9 +320,12 @@ void TestKdbx4::testUpgradeMasterKeyIntegrity()
         QFAIL(qPrintable(reader.errorString()));
     }
     QCOMPARE(reader.version(), expectedVersion & KeePass2::FILE_VERSION_CRITICAL_MASK);
+    if (expectedVersion != KeePass2::FILE_VERSION_3) {
+        QVERIFY(db2->kdf()->uuid() != KeePass2::KDF_AES_KDBX3);
+    }
 }
 
-void TestKdbx4::testUpgradeMasterKeyIntegrity_data()
+void TestKdbx4Argon2::testUpgradeMasterKeyIntegrity_data()
 {
     QTest::addColumn<QString>("upgradeAction");
     QTest::addColumn<quint32>("expectedVersion");
@@ -330,7 +344,7 @@ void TestKdbx4::testUpgradeMasterKeyIntegrity_data()
     QTest::newRow("Upgrade (implicit): entry-customdata") << QString("entry-customdata") << KeePass2::FILE_VERSION_4;
 }
 
-void TestKdbx4::testCustomData()
+void TestKdbx4Argon2::testCustomData()
 {
     Database db;
 
@@ -424,13 +438,8 @@ void TestKdbx4::testCustomData()
     QCOMPARE(newEntry->customData()->value(customDataKey2), customData2);
 }
 
-QSharedPointer<Kdf> TestKdbx4::fastKdf(QSharedPointer<Kdf> kdf)
+void TestKdbx4AesKdf::initTestCaseImpl()
 {
-    kdf->setRounds(1);
-
-    if (kdf->uuid() == KeePass2::KDF_ARGON2) {
-        kdf->processParameters({{KeePass2::KDFPARAM_ARGON2_MEMORY, 1024}, {KeePass2::KDFPARAM_ARGON2_PARALLELISM, 1}});
-    }
-
-    return kdf;
+    m_xmlDb->changeKdf(fastKdf(KeePass2::uuidToKdf(KeePass2::KDF_AES_KDBX4)));
+    m_kdbxSourceDb->changeKdf(fastKdf(KeePass2::uuidToKdf(KeePass2::KDF_AES_KDBX4)));
 }
diff --git a/tests/TestKdbx4.h b/tests/TestKdbx4.h
index e97084148..53f2295e0 100644
--- a/tests/TestKdbx4.h
+++ b/tests/TestKdbx4.h
@@ -20,7 +20,7 @@
 
 #include "TestKeePass2Format.h"
 
-class TestKdbx4 : public TestKeePass2Format
+class TestKdbx4Argon2 : public TestKeePass2Format
 {
     Q_OBJECT
 
@@ -51,8 +51,14 @@ protected:
                   bool& hasError,
                   QString& errorString) override;
     void writeKdbx(QIODevice* device, Database* db, bool& hasError, QString& errorString) override;
+};
+
+class TestKdbx4AesKdf : public TestKdbx4Argon2
+{
+    Q_OBJECT
 
-    QSharedPointer<Kdf> fastKdf(QSharedPointer<Kdf> kdf);
+protected:
+    void initTestCaseImpl() override;
 };
 
 #endif // KEEPASSXC_TEST_KDBX4_H
diff --git a/tests/TestKeePass2Format.cpp b/tests/TestKeePass2Format.cpp
index 012a1b11a..ce4f63fed 100644
--- a/tests/TestKeePass2Format.cpp
+++ b/tests/TestKeePass2Format.cpp
@@ -22,7 +22,9 @@
 #include "core/Metadata.h"
 #include "crypto/Crypto.h"
 #include "format/KdbxXmlReader.h"
+#include "keys/FileKey.h"
 #include "keys/PasswordKey.h"
+#include "mock/MockChallengeResponseKey.h"
 
 #include "FailDevice.h"
 #include "config-keepassx-tests.h"
@@ -566,6 +568,168 @@ void TestKeePass2Format::testKdbxDeviceFailure()
     QCOMPARE(errorString, QString("FAILDEVICE"));
 }
 
+Q_DECLARE_METATYPE(QSharedPointer<CompositeKey>)
+
+void TestKeePass2Format::testKdbxKeyChange()
+{
+    QFETCH(QSharedPointer<CompositeKey>, key1);
+    QFETCH(QSharedPointer<CompositeKey>, key2);
+
+    bool hasError;
+    QString errorString;
+
+    // write new database
+    QBuffer buffer;
+    buffer.open(QBuffer::ReadWrite);
+    buffer.seek(0);
+    QSharedPointer<Database> db(new Database());
+    db->changeKdf(fastKdf(KeePass2::uuidToKdf(m_kdbxSourceDb->kdf()->uuid())));
+    db->setRootGroup(m_kdbxSourceDb->rootGroup()->clone(Entry::CloneNoFlags, Group::CloneIncludeEntries));
+
+    db->setKey(key1);
+    writeKdbx(&buffer, db.data(), hasError, errorString);
+    QVERIFY(!hasError);
+
+    // read database
+    db = QSharedPointer<Database>::create();
+    buffer.seek(0);
+    readKdbx(&buffer, key1, db, hasError, errorString);
+    if (hasError) {
+        QFAIL(qPrintable(QStringLiteral("Error while reading database: ").append(errorString)));
+    }
+    QVERIFY(db.data());
+
+    // change key
+    db->setKey(key2);
+
+    // write database
+    buffer.seek(0);
+    writeKdbx(&buffer, db.data(), hasError, errorString);
+    QVERIFY(!hasError);
+
+    // read database
+    db = QSharedPointer<Database>::create();
+    buffer.seek(0);
+    readKdbx(&buffer, key2, db, hasError, errorString);
+    if (hasError) {
+        QFAIL(qPrintable(QStringLiteral("Error while reading database: ").append(errorString)));
+    }
+    QVERIFY(db.data());
+    QVERIFY(db->rootGroup() != m_kdbxSourceDb->rootGroup());
+    QVERIFY(db->rootGroup()->uuid() == m_kdbxSourceDb->rootGroup()->uuid());
+}
+
+void TestKeePass2Format::testKdbxKeyChange_data()
+{
+    QTest::addColumn<QSharedPointer<CompositeKey>>("key1");
+    QTest::addColumn<QSharedPointer<CompositeKey>>("key2");
+
+    auto passwordKey1 = QSharedPointer<PasswordKey>::create("abc");
+    auto passwordKey2 = QSharedPointer<PasswordKey>::create("def");
+
+    QByteArray fileKeyBytes1("uvw");
+    QBuffer fileKeyBuffer1(&fileKeyBytes1);
+    fileKeyBuffer1.open(QBuffer::ReadOnly);
+    auto fileKey1 = QSharedPointer<FileKey>::create();
+    fileKey1->load(&fileKeyBuffer1);
+
+    QByteArray fileKeyBytes2("xzy");
+    QBuffer fileKeyBuffer2(&fileKeyBytes1);
+    fileKeyBuffer2.open(QBuffer::ReadOnly);
+    auto fileKey2 = QSharedPointer<FileKey>::create();
+    fileKey2->load(&fileKeyBuffer2);
+
+    auto crKey1 = QSharedPointer<MockChallengeResponseKey>::create(QByteArray("123"));
+    auto crKey2 = QSharedPointer<MockChallengeResponseKey>::create(QByteArray("456"));
+
+    // empty key
+    auto compositeKey0 = QSharedPointer<CompositeKey>::create();
+
+    // all in
+    auto compositeKey1_1 = QSharedPointer<CompositeKey>::create();
+    compositeKey1_1->addKey(passwordKey1);
+    compositeKey1_1->addKey(fileKey1);
+    compositeKey1_1->addChallengeResponseKey(crKey1);
+    auto compositeKey1_2 = QSharedPointer<CompositeKey>::create();
+    compositeKey1_2->addKey(passwordKey2);
+    compositeKey1_2->addKey(fileKey2);
+    compositeKey1_2->addChallengeResponseKey(crKey2);
+
+    QTest::newRow("Change:  Empty Key -> Full Key") << compositeKey0 << compositeKey1_1;
+    QTest::newRow("Change:   Full Key -> Empty Key") << compositeKey1_1 << compositeKey0;
+    QTest::newRow("Change: Full Key 1 -> Full Key 2") << compositeKey1_1 << compositeKey1_2;
+
+    // only password
+    auto compositeKey2_1 = QSharedPointer<CompositeKey>::create();
+    compositeKey2_1->addKey(passwordKey1);
+    auto compositeKey2_2 = QSharedPointer<CompositeKey>::create();
+    compositeKey2_2->addKey(passwordKey2);
+
+    QTest::newRow("Change:   Password -> Empty Key") << compositeKey2_1 << compositeKey0;
+    QTest::newRow("Change:  Empty Key -> Password") << compositeKey0 << compositeKey2_1;
+    QTest::newRow("Change:   Full Key -> Password 1") << compositeKey1_1 << compositeKey2_1;
+    QTest::newRow("Change:   Full Key -> Password 2") << compositeKey1_1 << compositeKey2_2;
+    QTest::newRow("Change: Password 1 -> Full Key") << compositeKey2_1 << compositeKey1_1;
+    QTest::newRow("Change: Password 2 -> Full Key") << compositeKey2_2 << compositeKey1_1;
+    QTest::newRow("Change: Password 1 -> Password 2") << compositeKey2_1 << compositeKey2_2;
+
+    // only key file
+    auto compositeKey3_1 = QSharedPointer<CompositeKey>::create();
+    compositeKey3_1->addKey(fileKey1);
+    auto compositeKey3_2 = QSharedPointer<CompositeKey>::create();
+    compositeKey3_2->addKey(fileKey2);
+
+    QTest::newRow("Change:   Key File -> Empty Key") << compositeKey3_1 << compositeKey0;
+    QTest::newRow("Change:  Empty Key -> Key File") << compositeKey0 << compositeKey3_1;
+    QTest::newRow("Change:   Full Key -> Key File 1") << compositeKey1_1 << compositeKey3_1;
+    QTest::newRow("Change:   Full Key -> Key File 2") << compositeKey1_1 << compositeKey3_2;
+    QTest::newRow("Change: Key File 1 -> Full Key") << compositeKey3_1 << compositeKey1_1;
+    QTest::newRow("Change: Key File 2 -> Full Key") << compositeKey3_2 << compositeKey1_1;
+    QTest::newRow("Change: Key File 1 -> Key File 2") << compositeKey3_1 << compositeKey3_2;
+
+    // only cr key
+    auto compositeKey4_1 = QSharedPointer<CompositeKey>::create();
+    compositeKey4_1->addChallengeResponseKey(crKey1);
+    auto compositeKey4_2 = QSharedPointer<CompositeKey>::create();
+    compositeKey4_2->addChallengeResponseKey(crKey2);
+
+    QTest::newRow("Change:    CR Key -> Empty Key") << compositeKey4_1 << compositeKey0;
+    QTest::newRow("Change: Empty Key -> CR Key") << compositeKey0 << compositeKey4_1;
+    QTest::newRow("Change:  Full Key -> CR Key 1") << compositeKey1_1 << compositeKey4_1;
+    QTest::newRow("Change:  Full Key -> CR Key 2") << compositeKey1_1 << compositeKey4_2;
+    QTest::newRow("Change:  CR Key 1 -> Full Key") << compositeKey4_1 << compositeKey1_1;
+    QTest::newRow("Change:  CR Key 2 -> Full Key") << compositeKey4_2 << compositeKey1_1;
+    QTest::newRow("Change:  CR Key 1 -> CR Key 2") << compositeKey4_1 << compositeKey4_2;
+
+    // rotate
+    QTest::newRow("Change: Password -> Key File") << compositeKey2_1 << compositeKey3_1;
+    QTest::newRow("Change: Key File -> Password") << compositeKey3_1 << compositeKey2_1;
+    QTest::newRow("Change: Password -> Key File") << compositeKey2_1 << compositeKey3_1;
+    QTest::newRow("Change: Key File -> Password") << compositeKey3_1 << compositeKey2_1;
+    QTest::newRow("Change: Password -> CR Key") << compositeKey2_1 << compositeKey4_1;
+    QTest::newRow("Change:   CR Key -> Password") << compositeKey4_1 << compositeKey2_1;
+    QTest::newRow("Change: Key File -> CR Key") << compositeKey3_1 << compositeKey4_1;
+    QTest::newRow("Change:   CR Key -> Key File") << compositeKey4_1 << compositeKey3_1;
+
+    // leave one out
+    auto compositeKey5_1 = QSharedPointer<CompositeKey>::create();
+    compositeKey5_1->addKey(fileKey1);
+    compositeKey5_1->addChallengeResponseKey(crKey1);
+    auto compositeKey5_2 = QSharedPointer<CompositeKey>::create();
+    compositeKey5_2->addKey(passwordKey1);
+    compositeKey5_2->addChallengeResponseKey(crKey1);
+    auto compositeKey5_3 = QSharedPointer<CompositeKey>::create();
+    compositeKey5_3->addKey(passwordKey1);
+    compositeKey5_3->addKey(fileKey1);
+
+    QTest::newRow("Change:    Full Key -> No Password") << compositeKey1_1 << compositeKey5_1;
+    QTest::newRow("Change: No Password -> Full Key") << compositeKey5_1 << compositeKey1_1;
+    QTest::newRow("Change:    Full Key -> No Key File") << compositeKey1_1 << compositeKey5_2;
+    QTest::newRow("Change: No Key File -> Full Key") << compositeKey5_2 << compositeKey1_1;
+    QTest::newRow("Change:    Full Key -> No CR Key") << compositeKey1_1 << compositeKey5_3;
+    QTest::newRow("Change:   No CR Key -> Full Key") << compositeKey5_3 << compositeKey1_1;
+}
+
 /**
  * Test for catching mapping errors with duplicate attachments.
  */
@@ -637,3 +801,17 @@ void TestKeePass2Format::testDuplicateAttachments()
     QCOMPARE(db->rootGroup()->entries()[2]->attachments()->value("c2"), attachment2);
     QCOMPARE(db->rootGroup()->entries()[2]->attachments()->value("c3"), attachment3);
 }
+
+/**
+ * @return fast "dummy" KDF
+ */
+QSharedPointer<Kdf> TestKeePass2Format::fastKdf(QSharedPointer<Kdf> kdf) const
+{
+    kdf->setRounds(1);
+
+    if (kdf->uuid() == KeePass2::KDF_ARGON2) {
+        kdf->processParameters({{KeePass2::KDFPARAM_ARGON2_MEMORY, 1024}, {KeePass2::KDFPARAM_ARGON2_PARALLELISM, 1}});
+    }
+
+    return kdf;
+}
diff --git a/tests/TestKeePass2Format.h b/tests/TestKeePass2Format.h
index 2d4a4b85a..728d73ba9 100644
--- a/tests/TestKeePass2Format.h
+++ b/tests/TestKeePass2Format.h
@@ -62,6 +62,8 @@ private slots:
     void testKdbxAttachments();
     void testKdbxNonAsciiPasswords();
     void testKdbxDeviceFailure();
+    void testKdbxKeyChange();
+    void testKdbxKeyChange_data();
     void testDuplicateAttachments();
 
 protected:
@@ -84,6 +86,8 @@ protected:
                           QString& errorString) = 0;
     virtual void writeKdbx(QIODevice* device, Database* db, bool& hasError, QString& errorString) = 0;
 
+    QSharedPointer<Kdf> fastKdf(QSharedPointer<Kdf> kdf) const;
+
     QSharedPointer<Database> m_xmlDb;
     QSharedPointer<Database> m_kdbxSourceDb;
     QSharedPointer<Database> m_kdbxTargetDb;