From e9b2a1c8426b230dfb18f7779ce91d5b6926f628 Mon Sep 17 00:00:00 2001 From: Jonathan White Date: Sun, 28 Apr 2024 23:22:01 -0400 Subject: [PATCH 1/2] Prevent KeeShare from merging database custom data This issue previously caused parent databases to be marked as modified on unlock. This was because of the new protections against byte-by-byte side channel attacks adds a randomized string to the database custom data. We should never be merging database custom data with keeshare or imports since we are merging groups only. Also prevent overwrite of auto-generated custom data fields, Last Modified and Random Slug. --- src/core/CustomData.cpp | 7 ++++++ src/core/CustomData.h | 3 +++ src/core/Database.cpp | 2 +- src/core/Merger.cpp | 14 ++++++++++-- src/core/Merger.h | 2 ++ src/gui/DatabaseTabWidget.cpp | 1 + src/keeshare/ShareImport.cpp | 3 +++ tests/TestMerge.cpp | 40 +++++++++++++++++++++++++++++++++-- tests/TestMerge.h | 1 + 9 files changed, 68 insertions(+), 5 deletions(-) diff --git a/src/core/CustomData.cpp b/src/core/CustomData.cpp index a9c5bcd3b5..e005766326 100644 --- a/src/core/CustomData.cpp +++ b/src/core/CustomData.cpp @@ -25,6 +25,8 @@ const QString CustomData::Created = QStringLiteral("_CREATED"); const QString CustomData::BrowserKeyPrefix = QStringLiteral("KPXC_BROWSER_"); const QString CustomData::BrowserLegacyKeyPrefix = QStringLiteral("Public Key: "); const QString CustomData::ExcludeFromReportsLegacy = QStringLiteral("KnownBad"); +const QString CustomData::FdoSecretsExposedGroup = QStringLiteral("FDO_SECRETS_EXPOSED_GROUP"); +const QString CustomData::RandomSlug = QStringLiteral("KPXC_RANDOM_SLUG"); // Fallback item for return by reference static const CustomData::CustomDataItem NULL_ITEM{}; @@ -191,6 +193,11 @@ bool CustomData::isProtected(const QString& key) const return key.startsWith(CustomData::BrowserKeyPrefix) || key.startsWith(CustomData::Created); } +bool CustomData::isAutoGenerated(const QString& key) const +{ + return key == LastModified || key == RandomSlug; +} + bool CustomData::operator==(const CustomData& other) const { return (m_data == other.m_data); diff --git a/src/core/CustomData.h b/src/core/CustomData.h index f67fc61db4..a8ad044870 100644 --- a/src/core/CustomData.h +++ b/src/core/CustomData.h @@ -51,6 +51,7 @@ class CustomData : public ModifiableObject QDateTime lastModified() const; QDateTime lastModified(const QString& key) const; bool isProtected(const QString& key) const; + bool isAutoGenerated(const QString& key) const; void set(const QString& key, CustomDataItem item); void set(const QString& key, const QString& value, const QDateTime& lastModified = {}); void remove(const QString& key); @@ -68,6 +69,8 @@ class CustomData : public ModifiableObject static const QString Created; static const QString BrowserKeyPrefix; static const QString BrowserLegacyKeyPrefix; + static const QString FdoSecretsExposedGroup; + static const QString RandomSlug; // Pre-KDBX 4.1 static const QString ExcludeFromReportsLegacy; diff --git a/src/core/Database.cpp b/src/core/Database.cpp index a3d569c91d..14ddd87d92 100644 --- a/src/core/Database.cpp +++ b/src/core/Database.cpp @@ -276,7 +276,7 @@ bool Database::saveAs(const QString& filePath, SaveAction action, const QString& // Add random data to prevent side-channel data deduplication attacks int length = Random::instance()->randomUIntRange(64, 512); - m_metadata->customData()->set("KPXC_RANDOM_SLUG", Random::instance()->randomArray(length).toHex()); + m_metadata->customData()->set(CustomData::RandomSlug, Random::instance()->randomArray(length).toHex()); // Prevent destructive operations while saving QMutexLocker locker(&m_saveMutex); diff --git a/src/core/Merger.cpp b/src/core/Merger.cpp index 5e14e8452e..ef90422426 100644 --- a/src/core/Merger.cpp +++ b/src/core/Merger.cpp @@ -57,6 +57,11 @@ void Merger::resetForcedMergeMode() m_mode = Group::Default; } +void Merger::setSkipDatabaseCustomData(bool state) +{ + m_skipCustomData = state; +} + QStringList Merger::merge() { // Order of merge steps is important - it is possible that we @@ -514,6 +519,11 @@ Merger::ChangeList Merger::mergeMetadata(const MergeContext& context) } } + // Some merges shouldn't modify the database custom data + if (m_skipCustomData) { + return changes; + } + // Merge Custom Data if source is newer const auto targetCustomDataModificationTime = targetMetadata->customData()->lastModified(); const auto sourceCustomDataModificationTime = sourceMetadata->customData()->lastModified(); @@ -535,8 +545,8 @@ Merger::ChangeList Merger::mergeMetadata(const MergeContext& context) // Transfer new/existing keys for (const auto& key : sourceCustomDataKeys) { - // Don't merge this meta field, it is updated automatically. - if (key == CustomData::LastModified) { + // Don't merge auto-generated keys + if (sourceMetadata->customData()->isAutoGenerated(key)) { continue; } diff --git a/src/core/Merger.h b/src/core/Merger.h index e98f7a062f..75c8da9902 100644 --- a/src/core/Merger.h +++ b/src/core/Merger.h @@ -31,6 +31,7 @@ class Merger : public QObject Merger(const Group* sourceGroup, Group* targetGroup); void setForcedMergeMode(Group::MergeMode mode); void resetForcedMergeMode(); + void setSkipDatabaseCustomData(bool state); QStringList merge(); private: @@ -66,6 +67,7 @@ class Merger : public QObject private: MergeContext m_context; Group::MergeMode m_mode; + bool m_skipCustomData = false; }; #endif // KEEPASSXC_MERGER_H diff --git a/src/gui/DatabaseTabWidget.cpp b/src/gui/DatabaseTabWidget.cpp index 0cff4a6b8c..1333b5fefa 100644 --- a/src/gui/DatabaseTabWidget.cpp +++ b/src/gui/DatabaseTabWidget.cpp @@ -272,6 +272,7 @@ DatabaseWidget* DatabaseTabWidget::importFile() if (newDb) { // Merge the imported db into the new one Merger merger(db.data(), newDb.data()); + merger.setSkipDatabaseCustomData(true); merger.merge(); // Show the new database auto dbWidget = new DatabaseWidget(newDb, this); diff --git a/src/keeshare/ShareImport.cpp b/src/keeshare/ShareImport.cpp index eb93912e76..da978423fb 100644 --- a/src/keeshare/ShareImport.cpp +++ b/src/keeshare/ShareImport.cpp @@ -80,10 +80,12 @@ ShareObserver::Result ShareImport::containerInto(const QString& resolvedPath, auto key = QSharedPointer::create(); key->addKey(QSharedPointer::create(reference.password)); auto sourceDb = QSharedPointer::create(); + sourceDb->setEmitModified(false); if (!reader.readDatabase(&buffer, key, sourceDb.data())) { qCritical("Error while parsing the database: %s", qPrintable(reader.errorString())); return {reference.path, ShareObserver::Result::Error, reader.errorString()}; } + sourceDb->setEmitModified(true); qDebug("Synchronize %s %s with %s", qPrintable(reference.path), @@ -92,6 +94,7 @@ ShareObserver::Result ShareImport::containerInto(const QString& resolvedPath, Merger merger(sourceDb->rootGroup(), targetGroup); merger.setForcedMergeMode(Group::Synchronize); + merger.setSkipDatabaseCustomData(true); auto changelist = merger.merge(); if (!changelist.isEmpty()) { return {reference.path, ShareObserver::Result::Success, ShareImport::tr("Successful import")}; diff --git a/tests/TestMerge.cpp b/tests/TestMerge.cpp index d2fd5a000b..3dde3a063c 100644 --- a/tests/TestMerge.cpp +++ b/tests/TestMerge.cpp @@ -87,20 +87,56 @@ void TestMerge::testMergeNoChanges() m_clock->advanceSecond(1); Merger merger1(dbSource.data(), dbDestination.data()); - merger1.merge(); + auto changes = merger1.merge(); + QVERIFY(changes.isEmpty()); QCOMPARE(dbDestination->rootGroup()->entriesRecursive().size(), 2); QCOMPARE(dbSource->rootGroup()->entriesRecursive().size(), 2); m_clock->advanceSecond(1); Merger merger2(dbSource.data(), dbDestination.data()); - merger2.merge(); + changes = merger2.merge(); + QVERIFY(changes.isEmpty()); QCOMPARE(dbDestination->rootGroup()->entriesRecursive().size(), 2); QCOMPARE(dbSource->rootGroup()->entriesRecursive().size(), 2); } +/** + * Merging without database custom data (used by imports and KeeShare) + */ +void TestMerge::testMergeCustomData() +{ + QScopedPointer dbDestination(createTestDatabase()); + QScopedPointer dbSource( + createTestDatabaseStructureClone(dbDestination.data(), Entry::CloneNoFlags, Group::CloneIncludeEntries)); + + QCOMPARE(dbDestination->rootGroup()->entriesRecursive().size(), 2); + QCOMPARE(dbSource->rootGroup()->entriesRecursive().size(), 2); + + dbDestination->metadata()->customData()->set("TEST_CUSTOM_DATA", "OLD TESTING"); + + m_clock->advanceSecond(1); + + dbSource->metadata()->customData()->set("TEST_CUSTOM_DATA", "TESTING"); + + // First check that the custom data is not merged when skipped + Merger merger1(dbSource.data(), dbDestination.data()); + merger1.setSkipDatabaseCustomData(true); + auto changes = merger1.merge(); + + QVERIFY(changes.isEmpty()); + QCOMPARE(dbDestination->metadata()->customData()->value("TEST_CUSTOM_DATA"), QString("OLD TESTING")); + + // Second check that the custom data is merged otherwise + Merger merger2(dbSource.data(), dbDestination.data()); + changes = merger2.merge(); + + QCOMPARE(changes.size(), 1); + QCOMPARE(dbDestination->metadata()->customData()->value("TEST_CUSTOM_DATA"), QString("TESTING")); +} + /** * If the entry is updated in the source database, the update * should propagate in the destination database. diff --git a/tests/TestMerge.h b/tests/TestMerge.h index 2f4e4a224f..b9ea54f729 100644 --- a/tests/TestMerge.h +++ b/tests/TestMerge.h @@ -30,6 +30,7 @@ private slots: void cleanup(); void testMergeIntoNew(); void testMergeNoChanges(); + void testMergeCustomData(); void testResolveConflictNewer(); void testResolveConflictExisting(); void testResolveGroupConflictOlder(); From ada78b74148e687880e073aa49990fc5c12dcc8d Mon Sep 17 00:00:00 2001 From: Jonathan White Date: Mon, 18 Mar 2024 07:45:31 -0400 Subject: [PATCH 2/2] Preserve Secret Service exposed group setting on merge * Fixes #9371 - adds secret service custom data key to the list of protected custom data (will not be overwritten on merge) --- src/core/CustomData.cpp | 2 +- src/fdosecrets/FdoSecretsSettings.cpp | 18 ++++-------------- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/src/core/CustomData.cpp b/src/core/CustomData.cpp index e005766326..354dbf7d6b 100644 --- a/src/core/CustomData.cpp +++ b/src/core/CustomData.cpp @@ -190,7 +190,7 @@ void CustomData::updateLastModified(QDateTime lastModified) bool CustomData::isProtected(const QString& key) const { - return key.startsWith(CustomData::BrowserKeyPrefix) || key.startsWith(CustomData::Created); + return key.startsWith(BrowserKeyPrefix) || key == Created || key == FdoSecretsExposedGroup; } bool CustomData::isAutoGenerated(const QString& key) const diff --git a/src/fdosecrets/FdoSecretsSettings.cpp b/src/fdosecrets/FdoSecretsSettings.cpp index 2f1e951829..d24bc69a12 100644 --- a/src/fdosecrets/FdoSecretsSettings.cpp +++ b/src/fdosecrets/FdoSecretsSettings.cpp @@ -21,15 +21,6 @@ #include "core/Database.h" #include "core/Metadata.h" -namespace Keys -{ - namespace Db - { - constexpr auto FdoSecretsExposedGroup = "FDO_SECRETS_EXPOSED_GROUP"; - } // namespace Db - -} // namespace Keys - namespace FdoSecrets { @@ -98,20 +89,19 @@ namespace FdoSecrets return exposedGroup(db.data()); } - void FdoSecretsSettings::setExposedGroup(const QSharedPointer& db, - const QUuid& group) // clazy:exclude=function-args-by-value + void FdoSecretsSettings::setExposedGroup(const QSharedPointer& db, const QUuid& group) { setExposedGroup(db.data(), group); } QUuid FdoSecretsSettings::exposedGroup(Database* db) const { - return {db->metadata()->customData()->value(Keys::Db::FdoSecretsExposedGroup)}; + return {db->metadata()->customData()->value(CustomData::FdoSecretsExposedGroup)}; } - void FdoSecretsSettings::setExposedGroup(Database* db, const QUuid& group) // clazy:exclude=function-args-by-value + void FdoSecretsSettings::setExposedGroup(Database* db, const QUuid& group) { - db->metadata()->customData()->set(Keys::Db::FdoSecretsExposedGroup, group.toString()); + db->metadata()->customData()->set(CustomData::FdoSecretsExposedGroup, group.toString()); } } // namespace FdoSecrets