diff --git a/docs/topics/KeeShare.adoc b/docs/topics/KeeShare.adoc index 882e7f017c..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 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 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/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 e6fc24a1fe..80b6942d28 100644 --- a/src/browser/BrowserService.cpp +++ b/src/browser/BrowserService.cpp @@ -1548,8 +1548,8 @@ bool BrowserService::handleURL(const QString& entryUrl, return false; } - // Match the subdomains with the limited wildcard - if (siteQUrl.host().endsWith(entryQUrl.host())) { + // 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 a53e007e6d..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,8 +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 { + // 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(), origin); + .arg((get ? QString("webauthn.get") : QString("webauthn.create")), + 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 87a03d30f2..ace4af14d8 100644 --- a/src/core/Database.cpp +++ b/src/core/Database.cpp @@ -395,7 +395,9 @@ bool Database::performSave(const QString& filePath, SaveAction action, const QSt break; } case TempFile: { - QTemporaryFile tempFile; + // 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()) { HashingStream hashingStream(&tempFile, QCryptographicHash::Md5, kFileBlockToHashSizeBytes); if (!hashingStream.open(QIODevice::WriteOnly)) { @@ -407,9 +409,17 @@ bool Database::performSave(const QString& filePath, SaveAction action, const QSt } tempFile.close(); // flush to disk - // Delete the original db and move the temp file in place + // Move the original aside instead of deleting it so it can be restored on failure auto perms = QFile::permissions(filePath); - QFile::remove(filePath); + // 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) { + *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 @@ -417,15 +427,27 @@ bool Database::performSave(const QString& filePath, SaveAction action, const QSt if (tempFile.QFile::rename(filePath)) { // successfully saved the database tempFile.setAutoRemove(false); - QFile::setPermissions(filePath, perms); + 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 + } + + // 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) { *error = tr("%1\nBackup database located at %2").arg(tempFile.errorString(), tempFile.fileName()); diff --git a/src/crypto/SymmetricCipher.cpp b/src/crypto/SymmetricCipher.cpp index 61ba8fb29f..2a92245135 100644 --- a/src/crypto/SymmetricCipher.cpp +++ b/src/crypto/SymmetricCipher.cpp @@ -222,17 +222,19 @@ QString SymmetricCipher::modeToString(const Mode mode) int SymmetricCipher::defaultIvSize(Mode mode) { + // 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 Aes256_GCM: case Twofish_CBC: return 16; - case Salsa20: + case Aes256_GCM: case ChaCha20: return 12; + case Salsa20: + return 8; default: return -1; } diff --git a/src/format/Kdbx4Reader.cpp b/src/format/Kdbx4Reader.cpp index 0698a0b2bd..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,7 +169,7 @@ bool Kdbx4Reader::readHeaderField(StoreDataStream& device, Database* db) bool ok; auto fieldLen = Endian::readSizedInt(&device, KeePass2::BYTEORDER, &ok); - if (!ok) { + if (!ok || fieldLen > kMaxOuterHeaderFieldSize) { raiseError(tr("Invalid header field length: field %1").arg(fieldID)); return false; } @@ -260,7 +264,7 @@ bool Kdbx4Reader::readInnerHeaderField(QIODevice* device) bool ok; auto fieldLen = Endian::readSizedInt(device, KeePass2::BYTEORDER, &ok); - if (!ok) { + if (!ok || fieldLen > kMaxInnerHeaderFieldSize) { raiseError(tr("Invalid inner header field length: field %1").arg(static_cast(fieldID))); return false; } 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..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) { + 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..2a4b4bb527 100644 --- a/src/streams/HashedBlockStream.h +++ b/src/streams/HashedBlockStream.h @@ -34,6 +34,9 @@ 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; protected: diff --git a/src/streams/HmacBlockStream.cpp b/src/streams/HmacBlockStream.cpp index bb9eb477e5..bf356573be 100644 --- a/src/streams/HmacBlockStream.cpp +++ b/src/streams/HmacBlockStream.cpp @@ -19,6 +19,7 @@ #include "core/Endian.h" #include "crypto/CryptoHash.h" +#include "streams/HashedBlockStream.h" const QSysInfo::Endian HmacBlockStream::ByteOrder = QSysInfo::LittleEndian; @@ -140,7 +141,7 @@ bool HmacBlockStream::readHashedBlock() return false; } auto blockSize = Endian::bytesToSizedInt(blockSizeBytes, ByteOrder); - if (blockSize < 0) { + if (blockSize < 0 || blockSize > HashedBlockStream::MaxBlockSize) { m_error = true; setErrorString("Invalid block size."); return false; diff --git a/tests/TestBrowser.cpp b/tests/TestBrowser.cpp index 15afa2b809..a7035475cc 100644 --- a/tests/TestBrowser.cpp +++ b/tests/TestBrowser.cpp @@ -757,6 +757,30 @@ void TestBrowser::testSubdomainsAndPaths() QCOMPARE(result.length(), 1); } +void TestBrowser::testSubdomainAnchor() +{ + const QString entryUrl = "https://bad.example.com"; + + auto db = QSharedPointer::create(); + auto* root = db->rootGroup(); + auto* entry = new Entry(); + entry->setGroup(root); + entry->setUrl(entryUrl); + entry->setUsername("u"); + entry->setPassword("p"); + + // 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 { QList entries; diff --git a/tests/TestBrowser.h b/tests/TestBrowser.h index 9b1b80cc88..223d57769f 100644 --- a/tests/TestBrowser.h +++ b/tests/TestBrowser.h @@ -48,6 +48,7 @@ private slots: void testSearchEntriesWithWildcardURLs(); void testInvalidEntries(); void testSubdomainsAndPaths(); + void testSubdomainAnchor(); void testBestMatchingCredentials(); void testBestMatchingWithAdditionalURLs(); void testRestrictBrowserKey();