Improve memory hygiene

src/core/Database.cpp

@@ -72,11 +72,7 @@ Database::Database(const QString& filePath)
 
 Database::~Database()
 {
-    s_uuidMap.remove(m_uuid);
-
-    if (m_modified) {
-        emit databaseDiscarded();
-    }
+    releaseData();
 }
 
 QUuid Database::uuid() const
@@ -320,7 +316,11 @@ bool Database::writeDatabase(QIODevice* device, QString* error)
         return false;
     }
 
-    QByteArray oldTransformedKey = m_data.transformedMasterKey;
+    PasswordKey oldTransformedKey;
+    if (m_data.hasKey) {
+        oldTransformedKey.setHash(m_data.transformedMasterKey->rawKey());
+    }
+
     KeePass2Writer writer;
     setEmitModified(false);
     writer.writeDatabase(device, this);
@@ -333,9 +333,10 @@ bool Database::writeDatabase(QIODevice* device, QString* error)
         return false;
     }
 
-    Q_ASSERT(!m_data.transformedMasterKey.isEmpty());
-    Q_ASSERT(m_data.transformedMasterKey != oldTransformedKey);
-    if (m_data.transformedMasterKey.isEmpty() || m_data.transformedMasterKey == oldTransformedKey) {
+    QByteArray newKey = m_data.transformedMasterKey->rawKey();
+    Q_ASSERT(!newKey.isEmpty());
+    Q_ASSERT(newKey != oldTransformedKey.rawKey());
+    if (newKey.isEmpty() || newKey == oldTransformedKey.rawKey()) {
         if (error) {
             *error = tr("Key not transformed. This is a bug, please report it to the developers!");
         }
@@ -378,6 +379,43 @@ bool Database::import(const QString& xmlExportPath, QString* error)
     return true;
 }
 
+/**
+ * Release all stored group, entry, and meta data of this database.
+ *
+ * Call this method to ensure all data is cleared even if valid
+ * pointers to this Database object are still being held.
+ *
+ * A previously reparented root group will not be freed.
+ */
+
+void Database::releaseData()
+{
+    s_uuidMap.remove(m_uuid);
+    m_uuid = QUuid();
+
+    if (m_modified) {
+        emit databaseDiscarded();
+    }
+
+    m_data.clear();
+
+    if (m_rootGroup && m_rootGroup->parent() == this) {
+        delete m_rootGroup;
+    }
+    if (m_metadata) {
+        delete m_metadata;
+    }
+    if (m_fileWatcher) {
+        delete m_fileWatcher;
+    }
+
+    m_deletedObjects.clear();
+    m_commonUsernames.clear();
+
+    m_initialized = false;
+    m_modified = false;
+}
+
 /**
  * Remove the old backup and replace it with a new one
  * backups are named <filename>.old.<extension>
@@ -598,19 +636,24 @@ Database::CompressionAlgorithm Database::compressionAlgorithm() const
 
 QByteArray Database::transformedMasterKey() const
 {
-    return m_data.transformedMasterKey;
+    return m_data.transformedMasterKey->rawKey();
 }
 
 QByteArray Database::challengeResponseKey() const
 {
-    return m_data.challengeResponseKey;
+    return m_data.challengeResponseKey->rawKey();
 }
 
 bool Database::challengeMasterSeed(const QByteArray& masterSeed)
 {
     if (m_data.key) {
-        m_data.masterSeed = masterSeed;
-        return m_data.key->challenge(masterSeed, m_data.challengeResponseKey);
+        m_data.masterSeed->setHash(masterSeed);
+        QByteArray response;
+        bool ok = m_data.key->challenge(masterSeed, response);
+        if (ok && !response.isEmpty()) {
+            m_data.challengeResponseKey->setHash(response);
+        }
+        return ok;
     }
     return false;
 }
@@ -647,7 +690,7 @@ bool Database::setKey(const QSharedPointer<const CompositeKey>& key,
 
     if (!key) {
         m_data.key.reset();
-        m_data.transformedMasterKey = {};
+        m_data.transformedMasterKey.reset(new PasswordKey());
         m_data.hasKey = false;
         return true;
     }
@@ -657,22 +700,29 @@ bool Database::setKey(const QSharedPointer<const CompositeKey>& key,
         Q_ASSERT(!m_data.kdf->seed().isEmpty());
     }
 
-    QByteArray oldTransformedMasterKey = m_data.transformedMasterKey;
+    PasswordKey oldTransformedMasterKey;
+    if (m_data.hasKey) {
+        oldTransformedMasterKey.setHash(m_data.transformedMasterKey->rawKey());
+    }
+
     QByteArray transformedMasterKey;
+
     if (!transformKey) {
-        transformedMasterKey = oldTransformedMasterKey;
+        transformedMasterKey = QByteArray(oldTransformedMasterKey.rawKey());
     } else if (!key->transform(*m_data.kdf, transformedMasterKey)) {
         return false;
     }
 
     m_data.key = key;
-    m_data.transformedMasterKey = transformedMasterKey;
+    if (!transformedMasterKey.isEmpty()) {
+        m_data.transformedMasterKey->setHash(transformedMasterKey);
+    }
     m_data.hasKey = true;
     if (updateChangedTime) {
         m_metadata->setMasterKeyChanged(Clock::currentDateTimeUtc());
     }
 
-    if (oldTransformedMasterKey != m_data.transformedMasterKey) {
+    if (oldTransformedMasterKey.rawKey() != m_data.transformedMasterKey->rawKey()) {
         markAsModified();
     }
 
@@ -688,15 +738,15 @@ bool Database::verifyKey(const QSharedPointer<CompositeKey>& key) const
 {
     Q_ASSERT(hasKey());
 
-    if (!m_data.challengeResponseKey.isEmpty()) {
+    if (!m_data.challengeResponseKey->rawKey().isEmpty()) {
         QByteArray result;
 
-        if (!key->challenge(m_data.masterSeed, result)) {
+        if (!key->challenge(m_data.masterSeed->rawKey(), result)) {
             // challenge failed, (YubiKey?) removed?
             return false;
         }
 
-        if (m_data.challengeResponseKey != result) {
+        if (m_data.challengeResponseKey->rawKey() != result) {
             // wrong response from challenged device(s)
             return false;
         }
@@ -861,7 +911,7 @@ bool Database::changeKdf(const QSharedPointer<Kdf>& kdf)
     }
 
     setKdf(kdf);
-    m_data.transformedMasterKey = transformedMasterKey;
+    m_data.transformedMasterKey->setHash(transformedMasterKey);
     markAsModified();
 
     return true;

src/core/Database.h

@@ -23,11 +23,13 @@
 #include <QHash>
 #include <QObject>
 #include <QPointer>
+#include <QScopedPointer>
 
 #include "config-keepassx.h"
 #include "crypto/kdf/AesKdf.h"
 #include "crypto/kdf/Kdf.h"
 #include "format/KeePass2.h"
+#include "keys/PasswordKey.h"
 #include "keys/CompositeKey.h"
 
 class Entry;
@@ -76,6 +78,8 @@ class Database : public QObject
     bool extract(QByteArray&, QString* error = nullptr);
     bool import(const QString& xmlExportPath, QString* error = nullptr);
 
+    void releaseData();
+
     bool isInitialized() const;
     void setInitialized(bool initialized);
     bool isModified() const;
@@ -161,18 +165,39 @@ private slots:
         bool isReadOnly = false;
         QUuid cipher = KeePass2::CIPHER_AES256;
         CompressionAlgorithm compressionAlgorithm = CompressionGZip;
-        QByteArray transformedMasterKey;
-        QSharedPointer<Kdf> kdf = QSharedPointer<AesKdf>::create(true);
-        QSharedPointer<const CompositeKey> key;
+
+        QScopedPointer<PasswordKey> masterSeed;
+        QScopedPointer<PasswordKey> transformedMasterKey;
+        QScopedPointer<PasswordKey> challengeResponseKey;
+
         bool hasKey = false;
-        QByteArray masterSeed;
-        QByteArray challengeResponseKey;
+        QSharedPointer<const CompositeKey> key;
+        QSharedPointer<Kdf> kdf = QSharedPointer<AesKdf>::create(true);
+
         QVariantMap publicCustomData;
 
         DatabaseData()
+            : masterSeed(new PasswordKey())
+            , transformedMasterKey(new PasswordKey())
+            , challengeResponseKey(new PasswordKey())
         {
             kdf->randomizeSeed();
         }
+
+        void clear()
+        {
+            filePath.clear();
+
+            masterSeed.reset();
+            transformedMasterKey.reset();
+            challengeResponseKey.reset();
+
+            hasKey = false;
+            key.reset();
+            kdf.reset();
+
+            publicCustomData.clear();
+        }
     };
 
     void createRecycleBin();
@@ -182,9 +207,9 @@ private slots:
     bool restoreDatabase(const QString& filePath);
     bool performSave(const QString& filePath, QString* error, bool atomic, bool backup);
 
-    Metadata* const m_metadata;
+    QPointer<Metadata> const m_metadata;
     DatabaseData m_data;
-    Group* m_rootGroup;
+    QPointer<Group> m_rootGroup;
     QList<DeletedObject> m_deletedObjects;
     QPointer<QTimer> m_timer;
     QPointer<FileWatcher> m_fileWatcher;

src/gui/DatabaseWidget.cpp

@@ -413,6 +413,8 @@ void DatabaseWidget::replaceDatabase(QSharedPointer<Database> db)
     // Keep the instance active till the end of this function
     Q_UNUSED(oldDb);
 #endif
+
+    oldDb->releaseData();
 }
 
 void DatabaseWidget::cloneEntry()

src/keys/PasswordKey.cpp

@@ -48,19 +48,29 @@ PasswordKey::~PasswordKey()
     }
 }
 
-QSharedPointer<PasswordKey> PasswordKey::fromRawKey(const QByteArray& rawKey)
-{
-    auto result = QSharedPointer<PasswordKey>::create();
-    std::memcpy(result->m_key, rawKey.data(), std::min(SHA256_SIZE, rawKey.size()));
-    return result;
-}
-
 QByteArray PasswordKey::rawKey() const
 {
+    if (!m_isInitialized) {
+        return {};
+    }
     return QByteArray::fromRawData(m_key, SHA256_SIZE);
 }
 
 void PasswordKey::setPassword(const QString& password)
 {
-    std::memcpy(m_key, CryptoHash::hash(password.toUtf8(), CryptoHash::Sha256).data(), SHA256_SIZE);
+    setHash(CryptoHash::hash(password.toUtf8(), CryptoHash::Sha256));
+}
+
+void PasswordKey::setHash(const QByteArray& hash)
+{
+    Q_ASSERT(hash.size() == SHA256_SIZE);
+    std::memcpy(m_key, hash.data(), std::min(SHA256_SIZE, hash.size()));
+    m_isInitialized = true;
+}
+
+QSharedPointer<PasswordKey> PasswordKey::fromRawKey(const QByteArray& rawKey)
+{
+    auto result = QSharedPointer<PasswordKey>::create();
+    result->setHash(rawKey);
+    return result;
 }

src/keys/PasswordKey.h

@@ -33,13 +33,15 @@ class PasswordKey : public Key
     ~PasswordKey() override;
     QByteArray rawKey() const override;
     void setPassword(const QString& password);
+    void setHash(const QByteArray& hash);
 
     static QSharedPointer<PasswordKey> fromRawKey(const QByteArray& rawKey);
 
 private:
     static constexpr int SHA256_SIZE = 32;
 
     char* m_key = nullptr;
+    bool m_isInitialized = false;
 };
 
 #endif // KEEPASSX_PASSWORDKEY_H