From 5996ba51c9a1bbcb830a0defcf46b70ddc387c8f Mon Sep 17 00:00:00 2001 From: Janek Bevendorff Date: Fri, 8 Nov 2019 22:21:33 +0100 Subject: [PATCH] Use PasswordKey for storing transformed secrets. The transformed secrets were stored in normal QByteArrays, which are at risk of being swapped out. We now use secure PasswordKey objects instead. There are still a few areas where QByteArrays are used for storing secrets, but since they are all temporary, they are less critical. It may be worth hunting those down as well, though. --- src/core/Database.cpp | 54 ++++++++++++++++++++++++++-------------- src/core/Database.h | 34 +++++++++++++++++++++---- src/keys/PasswordKey.cpp | 26 +++++++++++++------ src/keys/PasswordKey.h | 2 ++ 4 files changed, 85 insertions(+), 31 deletions(-) diff --git a/src/core/Database.cpp b/src/core/Database.cpp index e62124d0..4cccd6d5 100644 --- a/src/core/Database.cpp +++ b/src/core/Database.cpp @@ -316,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); @@ -329,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!"); } @@ -382,6 +387,7 @@ bool Database::import(const QString& xmlExportPath, QString* error) * * A previously reparented root group will not be freed. */ + void Database::releaseData() { s_uuidMap.remove(m_uuid); @@ -391,7 +397,7 @@ void Database::releaseData() emit databaseDiscarded(); } - m_data = DatabaseData(); + m_data.clear(); if (m_rootGroup && m_rootGroup->parent() == this) { delete m_rootGroup; @@ -630,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; } @@ -679,7 +690,7 @@ bool Database::setKey(const QSharedPointer& key, if (!key) { m_data.key.reset(); - m_data.transformedMasterKey = {}; + m_data.transformedMasterKey.reset(new PasswordKey()); m_data.hasKey = false; return true; } @@ -689,22 +700,29 @@ bool Database::setKey(const QSharedPointer& 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(); } @@ -720,15 +738,15 @@ bool Database::verifyKey(const QSharedPointer& 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; } @@ -893,7 +911,7 @@ bool Database::changeKdf(const QSharedPointer& kdf) } setKdf(kdf); - m_data.transformedMasterKey = transformedMasterKey; + m_data.transformedMasterKey->setHash(transformedMasterKey); markAsModified(); return true; diff --git a/src/core/Database.h b/src/core/Database.h index afb89271..9c992994 100644 --- a/src/core/Database.h +++ b/src/core/Database.h @@ -23,12 +23,15 @@ #include #include #include +#include #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; enum class EntryReferenceType; class FileWatcher; @@ -162,18 +165,39 @@ private: bool isReadOnly = false; QUuid cipher = KeePass2::CIPHER_AES256; CompressionAlgorithm compressionAlgorithm = CompressionGZip; - QByteArray transformedMasterKey; - QSharedPointer kdf = QSharedPointer::create(true); - QSharedPointer key; + + QScopedPointer masterSeed; + QScopedPointer transformedMasterKey; + QScopedPointer challengeResponseKey; + bool hasKey = false; - QByteArray masterSeed; - QByteArray challengeResponseKey; + QSharedPointer key; + QSharedPointer kdf = QSharedPointer::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(); diff --git a/src/keys/PasswordKey.cpp b/src/keys/PasswordKey.cpp index 77d2f276..4393a178 100644 --- a/src/keys/PasswordKey.cpp +++ b/src/keys/PasswordKey.cpp @@ -48,19 +48,29 @@ PasswordKey::~PasswordKey() } } -QSharedPointer PasswordKey::fromRawKey(const QByteArray& rawKey) -{ - auto result = QSharedPointer::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::fromRawKey(const QByteArray& rawKey) +{ + auto result = QSharedPointer::create(); + result->setHash(rawKey); + return result; } diff --git a/src/keys/PasswordKey.h b/src/keys/PasswordKey.h index 4408cabc..b8450667 100644 --- a/src/keys/PasswordKey.h +++ b/src/keys/PasswordKey.h @@ -33,6 +33,7 @@ public: ~PasswordKey() override; QByteArray rawKey() const override; void setPassword(const QString& password); + void setHash(const QByteArray& hash); static QSharedPointer fromRawKey(const QByteArray& rawKey); @@ -40,6 +41,7 @@ private: static constexpr int SHA256_SIZE = 32; char* m_key = nullptr; + bool m_isInitialized = false; }; #endif // KEEPASSX_PASSWORDKEY_H