diff --git a/src/core/Database.cpp b/src/core/Database.cpp index 562159aad3..c9364fb5b7 100644 --- a/src/core/Database.cpp +++ b/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); diff --git a/src/format/Kdbx4Writer.cpp b/src/format/Kdbx4Writer.cpp index 08a0df013e..d8f412b055 100644 --- a/src/format/Kdbx4Writer.cpp +++ b/src/format/Kdbx4Writer.cpp @@ -19,10 +19,14 @@ #include +#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 allEntries = db->rootGroup()->entriesRecursive(true); - QSet writtenAttachments; + QHash writtenAttachments; + KdbxXmlWriter::BinaryIdxMap idxMap; + qint64 nextIdx = 0; - for (Entry* entry : allEntries) { + for (const Entry* entry : allEntries) { const QList 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; } /** diff --git a/src/format/Kdbx4Writer.h b/src/format/Kdbx4Writer.h index c8540245b1..7b6af2f5d1 100644 --- a/src/format/Kdbx4Writer.h +++ b/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); }; diff --git a/src/format/KdbxXmlWriter.cpp b/src/format/KdbxXmlWriter.cpp index 35ed5ffdb3..53a7bfae90 100644 --- a/src/format/KdbxXmlWriter.cpp +++ b/src/format/KdbxXmlWriter.cpp @@ -19,9 +19,13 @@ #include #include +#include #include "core/Endian.h" +#include "crypto/CryptoHash.h" #include "format/KeePass2RandomStream.h" +#include "keeshare/KeeShare.h" +#include "keeshare/KeeShareSettings.h" #include "streams/qtiocompressor.h" /** @@ -29,6 +33,15 @@ */ 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 allEntries = m_db->rootGroup()->entriesRecursive(true); - int nextId = 0; + QHash writtenAttachments; + qint64 nextIdx = 0; for (Entry* entry : allEntries) { const QList 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 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::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(); diff --git a/src/format/KdbxXmlWriter.h b/src/format/KdbxXmlWriter.h index 181e007baf..d36151942d 100644 --- a/src/format/KdbxXmlWriter.h +++ b/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, 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 m_db; QPointer m_meta; KeePass2RandomStream* m_randomStream = nullptr; - QHash m_idMap; + BinaryIdxMap m_binaryIdxMap; QByteArray m_headerHash; bool m_error = false; diff --git a/tests/TestKdbx3.cpp b/tests/TestKdbx3.cpp index bcc3df77a0..097fef6f98 100644 --- a/tests/TestKdbx3.cpp +++ b/tests/TestKdbx3.cpp @@ -169,3 +169,88 @@ void TestKdbx3::testBrokenHeaderHash() auto db = QSharedPointer::create(); QVERIFY(!db->open(filename, key, nullptr)); } + +void TestKdbx3::testAttachmentIndexStability() +{ + QScopedPointer db(new Database()); + db->changeKdf(fastKdf(KeePass2::uuidToKdf(KeePass2::KDF_AES_KDBX3))); + auto compositeKey = QSharedPointer::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::create(); + reader.readDatabase(&buffer, QSharedPointer::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); +} diff --git a/tests/TestKdbx3.h b/tests/TestKdbx3.h index deb965d903..2994e355db 100644 --- a/tests/TestKdbx3.h +++ b/tests/TestKdbx3.h @@ -30,6 +30,7 @@ private slots: void testProtectedStrings(); void testBrokenHeaderHash(); void testFormat300(); + void testAttachmentIndexStability(); protected: void initTestCaseImpl() override; diff --git a/tests/TestKdbx4.cpp b/tests/TestKdbx4.cpp index ff5b63b6d5..cf103c3198 100644 --- a/tests/TestKdbx4.cpp +++ b/tests/TestKdbx4.cpp @@ -24,6 +24,10 @@ #include "format/KeePass2.h" #include "format/KeePass2Reader.h" #include "format/KeePass2Writer.h" +#ifdef WITH_XC_KEESHARE +#include "keeshare/KeeShare.h" +#include "keeshare/KeeShareSettings.h" +#endif #include "keys/FileKey.h" #include "keys/PasswordKey.h" #include "mock/MockChallengeResponseKey.h" @@ -72,7 +76,7 @@ QSharedPointer TestKdbx4Argon2::readXml(QBuffer* buf, bool strictMode, void TestKdbx4Argon2::writeXml(QBuffer* buf, Database* db, bool& hasError, QString& errorString) { - KdbxXmlWriter writer(KeePass2::FILE_VERSION_4); + KdbxXmlWriter writer(KeePass2::FILE_VERSION_4, {}); writer.writeDatabase(buf, db); hasError = writer.hasError(); errorString = writer.errorString(); @@ -417,6 +421,107 @@ void TestKdbx4Format::testUpgradeMasterKeyIntegrity_data() QTest::newRow("Upgrade (implicit): entry-customdata") << QString("entry-customdata") << KeePass2::FILE_VERSION_4; } +void TestKdbx4Format::testAttachmentIndexStability() +{ + QScopedPointer db(new Database()); + db->changeKdf(fastKdf(KeePass2::uuidToKdf(KeePass2::KDF_ARGON2ID))); + auto compositeKey = QSharedPointer::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); + + // Simulate KeeShare group, which uses its own attachment namespace + auto group2 = new Group(); + group2->setUuid(QUuid::createUuid()); + QVERIFY(!group2->uuid().isNull()); + group2->setParent(group1); +#ifdef WITH_XC_KEESHARE + KeeShareSettings::Reference ref; + ref.type = KeeShareSettings::SynchronizeWith; + ref.path = "123"; + KeeShare::setReferenceTo(group2, ref); + QVERIFY(KeeShare::isShared(group2)); +#endif + + 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(group2); + + QBuffer buffer; + buffer.open(QBuffer::ReadWrite); + KeePass2Writer writer; + QVERIFY(writer.writeDatabase(&buffer, db.data())); + QVERIFY(writer.version() >= KeePass2::FILE_VERSION_4); + + buffer.seek(0); + KeePass2Reader reader; + + // Re-read database and check that all attachments are still correctly assigned + auto db2 = QSharedPointer::create(); + reader.readDatabase(&buffer, QSharedPointer::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); + +#ifdef WITH_XC_KEESHARE + QVERIFY(KeeShare::isShared(db2->rootGroup()->findEntryByUuid(uuid3)->group())); +#endif + 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); +} + void TestKdbx4Format::testCustomData() { Database db; diff --git a/tests/TestKdbx4.h b/tests/TestKdbx4.h index 0955fcb603..cd5bc4788c 100644 --- a/tests/TestKdbx4.h +++ b/tests/TestKdbx4.h @@ -64,6 +64,7 @@ private slots: void testFormat410Upgrade(); void testUpgradeMasterKeyIntegrity(); void testUpgradeMasterKeyIntegrity_data(); + void testAttachmentIndexStability(); void testCustomData(); };