security improvements

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[]

share/translations/keepassxc_en.ts

@@ -1671,6 +1671,10 @@ Backup database located at %2</source>
         <source>No file path was provided.</source>
         <translation type="unfinished"></translation>
     </message>
+    <message>
+        <source>Could not rename original database file</source>
+        <translation type="unfinished"></translation>
+    </message>
 </context>
 <context>
     <name>DatabaseOpenDialog</name>

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;
     }
 

src/browser/PasskeyUtils.cpp

@@ -22,9 +22,22 @@
 #include "core/Tools.h"
 #include "gui/UrlTools.h"
 
+#include <QJsonArray>
+#include <QJsonDocument>
 #include <QList>
 #include <QUrl>
 
+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 ["<escaped>"]
+        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

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,25 +409,45 @@ 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
             // errors. This prevents errors when saving across file systems.
             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());

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;
     }

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<const CompositeKey> key,
@@ -165,7 +169,7 @@ bool Kdbx4Reader::readHeaderField(StoreDataStream& device, Database* db)
 
     bool ok;
     auto fieldLen = Endian::readSizedInt<quint32>(&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<quint32>(device, KeePass2::BYTEORDER, &ok);
-    if (!ok) {
+    if (!ok || fieldLen > kMaxInnerHeaderFieldSize) {
         raiseError(tr("Invalid inner header field length: field %1").arg(static_cast<int>(fieldID)));
         return false;
     }

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.");

src/streams/HashedBlockStream.cpp

@@ -141,7 +141,7 @@ bool HashedBlockStream::readHashedBlock()
     }
 
     m_blockSize = Endian::readSizedInt<qint32>(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;

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:

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<qint32>(blockSizeBytes, ByteOrder);
-    if (blockSize < 0) {
+    if (blockSize < 0 || blockSize > HashedBlockStream::MaxBlockSize) {
         m_error = true;
         setErrorString("Invalid block size.");
         return false;

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<Database>::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<Entry*> TestBrowser::createEntries(QStringList& urls, Group* root, bool additionalUrl) const
 {
     QList<Entry*> entries;

tests/TestBrowser.h

@@ -48,6 +48,7 @@ private slots:
     void testSearchEntriesWithWildcardURLs();
     void testInvalidEntries();
     void testSubdomainsAndPaths();
+    void testSubdomainAnchor();
     void testBestMatchingCredentials();
     void testBestMatchingWithAdditionalURLs();
     void testRestrictBrowserKey();