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.
fabriz · Apr 29, 2024 · 3829bcd · 3829bcd
1 parent 4f12f57
commit 3829bcd
Show file tree

Hide file tree

Showing 9 changed files with 68 additions and 5 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -80,10 +80,12 @@ ShareObserver::Result ShareImport::containerInto(const QString& resolvedPath,
     auto key = QSharedPointer<CompositeKey>::create();
     key->addKey(QSharedPointer<PasswordKey>::create(reference.password));
     auto sourceDb = QSharedPointer<Database>::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
@@ -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<Database> dbDestination(createTestDatabase());
+    QScopedPointer<Database> 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
@@ -30,6 +30,7 @@ private slots:
     void cleanup();
     void testMergeIntoNew();
     void testMergeNoChanges();
+    void testMergeCustomData();
     void testResolveConflictNewer();
     void testResolveConflictExisting();
     void testResolveGroupConflictOlder();