From 3e97f452bb74f66e30ea077768c693c4da063d8a Mon Sep 17 00:00:00 2001 From: curious-rabbit Date: Sun, 10 May 2026 00:11:40 +0200 Subject: [PATCH 1/7] security improvements --- docs/topics/KeeShare.adoc | 2 +- src/browser/BrowserService.cpp | 5 +++-- src/browser/PasskeyUtils.cpp | 30 +++++++++++++++++++++++++- src/core/Database.cpp | 36 +++++++++++++++++++------------ src/crypto/CryptoHash.cpp | 4 ---- src/crypto/SymmetricCipher.cpp | 15 +------------ src/format/Kdbx4Reader.cpp | 8 +++++++ src/quickunlock/WindowsHello.cpp | 2 +- src/streams/HashedBlockStream.cpp | 2 +- src/streams/HmacBlockStream.cpp | 2 +- 10 files changed, 67 insertions(+), 39 deletions(-) diff --git a/docs/topics/KeeShare.adoc b/docs/topics/KeeShare.adoc index 882e7f017c..d919537a64 100644 --- a/docs/topics/KeeShare.adoc +++ b/docs/topics/KeeShare.adoc @@ -47,5 +47,5 @@ image::keeshare_shared_group.png[] === Technical Details and Limitations of Sharing Sharing relies on the combination of file exports and imports as well as the synchronization mechanism provided by KeePassXC. Since the merge algorithm uses the history of entries to prevent data loss, this history must be enabled and have a sufficient size. Furthermore, the merge algorithm is location independent, therefore it does not matter if entries are moved outside of an import group. These entries will be updated nonetheless. Moving entries outside of export groups will prevent a further export of the entry, but it will not ensure that the already shared data will be removed from any client. -KeeShare uses a custom certification mechanism to ensure that the source of the data is the expected one. This ensures that the data was exported by the signer but it is not possible to detect if someone replaced the data with an older version from a valid signer. To prevent this, the container could be placed at a location which is only writeable for valid signers. +KeeShare does not verify signatures on imported share containers. A signature is still produced when exporting a `.kdbx.share` container, but it is not checked at import time. The trust model relies entirely on filesystem permissions of the share container's storage location and the share password. Any party with write access to the share container path who knows the share password can substitute the database, and entries with newer modification timestamps will overwrite local entries during the merge. The container must be placed at a location which is only writable by parties whose data you trust. // end::content[] diff --git a/src/browser/BrowserService.cpp b/src/browser/BrowserService.cpp index e6fc24a1fe..32694287f7 100644 --- a/src/browser/BrowserService.cpp +++ b/src/browser/BrowserService.cpp @@ -1548,8 +1548,9 @@ bool BrowserService::handleURL(const QString& entryUrl, return false; } - // Match the subdomains with the limited wildcard - if (siteQUrl.host().endsWith(entryQUrl.host())) { + // Anchor on label boundary + if (siteQUrl.host() == entryQUrl.host() + || siteQUrl.host().endsWith(QStringLiteral(".") + entryQUrl.host())) { return true; } diff --git a/src/browser/PasskeyUtils.cpp b/src/browser/PasskeyUtils.cpp index a53e007e6d..786db96fc9 100644 --- a/src/browser/PasskeyUtils.cpp +++ b/src/browser/PasskeyUtils.cpp @@ -356,8 +356,36 @@ ExtensionResult PasskeyUtils::buildExtensionData(QJsonObject& extensionObject) c // Serialization order: https://w3c.github.io/webauthn/#clientdatajson-serialization QString PasskeyUtils::buildClientDataJson(const QJsonObject& publicKey, const QString& origin, bool get) const { + // JSON-escape user-supplied strings; spec mandates field order so QJsonDocument is unsuitable + auto jsonEscape = [](const QString& in) { + QString out; + out.reserve(in.size()); + for (QChar ch : in) { + ushort u = ch.unicode(); + switch (u) { + case '\\': out.append(QStringLiteral("\\\\")); break; + case '"': out.append(QStringLiteral("\\\"")); break; + case '\b': out.append(QStringLiteral("\\b")); break; + case '\f': out.append(QStringLiteral("\\f")); break; + case '\n': out.append(QStringLiteral("\\n")); break; + case '\r': out.append(QStringLiteral("\\r")); break; + case '\t': out.append(QStringLiteral("\\t")); break; + default: + if (u < 0x20) { + out.append(QString::asprintf("\\u%04x", u)); + } else { + out.append(ch); + } + break; + } + } + return out; + }; + return QString("{\"type\":\"%1\",\"challenge\":\"%2\",\"origin\":\"%3\",\"crossOrigin\":false}") - .arg((get ? QString("webauthn.get") : QString("webauthn.create")), publicKey["challenge"].toString(), origin); + .arg((get ? QString("webauthn.get") : QString("webauthn.create")), + jsonEscape(publicKey["challenge"].toString()), + jsonEscape(origin)); } QStringList PasskeyUtils::getAllowedCredentialsFromAssertionOptions(const QJsonObject& assertionOptions) const diff --git a/src/core/Database.cpp b/src/core/Database.cpp index 87a03d30f2..d7466cd94e 100644 --- a/src/core/Database.cpp +++ b/src/core/Database.cpp @@ -395,37 +395,45 @@ bool Database::performSave(const QString& filePath, SaveAction action, const QSt break; } case TempFile: { - QTemporaryFile tempFile; + // Place tempfile next to target so rename is atomic and no encrypted copy lands in /tmp + QFileInfo targetInfo(filePath); + QTemporaryFile tempFile(targetInfo.absolutePath() + "/." + targetInfo.fileName() + ".XXXXXX.tmp"); if (tempFile.open()) { HashingStream hashingStream(&tempFile, QCryptographicHash::Md5, kFileBlockToHashSizeBytes); if (!hashingStream.open(QIODevice::WriteOnly)) { return false; } - // write the database to the file if (!writeDatabase(&hashingStream, error)) { return false; } - tempFile.close(); // flush to disk + tempFile.close(); - // Delete the original db and move the temp file in place auto perms = QFile::permissions(filePath); - QFile::remove(filePath); + const QString sidelinedPath = filePath + ".old"; + QFile::remove(sidelinedPath); + const bool hadOriginal = QFile::exists(filePath); + if (hadOriginal && !QFile::rename(filePath, sidelinedPath)) { + if (error) { + *error = tr("Could not rename original database file"); + } + return false; + } - // Note: call into the QFile rename instead of QTemporaryFile - // due to an undocumented difference in how the function handles - // errors. This prevents errors when saving across file systems. if (tempFile.QFile::rename(filePath)) { - // successfully saved the database tempFile.setAutoRemove(false); + if (hadOriginal) { + QFile::remove(sidelinedPath); + } QFile::setPermissions(filePath, perms); - // Retain original creation time tempFile.setFileTime(createTime, QFile::FileBirthTime); - // store the new hash m_fileBlockHash = hashingStream.hashingResult(); return true; - } else if (backupFilePath.isEmpty() || !restoreDatabase(filePath, backupFilePath)) { - // Failed to copy new database in place, and - // failed to restore from backup or backups disabled + } + + if (hadOriginal) { + QFile::rename(sidelinedPath, filePath); + } + if (backupFilePath.isEmpty() || !restoreDatabase(filePath, backupFilePath)) { tempFile.setAutoRemove(false); if (error) { *error = tr("%1\nBackup database located at %2").arg(tempFile.errorString(), tempFile.fileName()); diff --git a/src/crypto/CryptoHash.cpp b/src/crypto/CryptoHash.cpp index a1dc6595ea..a01aedc903 100644 --- a/src/crypto/CryptoHash.cpp +++ b/src/crypto/CryptoHash.cpp @@ -66,10 +66,6 @@ void CryptoHash::addData(const QByteArray& data) { Q_D(CryptoHash); - if (data.isEmpty()) { - return; - } - try { if (d->hmacFunction) { d->hmacFunction->update(reinterpret_cast(data.data()), data.size()); diff --git a/src/crypto/SymmetricCipher.cpp b/src/crypto/SymmetricCipher.cpp index 61ba8fb29f..475d62535f 100644 --- a/src/crypto/SymmetricCipher.cpp +++ b/src/crypto/SymmetricCipher.cpp @@ -222,20 +222,7 @@ QString SymmetricCipher::modeToString(const Mode mode) int SymmetricCipher::defaultIvSize(Mode mode) { - switch (mode) { - case Aes128_CBC: - case Aes256_CBC: - case Aes128_CTR: - case Aes256_CTR: - case Aes256_GCM: - case Twofish_CBC: - return 16; - case Salsa20: - case ChaCha20: - return 12; - default: - return -1; - } + return ivSize(mode); } int SymmetricCipher::keySize(Mode mode) diff --git a/src/format/Kdbx4Reader.cpp b/src/format/Kdbx4Reader.cpp index 0698a0b2bd..856a3e996b 100644 --- a/src/format/Kdbx4Reader.cpp +++ b/src/format/Kdbx4Reader.cpp @@ -169,6 +169,10 @@ bool Kdbx4Reader::readHeaderField(StoreDataStream& device, Database* db) raiseError(tr("Invalid header field length: field %1").arg(fieldID)); return false; } + if (fieldLen > 1024 * 1024) { + raiseError(tr("Invalid header field length: field %1").arg(fieldID)); + return false; + } QByteArray fieldData; if (fieldLen != 0) { @@ -264,6 +268,10 @@ bool Kdbx4Reader::readInnerHeaderField(QIODevice* device) raiseError(tr("Invalid inner header field length: field %1").arg(static_cast(fieldID))); return false; } + if (fieldLen > 1024u * 1024u * 1024u) { + raiseError(tr("Invalid inner header field length: field %1").arg(static_cast(fieldID))); + return false; + } QByteArray fieldData; if (fieldLen != 0) { diff --git a/src/quickunlock/WindowsHello.cpp b/src/quickunlock/WindowsHello.cpp index 890e3499a5..cb106a2568 100644 --- a/src/quickunlock/WindowsHello.cpp +++ b/src/quickunlock/WindowsHello.cpp @@ -123,7 +123,7 @@ bool WindowsHello::setKey(const QUuid& dbUuid, const QByteArray& data) return false; } - // Encrypt the data using AES-256-CBC + // Encrypt the data using AES-256-GCM SymmetricCipher cipher; if (!cipher.init(SymmetricCipher::Aes256_GCM, SymmetricCipher::Encrypt, key, challenge)) { m_error = QObject::tr("Failed to init KeePassXC crypto."); diff --git a/src/streams/HashedBlockStream.cpp b/src/streams/HashedBlockStream.cpp index d04b42dfbb..19c6a6aba4 100644 --- a/src/streams/HashedBlockStream.cpp +++ b/src/streams/HashedBlockStream.cpp @@ -141,7 +141,7 @@ bool HashedBlockStream::readHashedBlock() } m_blockSize = Endian::readSizedInt(m_baseDevice, ByteOrder, &ok); - if (!ok || m_blockSize < 0) { + if (!ok || m_blockSize < 0 || m_blockSize > 64 * 1024 * 1024) { m_error = true; setErrorString("Invalid block size."); return false; diff --git a/src/streams/HmacBlockStream.cpp b/src/streams/HmacBlockStream.cpp index bb9eb477e5..69566e4b35 100644 --- a/src/streams/HmacBlockStream.cpp +++ b/src/streams/HmacBlockStream.cpp @@ -140,7 +140,7 @@ bool HmacBlockStream::readHashedBlock() return false; } auto blockSize = Endian::bytesToSizedInt(blockSizeBytes, ByteOrder); - if (blockSize < 0) { + if (blockSize < 0 || blockSize > 64 * 1024 * 1024) { m_error = true; setErrorString("Invalid block size."); return false; From 6e42d8811a25b4c263d1fb3b6c983612653dfd77 Mon Sep 17 00:00:00 2001 From: curious-rabbit Date: Wed, 13 May 2026 23:32:48 +0200 Subject: [PATCH 2/7] improve patch --- src/browser/PasskeyUtils.cpp | 2 +- src/crypto/SymmetricCipher.cpp | 18 ++++++++++++++- tests/TestBrowser.cpp | 40 ++++++++++++++++++++++++++++++++++ tests/TestBrowser.h | 2 ++ 4 files changed, 60 insertions(+), 2 deletions(-) diff --git a/src/browser/PasskeyUtils.cpp b/src/browser/PasskeyUtils.cpp index 786db96fc9..d85668efeb 100644 --- a/src/browser/PasskeyUtils.cpp +++ b/src/browser/PasskeyUtils.cpp @@ -356,7 +356,7 @@ ExtensionResult PasskeyUtils::buildExtensionData(QJsonObject& extensionObject) c // Serialization order: https://w3c.github.io/webauthn/#clientdatajson-serialization QString PasskeyUtils::buildClientDataJson(const QJsonObject& publicKey, const QString& origin, bool get) const { - // JSON-escape user-supplied strings; spec mandates field order so QJsonDocument is unsuitable + // Hand-rolled JSON (spec mandates field order); escape inputs as defense-in-depth since result is signed auto jsonEscape = [](const QString& in) { QString out; out.reserve(in.size()); diff --git a/src/crypto/SymmetricCipher.cpp b/src/crypto/SymmetricCipher.cpp index 475d62535f..c949a95fda 100644 --- a/src/crypto/SymmetricCipher.cpp +++ b/src/crypto/SymmetricCipher.cpp @@ -222,7 +222,23 @@ QString SymmetricCipher::modeToString(const Mode mode) int SymmetricCipher::defaultIvSize(Mode mode) { - return ivSize(mode); + // Standard nonce sizes for new random IVs. Decryption uses the IV stored on disk. + switch (mode) { + case Aes128_CBC: + case Aes256_CBC: + case Aes128_CTR: + case Aes256_CTR: + case Twofish_CBC: + return 16; // block size + case Aes256_GCM: + return 12; // NIST SP 800-38D recommends 96-bit nonces + case ChaCha20: + return 12; // RFC 8439 + case Salsa20: + return 12; // spec is 8 bytes, kept at 12 for compatibility + default: + return -1; + } } int SymmetricCipher::keySize(Mode mode) diff --git a/tests/TestBrowser.cpp b/tests/TestBrowser.cpp index 15afa2b809..6ba7b32c65 100644 --- a/tests/TestBrowser.cpp +++ b/tests/TestBrowser.cpp @@ -757,6 +757,46 @@ void TestBrowser::testSubdomainsAndPaths() QCOMPARE(result.length(), 1); } +void TestBrowser::testSubdomainAnchor_data() +{ + QTest::addColumn("entryUrl"); + QTest::addColumn("siteUrl"); + QTest::addColumn("shouldMatch"); + + QTest::newRow("exact") + << "https://bad.example.com" << "https://bad.example.com" << true; + QTest::newRow("true subdomain") + << "https://bad.example.com" << "https://login.bad.example.com" << true; + QTest::newRow("sibling host (regression for endsWith bug)") + << "https://bad.example.com" << "https://notbad.example.com" << false; + QTest::newRow("sibling on same eTLD+1") + << "https://bad.example.com" << "https://good.example.com" << false; + QTest::newRow("unrelated") + << "https://bad.example.com" << "https://attacker.test" << false; +} + +void TestBrowser::testSubdomainAnchor() +{ + QFETCH(QString, entryUrl); + QFETCH(QString, siteUrl); + QFETCH(bool, shouldMatch); + + auto db = QSharedPointer::create(); + auto* root = db->rootGroup(); + auto* entry = new Entry(); + entry->setGroup(root); + entry->setUrl(entryUrl); + entry->setUsername("u"); + entry->setPassword("p"); + + auto entries = m_browserService->searchEntries(db, siteUrl, ""); + if (shouldMatch) { + QCOMPARE(entries.length(), 1); + } else { + QCOMPARE(entries.length(), 0); + } +} + QList TestBrowser::createEntries(QStringList& urls, Group* root, bool additionalUrl) const { QList entries; diff --git a/tests/TestBrowser.h b/tests/TestBrowser.h index 9b1b80cc88..a7b0b9f895 100644 --- a/tests/TestBrowser.h +++ b/tests/TestBrowser.h @@ -48,6 +48,8 @@ private slots: void testSearchEntriesWithWildcardURLs(); void testInvalidEntries(); void testSubdomainsAndPaths(); + void testSubdomainAnchor_data(); + void testSubdomainAnchor(); void testBestMatchingCredentials(); void testBestMatchingWithAdditionalURLs(); void testRestrictBrowserKey(); From a1216bbaa4ff13eadfe60d5055508bd82c496e98 Mon Sep 17 00:00:00 2001 From: Jonathan White Date: Tue, 22 Sep 2026 15:52:46 -0400 Subject: [PATCH 3/7] improve patch --- src/browser/PasskeyUtils.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/browser/PasskeyUtils.cpp b/src/browser/PasskeyUtils.cpp index d85668efeb..53c529407c 100644 --- a/src/browser/PasskeyUtils.cpp +++ b/src/browser/PasskeyUtils.cpp @@ -356,7 +356,9 @@ ExtensionResult PasskeyUtils::buildExtensionData(QJsonObject& extensionObject) c // Serialization order: https://w3c.github.io/webauthn/#clientdatajson-serialization QString PasskeyUtils::buildClientDataJson(const QJsonObject& publicKey, const QString& origin, bool get) const { - // Hand-rolled JSON (spec mandates field order); escape inputs as defense-in-depth since result is signed + // Hand-rolled JSON since the spec mandates field order. origin is escaped as + // defense-in-depth because the result is signed; the cleaner fix would be to + // pass the already-validated effectiveDomain in here instead of the raw origin. auto jsonEscape = [](const QString& in) { QString out; out.reserve(in.size()); @@ -384,7 +386,7 @@ QString PasskeyUtils::buildClientDataJson(const QJsonObject& publicKey, const QS return QString("{\"type\":\"%1\",\"challenge\":\"%2\",\"origin\":\"%3\",\"crossOrigin\":false}") .arg((get ? QString("webauthn.get") : QString("webauthn.create")), - jsonEscape(publicKey["challenge"].toString()), + publicKey["challenge"].toString(), // base64url, nothing to escape jsonEscape(origin)); } From 69b98b094173601c91fb19be05eff9b3ea126e22 Mon Sep 17 00:00:00 2001 From: curious-rabbit Date: Fri, 15 May 2026 12:53:04 +0200 Subject: [PATCH 4/7] improve patch --- tests/TestBrowser.cpp | 38 +++++++++++--------------------------- tests/TestBrowser.h | 1 - 2 files changed, 11 insertions(+), 28 deletions(-) diff --git a/tests/TestBrowser.cpp b/tests/TestBrowser.cpp index 6ba7b32c65..a7035475cc 100644 --- a/tests/TestBrowser.cpp +++ b/tests/TestBrowser.cpp @@ -757,29 +757,9 @@ void TestBrowser::testSubdomainsAndPaths() QCOMPARE(result.length(), 1); } -void TestBrowser::testSubdomainAnchor_data() -{ - QTest::addColumn("entryUrl"); - QTest::addColumn("siteUrl"); - QTest::addColumn("shouldMatch"); - - QTest::newRow("exact") - << "https://bad.example.com" << "https://bad.example.com" << true; - QTest::newRow("true subdomain") - << "https://bad.example.com" << "https://login.bad.example.com" << true; - QTest::newRow("sibling host (regression for endsWith bug)") - << "https://bad.example.com" << "https://notbad.example.com" << false; - QTest::newRow("sibling on same eTLD+1") - << "https://bad.example.com" << "https://good.example.com" << false; - QTest::newRow("unrelated") - << "https://bad.example.com" << "https://attacker.test" << false; -} - void TestBrowser::testSubdomainAnchor() { - QFETCH(QString, entryUrl); - QFETCH(QString, siteUrl); - QFETCH(bool, shouldMatch); + const QString entryUrl = "https://bad.example.com"; auto db = QSharedPointer::create(); auto* root = db->rootGroup(); @@ -789,12 +769,16 @@ void TestBrowser::testSubdomainAnchor() entry->setUsername("u"); entry->setPassword("p"); - auto entries = m_browserService->searchEntries(db, siteUrl, ""); - if (shouldMatch) { - QCOMPARE(entries.length(), 1); - } else { - QCOMPARE(entries.length(), 0); - } + // exact + QCOMPARE(m_browserService->searchEntries(db, "https://bad.example.com", "").length(), 1); + // true subdomain + QCOMPARE(m_browserService->searchEntries(db, "https://login.bad.example.com", "").length(), 1); + // sibling host (regression for endsWith bug) + QCOMPARE(m_browserService->searchEntries(db, "https://notbad.example.com", "").length(), 0); + // sibling on same eTLD+1 + QCOMPARE(m_browserService->searchEntries(db, "https://good.example.com", "").length(), 0); + // unrelated + QCOMPARE(m_browserService->searchEntries(db, "https://attacker.test", "").length(), 0); } QList TestBrowser::createEntries(QStringList& urls, Group* root, bool additionalUrl) const diff --git a/tests/TestBrowser.h b/tests/TestBrowser.h index a7b0b9f895..223d57769f 100644 --- a/tests/TestBrowser.h +++ b/tests/TestBrowser.h @@ -48,7 +48,6 @@ private slots: void testSearchEntriesWithWildcardURLs(); void testInvalidEntries(); void testSubdomainsAndPaths(); - void testSubdomainAnchor_data(); void testSubdomainAnchor(); void testBestMatchingCredentials(); void testBestMatchingWithAdditionalURLs(); From 3073ee286f4cf975eed696c53dd65e57eabd6ddd Mon Sep 17 00:00:00 2001 From: curious-rabbit Date: Mon, 25 May 2026 14:22:27 +0200 Subject: [PATCH 5/7] update patch --- src/streams/HashedBlockStream.cpp | 2 +- src/streams/HashedBlockStream.h | 2 ++ src/streams/HmacBlockStream.cpp | 3 ++- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/streams/HashedBlockStream.cpp b/src/streams/HashedBlockStream.cpp index 19c6a6aba4..248d17910e 100644 --- a/src/streams/HashedBlockStream.cpp +++ b/src/streams/HashedBlockStream.cpp @@ -141,7 +141,7 @@ bool HashedBlockStream::readHashedBlock() } m_blockSize = Endian::readSizedInt(m_baseDevice, ByteOrder, &ok); - if (!ok || m_blockSize < 0 || m_blockSize > 64 * 1024 * 1024) { + if (!ok || m_blockSize < 0 || m_blockSize > MaxBlockSize) { m_error = true; setErrorString("Invalid block size."); return false; diff --git a/src/streams/HashedBlockStream.h b/src/streams/HashedBlockStream.h index dc1c16a1a6..1f4240e6d4 100644 --- a/src/streams/HashedBlockStream.h +++ b/src/streams/HashedBlockStream.h @@ -34,6 +34,8 @@ class HashedBlockStream : public LayeredStream bool reset() override; void close() override; + static constexpr qint32 MaxBlockSize = 64 * 1024 * 1024; + bool atEnd() const override; protected: diff --git a/src/streams/HmacBlockStream.cpp b/src/streams/HmacBlockStream.cpp index 69566e4b35..7c5357f2d7 100644 --- a/src/streams/HmacBlockStream.cpp +++ b/src/streams/HmacBlockStream.cpp @@ -17,6 +17,7 @@ #include "HmacBlockStream.h" +#include "streams/HashedBlockStream.h" #include "core/Endian.h" #include "crypto/CryptoHash.h" @@ -140,7 +141,7 @@ bool HmacBlockStream::readHashedBlock() return false; } auto blockSize = Endian::bytesToSizedInt(blockSizeBytes, ByteOrder); - if (blockSize < 0 || blockSize > 64 * 1024 * 1024) { + if (blockSize < 0 || blockSize > HashedBlockStream::MaxBlockSize) { m_error = true; setErrorString("Invalid block size."); return false; From 1b01b48dabf7bd8ecfa29c01657653fcdf56d1d2 Mon Sep 17 00:00:00 2001 From: Jonathan White Date: Tue, 22 Sep 2026 22:22:26 -0400 Subject: [PATCH 6/7] code format --- share/translations/keepassxc_en.ts | 4 ++++ src/browser/BrowserService.cpp | 3 +-- src/browser/PasskeyUtils.cpp | 28 +++++++++++++++++++++------- src/streams/HmacBlockStream.cpp | 2 +- 4 files changed, 27 insertions(+), 10 deletions(-) diff --git a/share/translations/keepassxc_en.ts b/share/translations/keepassxc_en.ts index 4ca387959d..95c9ccfd24 100644 --- a/share/translations/keepassxc_en.ts +++ b/share/translations/keepassxc_en.ts @@ -1671,6 +1671,10 @@ Backup database located at %2 No file path was provided. + + Could not rename original database file + + DatabaseOpenDialog diff --git a/src/browser/BrowserService.cpp b/src/browser/BrowserService.cpp index 32694287f7..6c3a9bcd53 100644 --- a/src/browser/BrowserService.cpp +++ b/src/browser/BrowserService.cpp @@ -1549,8 +1549,7 @@ bool BrowserService::handleURL(const QString& entryUrl, } // Anchor on label boundary - if (siteQUrl.host() == entryQUrl.host() - || siteQUrl.host().endsWith(QStringLiteral(".") + entryQUrl.host())) { + if (siteQUrl.host() == entryQUrl.host() || siteQUrl.host().endsWith(QStringLiteral(".") + entryQUrl.host())) { return true; } diff --git a/src/browser/PasskeyUtils.cpp b/src/browser/PasskeyUtils.cpp index 53c529407c..644db597e9 100644 --- a/src/browser/PasskeyUtils.cpp +++ b/src/browser/PasskeyUtils.cpp @@ -365,13 +365,27 @@ QString PasskeyUtils::buildClientDataJson(const QJsonObject& publicKey, const QS for (QChar ch : in) { ushort u = ch.unicode(); switch (u) { - case '\\': out.append(QStringLiteral("\\\\")); break; - case '"': out.append(QStringLiteral("\\\"")); break; - case '\b': out.append(QStringLiteral("\\b")); break; - case '\f': out.append(QStringLiteral("\\f")); break; - case '\n': out.append(QStringLiteral("\\n")); break; - case '\r': out.append(QStringLiteral("\\r")); break; - case '\t': out.append(QStringLiteral("\\t")); break; + case '\\': + out.append(QStringLiteral("\\\\")); + break; + case '"': + out.append(QStringLiteral("\\\"")); + break; + case '\b': + out.append(QStringLiteral("\\b")); + break; + case '\f': + out.append(QStringLiteral("\\f")); + break; + case '\n': + out.append(QStringLiteral("\\n")); + break; + case '\r': + out.append(QStringLiteral("\\r")); + break; + case '\t': + out.append(QStringLiteral("\\t")); + break; default: if (u < 0x20) { out.append(QString::asprintf("\\u%04x", u)); diff --git a/src/streams/HmacBlockStream.cpp b/src/streams/HmacBlockStream.cpp index 7c5357f2d7..bf356573be 100644 --- a/src/streams/HmacBlockStream.cpp +++ b/src/streams/HmacBlockStream.cpp @@ -17,9 +17,9 @@ #include "HmacBlockStream.h" -#include "streams/HashedBlockStream.h" #include "core/Endian.h" #include "crypto/CryptoHash.h" +#include "streams/HashedBlockStream.h" const QSysInfo::Endian HmacBlockStream::ByteOrder = QSysInfo::LittleEndian; From 41f542692f2091e3a2375e4d00ee965c6026186c Mon Sep 17 00:00:00 2001 From: curious-rabbit Date: Fri, 25 Sep 2026 13:31:00 +0200 Subject: [PATCH 7/7] improve patch --- docs/topics/KeeShare.adoc | 2 +- src/browser/BrowserService.cpp | 2 +- src/browser/PasskeyUtils.cpp | 60 +++++++++------------------------ src/core/Database.cpp | 28 +++++++++++---- src/crypto/CryptoHash.cpp | 4 +++ src/crypto/SymmetricCipher.cpp | 9 +++-- src/format/Kdbx4Reader.cpp | 16 ++++----- src/streams/HashedBlockStream.h | 1 + 8 files changed, 54 insertions(+), 68 deletions(-) diff --git a/docs/topics/KeeShare.adoc b/docs/topics/KeeShare.adoc index d919537a64..ded9a8ddc2 100644 --- a/docs/topics/KeeShare.adoc +++ b/docs/topics/KeeShare.adoc @@ -47,5 +47,5 @@ image::keeshare_shared_group.png[] === Technical Details and Limitations of Sharing Sharing relies on the combination of file exports and imports as well as the synchronization mechanism provided by KeePassXC. Since the merge algorithm uses the history of entries to prevent data loss, this history must be enabled and have a sufficient size. Furthermore, the merge algorithm is location independent, therefore it does not matter if entries are moved outside of an import group. These entries will be updated nonetheless. Moving entries outside of export groups will prevent a further export of the entry, but it will not ensure that the already shared data will be removed from any client. -KeeShare does not verify signatures on imported share containers. A signature is still produced when exporting a `.kdbx.share` container, but it is not checked at import time. The trust model relies entirely on filesystem permissions of the share container's storage location and the share password. Any party with write access to the share container path who knows the share password can substitute the database, and entries with newer modification timestamps will overwrite local entries during the merge. The container must be placed at a location which is only writable by parties whose data you trust. +KeeShare does not verify signatures when importing a share container. A signature is written when a `.kdbx.share` container is exported, but it is not checked at import time. Trust relies on the filesystem permissions of the container's location and on the share password. Anyone who can write to the container path and knows the share password can replace the database, and entries with newer modification timestamps will overwrite local entries during the merge. Place the container at a location which is only writable by parties whose data you trust. // end::content[] diff --git a/src/browser/BrowserService.cpp b/src/browser/BrowserService.cpp index 6c3a9bcd53..80b6942d28 100644 --- a/src/browser/BrowserService.cpp +++ b/src/browser/BrowserService.cpp @@ -1548,7 +1548,7 @@ bool BrowserService::handleURL(const QString& entryUrl, return false; } - // Anchor on label boundary + // Match on a label boundary so siblings like notbad.example.com do not match bad.example.com if (siteQUrl.host() == entryQUrl.host() || siteQUrl.host().endsWith(QStringLiteral(".") + entryQUrl.host())) { return true; } diff --git a/src/browser/PasskeyUtils.cpp b/src/browser/PasskeyUtils.cpp index 644db597e9..f8a464e72a 100644 --- a/src/browser/PasskeyUtils.cpp +++ b/src/browser/PasskeyUtils.cpp @@ -22,9 +22,22 @@ #include "core/Tools.h" #include "gui/UrlTools.h" +#include +#include #include #include +namespace +{ + // Escape a string for safe embedding in the hand-built clientDataJSON + QString jsonEscapeString(const QString& value) + { + // Reuse Qt's JSON encoder, then strip the array wrapper [""] + const auto encoded = QJsonDocument(QJsonArray{value}).toJson(QJsonDocument::Compact); + return QString::fromUtf8(encoded.mid(2, encoded.size() - 4)); + } +} // namespace + Q_GLOBAL_STATIC(PasskeyUtils, s_passkeyUtils); PasskeyUtils* PasskeyUtils::instance() @@ -356,52 +369,11 @@ ExtensionResult PasskeyUtils::buildExtensionData(QJsonObject& extensionObject) c // Serialization order: https://w3c.github.io/webauthn/#clientdatajson-serialization QString PasskeyUtils::buildClientDataJson(const QJsonObject& publicKey, const QString& origin, bool get) const { - // Hand-rolled JSON since the spec mandates field order. origin is escaped as - // defense-in-depth because the result is signed; the cleaner fix would be to - // pass the already-validated effectiveDomain in here instead of the raw origin. - auto jsonEscape = [](const QString& in) { - QString out; - out.reserve(in.size()); - for (QChar ch : in) { - ushort u = ch.unicode(); - switch (u) { - case '\\': - out.append(QStringLiteral("\\\\")); - break; - case '"': - out.append(QStringLiteral("\\\"")); - break; - case '\b': - out.append(QStringLiteral("\\b")); - break; - case '\f': - out.append(QStringLiteral("\\f")); - break; - case '\n': - out.append(QStringLiteral("\\n")); - break; - case '\r': - out.append(QStringLiteral("\\r")); - break; - case '\t': - out.append(QStringLiteral("\\t")); - break; - default: - if (u < 0x20) { - out.append(QString::asprintf("\\u%04x", u)); - } else { - out.append(ch); - } - break; - } - } - return out; - }; - + // Field order is mandated by the spec, so the JSON is built by hand with escaped inputs return QString("{\"type\":\"%1\",\"challenge\":\"%2\",\"origin\":\"%3\",\"crossOrigin\":false}") .arg((get ? QString("webauthn.get") : QString("webauthn.create")), - publicKey["challenge"].toString(), // base64url, nothing to escape - jsonEscape(origin)); + jsonEscapeString(publicKey["challenge"].toString()), + jsonEscapeString(origin)); } QStringList PasskeyUtils::getAllowedCredentialsFromAssertionOptions(const QJsonObject& assertionOptions) const diff --git a/src/core/Database.cpp b/src/core/Database.cpp index d7466cd94e..ace4af14d8 100644 --- a/src/core/Database.cpp +++ b/src/core/Database.cpp @@ -395,7 +395,7 @@ bool Database::performSave(const QString& filePath, SaveAction action, const QSt break; } case TempFile: { - // Place tempfile next to target so rename is atomic and no encrypted copy lands in /tmp + // Keep the temp file next to the target so the final rename stays on one file system QFileInfo targetInfo(filePath); QTemporaryFile tempFile(targetInfo.absolutePath() + "/." + targetInfo.fileName() + ".XXXXXX.tmp"); if (tempFile.open()) { @@ -403,14 +403,16 @@ bool Database::performSave(const QString& filePath, SaveAction action, const QSt if (!hashingStream.open(QIODevice::WriteOnly)) { return false; } + // write the database to the file if (!writeDatabase(&hashingStream, error)) { return false; } - tempFile.close(); + tempFile.close(); // flush to disk + // Move the original aside instead of deleting it so it can be restored on failure auto perms = QFile::permissions(filePath); - const QString sidelinedPath = filePath + ".old"; - QFile::remove(sidelinedPath); + // Derive from the unique temp name so an existing user file is never overwritten + const QString sidelinedPath = tempFile.fileName() + ".orig"; const bool hadOriginal = QFile::exists(filePath); if (hadOriginal && !QFile::rename(filePath, sidelinedPath)) { if (error) { @@ -419,20 +421,32 @@ bool Database::performSave(const QString& filePath, SaveAction action, const QSt return false; } + // Note: call into the QFile rename instead of QTemporaryFile + // due to an undocumented difference in how the function handles + // errors. This prevents errors when saving across file systems. if (tempFile.QFile::rename(filePath)) { + // successfully saved the database tempFile.setAutoRemove(false); if (hadOriginal) { QFile::remove(sidelinedPath); + QFile::setPermissions(filePath, perms); } - QFile::setPermissions(filePath, perms); + // Retain original creation time tempFile.setFileTime(createTime, QFile::FileBirthTime); + // store the new hash m_fileBlockHash = hashingStream.hashingResult(); return true; } - if (hadOriginal) { - QFile::rename(sidelinedPath, filePath); + // Saving failed, put the original database back in place + if (hadOriginal && QFile::rename(sidelinedPath, filePath)) { + if (error) { + *error = tempFile.errorString(); + } + return false; } + + // No original to restore, or restoring it failed, fall back to the backup if (backupFilePath.isEmpty() || !restoreDatabase(filePath, backupFilePath)) { tempFile.setAutoRemove(false); if (error) { diff --git a/src/crypto/CryptoHash.cpp b/src/crypto/CryptoHash.cpp index a01aedc903..a1dc6595ea 100644 --- a/src/crypto/CryptoHash.cpp +++ b/src/crypto/CryptoHash.cpp @@ -66,6 +66,10 @@ void CryptoHash::addData(const QByteArray& data) { Q_D(CryptoHash); + if (data.isEmpty()) { + return; + } + try { if (d->hmacFunction) { d->hmacFunction->update(reinterpret_cast(data.data()), data.size()); diff --git a/src/crypto/SymmetricCipher.cpp b/src/crypto/SymmetricCipher.cpp index c949a95fda..2a92245135 100644 --- a/src/crypto/SymmetricCipher.cpp +++ b/src/crypto/SymmetricCipher.cpp @@ -222,20 +222,19 @@ QString SymmetricCipher::modeToString(const Mode mode) int SymmetricCipher::defaultIvSize(Mode mode) { - // Standard nonce sizes for new random IVs. Decryption uses the IV stored on disk. + // Standard nonce sizes used when generating a new random IV switch (mode) { case Aes128_CBC: case Aes256_CBC: case Aes128_CTR: case Aes256_CTR: case Twofish_CBC: - return 16; // block size + return 16; case Aes256_GCM: - return 12; // NIST SP 800-38D recommends 96-bit nonces case ChaCha20: - return 12; // RFC 8439 + return 12; case Salsa20: - return 12; // spec is 8 bytes, kept at 12 for compatibility + return 8; default: return -1; } diff --git a/src/format/Kdbx4Reader.cpp b/src/format/Kdbx4Reader.cpp index 856a3e996b..a79fbe898c 100644 --- a/src/format/Kdbx4Reader.cpp +++ b/src/format/Kdbx4Reader.cpp @@ -31,6 +31,10 @@ #include "streams/SymmetricCipherStream.h" #include "streams/qtiocompressor.h" +// Upper bounds on header field lengths, used to reject malformed files +static constexpr quint32 kMaxOuterHeaderFieldSize = 1024 * 1024; +static constexpr quint32 kMaxInnerHeaderFieldSize = 1024u * 1024u * 1024u; + bool Kdbx4Reader::readDatabaseImpl(QIODevice* device, const QByteArray& headerData, QSharedPointer key, @@ -165,11 +169,7 @@ bool Kdbx4Reader::readHeaderField(StoreDataStream& device, Database* db) bool ok; auto fieldLen = Endian::readSizedInt(&device, KeePass2::BYTEORDER, &ok); - if (!ok) { - raiseError(tr("Invalid header field length: field %1").arg(fieldID)); - return false; - } - if (fieldLen > 1024 * 1024) { + if (!ok || fieldLen > kMaxOuterHeaderFieldSize) { raiseError(tr("Invalid header field length: field %1").arg(fieldID)); return false; } @@ -264,11 +264,7 @@ bool Kdbx4Reader::readInnerHeaderField(QIODevice* device) bool ok; auto fieldLen = Endian::readSizedInt(device, KeePass2::BYTEORDER, &ok); - if (!ok) { - raiseError(tr("Invalid inner header field length: field %1").arg(static_cast(fieldID))); - return false; - } - if (fieldLen > 1024u * 1024u * 1024u) { + if (!ok || fieldLen > kMaxInnerHeaderFieldSize) { raiseError(tr("Invalid inner header field length: field %1").arg(static_cast(fieldID))); return false; } diff --git a/src/streams/HashedBlockStream.h b/src/streams/HashedBlockStream.h index 1f4240e6d4..2a4b4bb527 100644 --- a/src/streams/HashedBlockStream.h +++ b/src/streams/HashedBlockStream.h @@ -34,6 +34,7 @@ class HashedBlockStream : public LayeredStream bool reset() override; void close() override; + // Upper bound on a single block, used to reject malformed input static constexpr qint32 MaxBlockSize = 64 * 1024 * 1024; bool atEnd() const override;