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.

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();

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

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);

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")};

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.

tests/TestMerge.h

@@ -30,6 +30,7 @@ private slots:
     void cleanup();
     void testMergeIntoNew();
     void testMergeNoChanges();
+    void testMergeCustomData();
     void testResolveConflictNewer();
     void testResolveConflictExisting();
     void testResolveGroupConflictOlder();