From 247ebf5a35f513f145ea4604ccb76b12cf0e45b3 Mon Sep 17 00:00:00 2001 From: Janek Bevendorff Date: Fri, 10 Jan 2020 02:11:43 +0100 Subject: [PATCH] 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 --- src/core/Database.cpp | 3 + tests/CMakeLists.txt | 2 +- tests/TestKdbx3.cpp | 2 + tests/TestKdbx4.cpp | 69 ++++++++------ tests/TestKdbx4.h | 10 +- tests/TestKeePass2Format.cpp | 178 +++++++++++++++++++++++++++++++++++ tests/TestKeePass2Format.h | 4 + 7 files changed, 235 insertions(+), 33 deletions(-) diff --git a/src/core/Database.cpp b/src/core/Database.cpp index 066abc4e3..fcd48f1e2 100644 --- a/src/core/Database.cpp +++ b/src/core/Database.cpp @@ -651,6 +651,9 @@ bool Database::challengeMasterSeed(const QByteArray& masterSeed) bool ok = m_data.key->challenge(masterSeed, response); if (ok && !response.isEmpty()) { m_data.challengeResponseKey->setHash(response); + } else if (ok && response.isEmpty()) { + // no CR key present, make sure buffer is empty + m_data.challengeResponseKey.reset(new PasswordKey); } return ok; } 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 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 -void TestKdbx4::initTestCaseImpl() + TestKdbx4Argon2 argon2Test; + TestKdbx4AesKdf aesKdfTest; + return QTest::qExec(&argon2Test, argc, argv) | QTest::qExec(&aesKdfTest, argc, argv); +} + +void TestKdbx4Argon2::initTestCaseImpl() { m_xmlDb->changeKdf(fastKdf(KeePass2::uuidToKdf(KeePass2::KDF_ARGON2))); m_kdbxSourceDb->changeKdf(fastKdf(KeePass2::uuidToKdf(KeePass2::KDF_ARGON2))); } -QSharedPointer TestKdbx4::readXml(const QString& path, bool strictMode, bool& hasError, QString& errorString) +QSharedPointer +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 TestKdbx4::readXml(const QString& path, bool strictMode return db; } -QSharedPointer TestKdbx4::readXml(QBuffer* buf, bool strictMode, bool& hasError, QString& errorString) +QSharedPointer TestKdbx4Argon2::readXml(QBuffer* buf, bool strictMode, bool& hasError, QString& errorString) { KdbxXmlReader reader(KeePass2::FILE_VERSION_4); reader.setStrictMode(strictMode); @@ -57,7 +67,7 @@ QSharedPointer 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 key, - QSharedPointer db, - bool& hasError, - QString& errorString) +void TestKdbx4Argon2::readKdbx(QIODevice* device, + QSharedPointer key, + QSharedPointer 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 key, - QSharedPointer db, - bool& hasError, - QString& errorString) +void TestKdbx4Argon2::readKdbx(const QString& path, + QSharedPointer key, + QSharedPointer 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::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("kdfUuid"); QTest::addColumn("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 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("upgradeAction"); QTest::addColumn("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 TestKdbx4::fastKdf(QSharedPointer 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; +}; - QSharedPointer fastKdf(QSharedPointer kdf); +class TestKdbx4AesKdf : public TestKdbx4Argon2 +{ + Q_OBJECT + +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) + +void TestKeePass2Format::testKdbxKeyChange() +{ + QFETCH(QSharedPointer, key1); + QFETCH(QSharedPointer, key2); + + bool hasError; + QString errorString; + + // write new database + QBuffer buffer; + buffer.open(QBuffer::ReadWrite); + buffer.seek(0); + QSharedPointer 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::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::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>("key1"); + QTest::addColumn>("key2"); + + auto passwordKey1 = QSharedPointer::create("abc"); + auto passwordKey2 = QSharedPointer::create("def"); + + QByteArray fileKeyBytes1("uvw"); + QBuffer fileKeyBuffer1(&fileKeyBytes1); + fileKeyBuffer1.open(QBuffer::ReadOnly); + auto fileKey1 = QSharedPointer::create(); + fileKey1->load(&fileKeyBuffer1); + + QByteArray fileKeyBytes2("xzy"); + QBuffer fileKeyBuffer2(&fileKeyBytes1); + fileKeyBuffer2.open(QBuffer::ReadOnly); + auto fileKey2 = QSharedPointer::create(); + fileKey2->load(&fileKeyBuffer2); + + auto crKey1 = QSharedPointer::create(QByteArray("123")); + auto crKey2 = QSharedPointer::create(QByteArray("456")); + + // empty key + auto compositeKey0 = QSharedPointer::create(); + + // all in + auto compositeKey1_1 = QSharedPointer::create(); + compositeKey1_1->addKey(passwordKey1); + compositeKey1_1->addKey(fileKey1); + compositeKey1_1->addChallengeResponseKey(crKey1); + auto compositeKey1_2 = QSharedPointer::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::create(); + compositeKey2_1->addKey(passwordKey1); + auto compositeKey2_2 = QSharedPointer::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::create(); + compositeKey3_1->addKey(fileKey1); + auto compositeKey3_2 = QSharedPointer::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::create(); + compositeKey4_1->addChallengeResponseKey(crKey1); + auto compositeKey4_2 = QSharedPointer::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::create(); + compositeKey5_1->addKey(fileKey1); + compositeKey5_1->addChallengeResponseKey(crKey1); + auto compositeKey5_2 = QSharedPointer::create(); + compositeKey5_2->addKey(passwordKey1); + compositeKey5_2->addChallengeResponseKey(crKey1); + auto compositeKey5_3 = QSharedPointer::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 TestKeePass2Format::fastKdf(QSharedPointer 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 fastKdf(QSharedPointer kdf) const; + QSharedPointer m_xmlDb; QSharedPointer m_kdbxSourceDb; QSharedPointer m_kdbxTargetDb;