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.
keepassxreboot · Apr 1, 2024 · b4ea4ed · b4ea4ed
1 parent 194409a
commit b4ea4ed
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 18 deletions.
diff --git 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<const CompositeKey>& 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
@@ -188,30 +188,30 @@ public slots:
         QScopedPointer<PasswordKey> challengeResponseKey;
 
         QSharedPointer<const CompositeKey> key;
-        QSharedPointer<Kdf> kdf = QSharedPointer<AesKdf>::create(true);
+        QSharedPointer<Kdf> 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();
         }
     };
 

diff --git 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<Database> DatabaseOpenWidget::database()

diff --git 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<Database>::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<Database>::create();