From 6481ecccd79c1bff53887401dc2f6496331007af Mon Sep 17 00:00:00 2001 From: Jonathan White Date: Sun, 17 Mar 2024 10:15:15 -0400 Subject: [PATCH] Fix crash on screen lock or computer sleep * Fixes #10455 * Fixes #10432 * Fixes #10415 Prevent setting critical key components to nullptr when database data is cleared. This can result in a crash due to race condition between threads. Added a bunch of asserts to detect this problem and if guards to prevent actual crashes. --- src/core/Database.cpp | 19 ++++++++++++--- src/core/Database.h | 23 +++++++++++-------- src/gui/DatabaseOpenWidget.cpp | 4 +--- .../DatabaseSettingsWidgetEncryption.cpp | 15 ++++++------ tests/TestKeePass2Format.cpp | 8 +++++-- 5 files changed, 44 insertions(+), 25 deletions(-) diff --git a/src/core/Database.cpp b/src/core/Database.cpp index 14bba2f38..a3d569c91 100644 --- a/src/core/Database.cpp +++ b/src/core/Database.cpp @@ -412,6 +412,9 @@ bool Database::performSave(const QString& filePath, SaveAction action, const QSt bool Database::writeDatabase(QIODevice* device, QString* error) { + Q_ASSERT(m_data.key); + Q_ASSERT(m_data.transformedDatabaseKey); + PasswordKey oldTransformedKey; if (m_data.key->isEmpty()) { oldTransformedKey.setRawKey(m_data.transformedDatabaseKey->rawKey()); @@ -767,18 +770,29 @@ Database::CompressionAlgorithm Database::compressionAlgorithm() const QByteArray Database::transformedDatabaseKey() const { + Q_ASSERT(m_data.transformedDatabaseKey); + if (!m_data.transformedDatabaseKey) { + return {}; + } return m_data.transformedDatabaseKey->rawKey(); } QByteArray Database::challengeResponseKey() const { + Q_ASSERT(m_data.challengeResponseKey); + if (!m_data.challengeResponseKey) { + return {}; + } return m_data.challengeResponseKey->rawKey(); } bool Database::challengeMasterSeed(const QByteArray& masterSeed) { + Q_ASSERT(m_data.key); + Q_ASSERT(m_data.masterSeed); + m_keyError.clear(); - if (m_data.key) { + if (m_data.key && m_data.masterSeed) { m_data.masterSeed->setRawKey(masterSeed); QByteArray response; bool ok = m_data.key->challenge(masterSeed, response, &m_keyError); @@ -824,8 +838,7 @@ bool Database::setKey(const QSharedPointer& key, m_keyError.clear(); if (!key) { - m_data.key.reset(); - m_data.transformedDatabaseKey.reset(new PasswordKey()); + m_data.resetKeys(); return true; } diff --git a/src/core/Database.h b/src/core/Database.h index 31a839734..decceeecf 100644 --- a/src/core/Database.h +++ b/src/core/Database.h @@ -188,30 +188,33 @@ private: QScopedPointer challengeResponseKey; QSharedPointer key; - QSharedPointer kdf = QSharedPointer::create(true); + QSharedPointer kdf; QVariantMap publicCustomData; DatabaseData() - : masterSeed(new PasswordKey()) - , transformedDatabaseKey(new PasswordKey()) - , challengeResponseKey(new PasswordKey()) { - kdf->randomizeSeed(); + clear(); } void clear() { + resetKeys(); filePath.clear(); + publicCustomData.clear(); + } - masterSeed.reset(); - transformedDatabaseKey.reset(); - challengeResponseKey.reset(); + void resetKeys() + { + masterSeed.reset(new PasswordKey()); + transformedDatabaseKey.reset(new PasswordKey()); + challengeResponseKey.reset(new PasswordKey()); key.reset(); - kdf.reset(); - publicCustomData.clear(); + // Default to AES KDF, KDBX4 databases overwrite this + kdf.reset(new AesKdf(true)); + kdf->randomizeSeed(); } }; diff --git a/src/gui/DatabaseOpenWidget.cpp b/src/gui/DatabaseOpenWidget.cpp index a20c525af..ce5a4fd9c 100644 --- a/src/gui/DatabaseOpenWidget.cpp +++ b/src/gui/DatabaseOpenWidget.cpp @@ -264,9 +264,7 @@ void DatabaseOpenWidget::clearForms() m_ui->hardwareKeyCombo->clear(); toggleQuickUnlockScreen(); - QString error; - m_db.reset(new Database()); - m_db->open(m_filename, nullptr, &error); + m_db.reset(new Database(m_filename)); } QSharedPointer DatabaseOpenWidget::database() diff --git a/src/gui/dbsettings/DatabaseSettingsWidgetEncryption.cpp b/src/gui/dbsettings/DatabaseSettingsWidgetEncryption.cpp index c18f2876a..0b1c2eb6a 100644 --- a/src/gui/dbsettings/DatabaseSettingsWidgetEncryption.cpp +++ b/src/gui/dbsettings/DatabaseSettingsWidgetEncryption.cpp @@ -101,22 +101,23 @@ void DatabaseSettingsWidgetEncryption::initialize() } auto version = KDBX4; - if (m_db->kdf()) { + if (m_db->key() && m_db->kdf()) { version = (m_db->kdf()->uuid() == KeePass2::KDF_AES_KDBX3) ? KDBX3 : KDBX4; } m_ui->compatibilitySelection->setCurrentIndex(version); bool isNewDatabase = false; - if (!m_db->kdf()) { + if (!m_db->key()) { + m_db->setKey(QSharedPointer::create(), true, false, false); + m_db->setKdf(KeePass2::uuidToKdf(KeePass2::KDF_ARGON2D)); + m_db->setCipher(KeePass2::CIPHER_AES256); + isNewDatabase = true; + } else if (!m_db->kdf()) { m_db->setKdf(KeePass2::uuidToKdf(KeePass2::KDF_ARGON2D)); isNewDatabase = true; } - if (!m_db->key()) { - m_db->setKey(QSharedPointer::create(), true, false, false); - m_db->setCipher(KeePass2::CIPHER_AES256); - isNewDatabase = true; - } + bool kdbx3Enabled = KeePass2Writer::kdbxVersionRequired(m_db.data(), true, true) <= KeePass2::FILE_VERSION_3_1; // check if the DB's custom data has a decryption time setting stored diff --git a/tests/TestKeePass2Format.cpp b/tests/TestKeePass2Format.cpp index a1c7b20d4..5451fb62b 100644 --- a/tests/TestKeePass2Format.cpp +++ b/tests/TestKeePass2Format.cpp @@ -582,7 +582,9 @@ void TestKeePass2Format::testKdbxKeyChange() db->setKey(key1); writeKdbx(&buffer, db.data(), hasError, errorString); - QVERIFY(!hasError); + if (hasError) { + QFAIL(qPrintable(QStringLiteral("Error while reading database: ").append(errorString))); + } // read database db = QSharedPointer::create(); @@ -599,7 +601,9 @@ void TestKeePass2Format::testKdbxKeyChange() // write database buffer.seek(0); writeKdbx(&buffer, db.data(), hasError, errorString); - QVERIFY(!hasError); + if (hasError) { + QFAIL(qPrintable(QStringLiteral("Error while reading database: ").append(errorString))); + } // read database db = QSharedPointer::create();