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
This commit is contained in:
Janek Bevendorff 2020-01-10 02:11:43 +01:00
parent cba8947ee8
commit 247ebf5a35
7 changed files with 235 additions and 33 deletions

View File

@ -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;
}

View File

@ -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

View File

@ -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)

View File

@ -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<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,7 +75,7 @@ void TestKdbx4::writeXml(QBuffer* buf, Database* db, bool& hasError, QString& er
errorString = writer.errorString();
}
void TestKdbx4::readKdbx(QIODevice* device,
void TestKdbx4Argon2::readKdbx(QIODevice* device,
QSharedPointer<const CompositeKey> key,
QSharedPointer<Database> db,
bool& hasError,
@ -80,7 +90,7 @@ void TestKdbx4::readKdbx(QIODevice* device,
QCOMPARE(reader.version(), KeePass2::FILE_VERSION_4);
}
void TestKdbx4::readKdbx(const QString& path,
void TestKdbx4Argon2::readKdbx(const QString& path,
QSharedPointer<const CompositeKey> key,
QSharedPointer<Database> db,
bool& hasError,
@ -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)));
}

View File

@ -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<Kdf> fastKdf(QSharedPointer<Kdf> kdf);
class TestKdbx4AesKdf : public TestKdbx4Argon2
{
Q_OBJECT
protected:
void initTestCaseImpl() override;
};
#endif // KEEPASSXC_TEST_KDBX4_H

View File

@ -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;
}

View File

@ -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;