Prevent byte-by-byte and attachment inference side channel attacks

Attack - KeeShare attachments can be inferred because of attachment de-duplication.

Solution - Prevent de-duplication of normal database entry attachments with those entry attachments synchronized/associated with a KeeShare database. This is done using the KeeShare database UUID injected into the hash calculation of the attachment prior to de-dupe. The attachments themselves are not modified in any way.

--------

Attack - Side channel byte-by-byte inference due to compression de-duplication of data between a KeeShare database and it's parent.

Solution - Generate a random array between 64 and 512 bytes, convert to hex, and store in the database custom data.

--------

Attack vector assumptions:
1. Compression is enabled
2. The attacker has access to a KeeShare database actively syncing with the victim's database
3. The victim's database is unlocked and syncing
4. The attacker can see the exact size of the victim's database after saving, and syncing, the KeeShare database

Thank you to Andrés Fábrega from Cornell University for theorizing and informing us of this attack vector.

src/core/Database.cpp

@@ -21,6 +21,7 @@
 #include "core/AsyncTask.h"
 #include "core/FileWatcher.h"
 #include "core/Group.h"
+#include "crypto/Random.h"
 #include "format/KdbxXmlReader.h"
 #include "format/KeePass2Reader.h"
 #include "format/KeePass2Writer.h"
@@ -269,6 +270,10 @@ bool Database::saveAs(const QString& filePath, SaveAction action, const QString&
     // Clear read-only flag
     m_fileWatcher->stop();
 
+    // 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());
+
     // Prevent destructive operations while saving
     QMutexLocker locker(&m_saveMutex);
 

src/format/Kdbx4Writer.cpp

@@ -19,10 +19,14 @@
 
 #include <QBuffer>
 
+#include "config-keepassx.h"
 #include "crypto/CryptoHash.h"
 #include "crypto/Random.h"
-#include "format/KdbxXmlWriter.h"
 #include "format/KeePass2RandomStream.h"
+#ifdef WITH_XC_KEESHARE
+#include "keeshare/KeeShare.h"
+#include "keeshare/KeeShareSettings.h"
+#endif
 #include "streams/HmacBlockStream.h"
 #include "streams/SymmetricCipherStream.h"
 #include "streams/qtiocompressor.h"
@@ -156,7 +160,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 +170,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 +215,42 @@ 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;
+            CryptoHash hash(CryptoHash::Sha256);
+#ifdef WITH_XC_KEESHARE
+            // Namespace KeeShare attachments so they don't get deduplicated together with attachments
+            // from other databases. Prevents potential filesize side channels.
+            if (auto shared = KeeShare::resolveSharedGroup(entry->group())) {
+                hash.addData(KeeShare::referenceOf(shared).uuid.toByteArray());
+            } else {
+                hash.addData(db->uuid().toByteArray());
             }
-
-            writeInnerHeaderField(device, KeePass2::InnerHeaderFieldID::Binary, data);
-            writtenAttachments.insert(data);
+#endif
+            hash.addData(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,38 @@ 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++);
+            CryptoHash hash(CryptoHash::Sha256);
+#ifdef WITH_XC_KEESHARE
+            // Namespace KeeShare attachments so they don't get deduplicated together with attachments
+            // from other databases. Prevents potential filesize side channels.
+            if (auto shared = KeeShare::resolveSharedGroup(entry->group())) {
+                hash.addData(KeeShare::referenceOf(shared).uuid.toByteArray());
+            } else {
+                hash.addData(m_db->uuid().toByteArray());
+            }
+#endif
+            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 +216,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 +241,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 +463,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;