Namespace deduplication of KeeShare attachments

src/format/Kdbx4Writer.cpp

@@ -21,8 +21,9 @@
 
 #include "crypto/CryptoHash.h"
 #include "crypto/Random.h"
-#include "format/KdbxXmlWriter.h"
 #include "format/KeePass2RandomStream.h"
+#include "keeshare/KeeShare.h"
+#include "keeshare/KeeShareSettings.h"
 #include "streams/HmacBlockStream.h"
 #include "streams/SymmetricCipherStream.h"
 #include "streams/qtiocompressor.h"
@@ -156,7 +157,7 @@ bool Kdbx4Writer::writeDatabase(QIODevice* device, Database* db)
         writeInnerHeaderField(outputDevice, KeePass2::InnerHeaderFieldID::InnerRandomStreamKey, protectedStreamKey));
 
     // Write attachments to the inner header
-    writeAttachments(outputDevice, db);
+    auto idxMap = writeAttachments(outputDevice, db);
 
     CHECK_RETURN_FALSE(writeInnerHeaderField(outputDevice, KeePass2::InnerHeaderFieldID::End, QByteArray()));
 
@@ -166,7 +167,7 @@ bool Kdbx4Writer::writeDatabase(QIODevice* device, Database* db)
         return false;
     }
 
-    KdbxXmlWriter xmlWriter(db->formatVersion());
+    KdbxXmlWriter xmlWriter(db->formatVersion(), idxMap);
     xmlWriter.writeDatabase(outputDevice, db, &randomStream, headerHash);
 
     // Explicitly close/reset streams so they are flushed and we can detect
@@ -211,25 +212,40 @@ bool Kdbx4Writer::writeInnerHeaderField(QIODevice* device, KeePass2::InnerHeader
     return true;
 }
 
-void Kdbx4Writer::writeAttachments(QIODevice* device, Database* db)
+KdbxXmlWriter::BinaryIdxMap Kdbx4Writer::writeAttachments(QIODevice* device, Database* db)
 {
     const QList<Entry*> allEntries = db->rootGroup()->entriesRecursive(true);
-    QSet<QByteArray> writtenAttachments;
+    QHash<QByteArray, qint64> writtenAttachments;
+    KdbxXmlWriter::BinaryIdxMap idxMap;
+    qint64 nextIdx = 0;
 
-    for (Entry* entry : allEntries) {
+    for (const Entry* entry : allEntries) {
         const QList<QString> attachmentKeys = entry->attachments()->keys();
         for (const QString& key : attachmentKeys) {
             QByteArray data("\x01");
             data.append(entry->attachments()->value(key));
 
-            if (writtenAttachments.contains(data)) {
-                continue;
+            // Namespace KeeShare attachments so they don't get deduplicated together with attachments
+            // from other databases. Prevents potential filesize side channels.
+            CryptoHash hash(CryptoHash::Sha256);
+            if (auto shared = KeeShare::resolveSharedGroup(entry->group())) {
+                hash.addData(KeeShare::referenceOf(shared).uuid.toByteArray());
+            } else {
+                hash.addData(db->uuid().toByteArray());
             }
+            hash.addData(data);
 
-            writeInnerHeaderField(device, KeePass2::InnerHeaderFieldID::Binary, data);
-            writtenAttachments.insert(data);
+            // Deduplicate attachments with the same hash
+            const auto hashResult = hash.result();
+            if (!writtenAttachments.contains(hashResult)) {
+                writeInnerHeaderField(device, KeePass2::InnerHeaderFieldID::Binary, data);
+                writtenAttachments.insert(hashResult, nextIdx++);
+            }
+            idxMap.insert(qMakePair(entry, key), writtenAttachments[hashResult]);
         }
     }
+
+    return idxMap;
 }
 
 /**

src/format/Kdbx4Writer.h

@@ -19,6 +19,7 @@
 #define KEEPASSX_KDBX4WRITER_H
 
 #include "KdbxWriter.h"
+#include "format/KdbxXmlWriter.h"
 
 /**
  * KDBX4 writer implementation.
@@ -32,7 +33,7 @@ class Kdbx4Writer : public KdbxWriter
 
 private:
     bool writeInnerHeaderField(QIODevice* device, KeePass2::InnerHeaderFieldID fieldId, const QByteArray& data);
-    void writeAttachments(QIODevice* device, Database* db);
+    KdbxXmlWriter::BinaryIdxMap writeAttachments(QIODevice* device, Database* db);
     static bool serializeVariantMap(const QVariantMap& map, QByteArray& outputBytes);
 };
 

src/format/KdbxXmlWriter.cpp

@@ -19,16 +19,29 @@
 
 #include <QBuffer>
 #include <QFile>
+#include <QMap>
 
 #include "core/Endian.h"
+#include "crypto/CryptoHash.h"
 #include "format/KeePass2RandomStream.h"
+#include "keeshare/KeeShare.h"
+#include "keeshare/KeeShareSettings.h"
 #include "streams/qtiocompressor.h"
 
 /**
  * @param version KDBX version
  */
 KdbxXmlWriter::KdbxXmlWriter(quint32 version)
     : m_kdbxVersion(version)
+{
+    Q_ASSERT_X(m_kdbxVersion < KeePass2::FILE_VERSION_4,
+               "KDBX version",
+               "KDBX version >= 4 requires explicit binary index map.");
+}
+
+KdbxXmlWriter::KdbxXmlWriter(quint32 version, KdbxXmlWriter::BinaryIdxMap binaryIdxMap)
+    : m_kdbxVersion(version)
+    , m_binaryIdxMap(std::move(binaryIdxMap))
 {
 }
 
@@ -46,7 +59,9 @@ void KdbxXmlWriter::writeDatabase(QIODevice* device,
     m_xml.setAutoFormattingIndent(-1); // 1 tab
     m_xml.setCodec("UTF-8");
 
-    generateIdMap();
+    if (m_kdbxVersion < KeePass2::FILE_VERSION_4) {
+        fillBinaryIdxMap();
+    }
 
     m_xml.setDevice(device);
     m_xml.writeStartDocument("1.0", true);
@@ -80,18 +95,37 @@ QString KdbxXmlWriter::errorString()
     return m_errorStr;
 }
 
-void KdbxXmlWriter::generateIdMap()
+/**
+ * Generate a map of entry attachments to deduplicated attachment index IDs.
+ * This is basically duplicated code from Kdbx4Writer.cpp for KDBX 3 compatibility.
+ * I don't have a good solution for getting rid of this duplication without getting rid of KDBX 3.
+ */
+void KdbxXmlWriter::fillBinaryIdxMap()
 {
     const QList<Entry*> allEntries = m_db->rootGroup()->entriesRecursive(true);
-    int nextId = 0;
+    QHash<QByteArray, qint64> writtenAttachments;
+    qint64 nextIdx = 0;
 
     for (Entry* entry : allEntries) {
         const QList<QString> attachmentKeys = entry->attachments()->keys();
         for (const QString& key : attachmentKeys) {
             QByteArray data = entry->attachments()->value(key);
-            if (!m_idMap.contains(data)) {
-                m_idMap.insert(data, nextId++);
+
+            // Namespace KeeShare attachments so they don't get deduplicated together with attachments
+            // from other databases. Prevents potential filesize side channels.
+            CryptoHash hash(CryptoHash::Sha256);
+            if (auto shared = KeeShare::resolveSharedGroup(entry->group())) {
+                hash.addData(KeeShare::referenceOf(shared).uuid.toByteArray());
+            } else {
+                hash.addData(m_db->uuid().toByteArray());
+            }
+            hash.addData(data);
+
+            const auto hashResult = hash.result();
+            if (!writtenAttachments.contains(hashResult)) {
+                writtenAttachments.insert(hashResult, nextIdx++);
             }
+            m_binaryIdxMap.insert(qMakePair(entry, key), writtenAttachments.value(hashResult));
         }
     }
 }
@@ -181,13 +215,19 @@ void KdbxXmlWriter::writeIcon(const QUuid& uuid, const Metadata::CustomIconData&
 
 void KdbxXmlWriter::writeBinaries()
 {
+    // Reverse binary index map
+    QMap<qint64, QByteArray> binaries;
+    for (auto i = m_binaryIdxMap.constBegin(); i != m_binaryIdxMap.constEnd(); ++i) {
+        if (!binaries.contains(i.value())) {
+            binaries.insert(i.value(), i.key().first->attachments()->value(i.key().second));
+        }
+    }
+
     m_xml.writeStartElement("Binaries");
 
-    QHash<QByteArray, int>::const_iterator i;
-    for (i = m_idMap.constBegin(); i != m_idMap.constEnd(); ++i) {
+    for (auto i = binaries.constBegin(); i != binaries.constEnd(); ++i) {
         m_xml.writeStartElement("Binary");
-
-        m_xml.writeAttribute("ID", QString::number(i.value()));
+        m_xml.writeAttribute("ID", QString::number(i.key()));
 
         QByteArray data;
         if (m_db->compressionAlgorithm() == Database::CompressionGZip) {
@@ -200,15 +240,15 @@ void KdbxXmlWriter::writeBinaries()
             compressor.setStreamFormat(QtIOCompressor::GzipFormat);
             compressor.open(QIODevice::WriteOnly);
 
-            qint64 bytesWritten = compressor.write(i.key());
-            Q_ASSERT(bytesWritten == i.key().size());
+            qint64 bytesWritten = compressor.write(i.value());
+            Q_ASSERT(bytesWritten == i.value().size());
             Q_UNUSED(bytesWritten);
             compressor.close();
 
             buffer.seek(0);
             data = buffer.readAll();
         } else {
-            data = i.key();
+            data = i.value();
         }
 
         if (!data.isEmpty()) {
@@ -422,7 +462,7 @@ void KdbxXmlWriter::writeEntry(const Entry* entry)
         writeString("Key", key);
 
         m_xml.writeStartElement("Value");
-        m_xml.writeAttribute("Ref", QString::number(m_idMap[entry->attachments()->value(key)]));
+        m_xml.writeAttribute("Ref", QString::number(m_binaryIdxMap[qMakePair(entry, key)]));
         m_xml.writeEndElement();
 
         m_xml.writeEndElement();

src/format/KdbxXmlWriter.h

@@ -30,7 +30,13 @@ class KeePass2RandomStream;
 class KdbxXmlWriter
 {
 public:
+    /**
+     * Map of entry + attachment key to KDBX 4 inner header binary index.
+     */
+    typedef QHash<QPair<const Entry*, QString>, qint64> BinaryIdxMap;
+
     explicit KdbxXmlWriter(quint32 version);
+    explicit KdbxXmlWriter(quint32 version, KdbxXmlWriter::BinaryIdxMap binaryIdxMap);
 
     void writeDatabase(QIODevice* device,
                        const Database* db,
@@ -43,7 +49,7 @@ class KdbxXmlWriter
     QString errorString();
 
 private:
-    void generateIdMap();
+    void fillBinaryIdxMap();
 
     void writeMetadata();
     void writeMemoryProtection();
@@ -85,7 +91,7 @@ class KdbxXmlWriter
     QPointer<const Database> m_db;
     QPointer<const Metadata> m_meta;
     KeePass2RandomStream* m_randomStream = nullptr;
-    QHash<QByteArray, int> m_idMap;
+    BinaryIdxMap m_binaryIdxMap;
     QByteArray m_headerHash;
 
     bool m_error = false;

tests/TestKdbx3.cpp

@@ -169,3 +169,88 @@ void TestKdbx3::testBrokenHeaderHash()
     auto db = QSharedPointer<Database>::create();
     QVERIFY(!db->open(filename, key, nullptr));
 }
+
+void TestKdbx3::testAttachmentIndexStability()
+{
+    QScopedPointer<Database> db(new Database());
+    db->changeKdf(fastKdf(KeePass2::uuidToKdf(KeePass2::KDF_AES_KDBX3)));
+    auto compositeKey = QSharedPointer<CompositeKey>::create();
+    db->setKey(compositeKey);
+    QVERIFY(!db->uuid().isNull());
+
+    auto root = db->rootGroup();
+
+    auto group1 = new Group();
+    group1->setUuid(QUuid::createUuid());
+    QVERIFY(!group1->uuid().isNull());
+    group1->setParent(root);
+
+    auto attachment1 = QByteArray("qwerty");
+    auto attachment2 = QByteArray("asdf");
+    auto attachment3 = QByteArray("zxcv");
+
+    auto entry1 = new Entry();
+    entry1->setUuid(QUuid::createUuid());
+    QVERIFY(!entry1->uuid().isNull());
+    auto uuid1 = entry1->uuid();
+    entry1->attachments()->set("a", attachment1);
+    QCOMPARE(entry1->attachments()->keys().size(), 1);
+    QCOMPARE(entry1->attachments()->values().size(), 1);
+    entry1->setGroup(root);
+
+    auto entry2 = new Entry();
+    entry2->setUuid(QUuid::createUuid());
+    QVERIFY(!entry2->uuid().isNull());
+    auto uuid2 = entry2->uuid();
+    entry2->attachments()->set("a", attachment1);
+    entry2->attachments()->set("b", attachment2);
+    QCOMPARE(entry2->attachments()->keys().size(), 2);
+    QCOMPARE(entry2->attachments()->values().size(), 2);
+    entry2->setGroup(group1);
+
+    auto entry3 = new Entry();
+    entry3->setUuid(QUuid::createUuid());
+    QVERIFY(!entry3->uuid().isNull());
+    auto uuid3 = entry3->uuid();
+    entry3->attachments()->set("a", attachment1);
+    entry3->attachments()->set("b", attachment2);
+    entry3->attachments()->set("x", attachment3);
+    entry3->attachments()->set("y", attachment3);
+    QCOMPARE(entry3->attachments()->keys().size(), 4);
+    QCOMPARE(entry3->attachments()->values().size(), 3);
+    entry3->setGroup(group1);
+
+    QBuffer buffer;
+    buffer.open(QBuffer::ReadWrite);
+    KeePass2Writer writer;
+    QVERIFY(writer.writeDatabase(&buffer, db.data()));
+    QCOMPARE(writer.version(), KeePass2::FILE_VERSION_3_1);
+
+    buffer.seek(0);
+    KeePass2Reader reader;
+
+    // Re-read database and check that all attachments are still correctly assigned
+    auto db2 = QSharedPointer<Database>::create();
+    reader.readDatabase(&buffer, QSharedPointer<CompositeKey>::create(), db2.data());
+    QVERIFY(!reader.hasError());
+    QVERIFY(!db->uuid().isNull());
+
+    auto a1 = db2->rootGroup()->findEntryByUuid(uuid1)->attachments();
+    QCOMPARE(a1->keys().size(), 1);
+    QCOMPARE(a1->values().size(), 1);
+    QCOMPARE(a1->value("a"), attachment1);
+
+    auto a2 = db2->rootGroup()->findEntryByUuid(uuid2)->attachments();
+    QCOMPARE(a2->keys().size(), 2);
+    QCOMPARE(a2->values().size(), 2);
+    QCOMPARE(a2->value("a"), attachment1);
+    QCOMPARE(a2->value("b"), attachment2);
+
+    auto a3 = db2->rootGroup()->findEntryByUuid(uuid3)->attachments();
+    QCOMPARE(a3->keys().size(), 4);
+    QCOMPARE(a3->values().size(), 3);
+    QCOMPARE(a3->value("a"), attachment1);
+    QCOMPARE(a3->value("b"), attachment2);
+    QCOMPARE(a3->value("x"), attachment3);
+    QCOMPARE(a3->value("y"), attachment3);
+}

tests/TestKdbx3.h

@@ -30,6 +30,7 @@ private slots:
     void testProtectedStrings();
     void testBrokenHeaderHash();
     void testFormat300();
+    void testAttachmentIndexStability();
 
 protected:
     void initTestCaseImpl() override;