security improvements

[curious-rabbit]

Several non critical security fixes

• Browser URL match: subdomain confusion
  File: src/browser/BrowserService.cpp ~line 1550
  siteHost.endsWith(entryHost) is not anchored on a label boundary.
  "notbad.example.com" matches an entry saved for "bad.example.com" (both pass the eTLD+1 check, then endsWith returns true). Entry gets offered for the wrong site. Confirm dialog still gates first use, but the manager has already told the user "this matches".
  Fix: require exact match or "." + entryHost suffix.

• Passkey clientDataJSON injection
  File: src/browser/PasskeyUtils.cpp buildClientDataJson, ~line 356
  origin is interpolated raw into a hand-rolled JSON. Validation only checks the host extracted via QUrl, not the original string. Origins like https://example.com#",\"injected\":\"yes pass validation and inject extra JSON keys into the signed clientDataJSON. Not exploitable today (browser sets origin from window.location.origin) but defense-in-depth.
  Fix: JSON-escape origin and challenge before interpolation.

• KeeShare docs claim signature verification that doesn't happen
  Files: src/keeshare/ShareImport.cpp ~line 54, docs/topics/KeeShare.adoc
  Import skips the signature file. Per PR KeeShare: Remove checking signed container and QuaZip #7223 verification was intentionally removed but the docs still say signers are verified. Anyone with write access to the share path who knows the share password can substitute the database.
  Fix: docs.

• SymmetricCipher::defaultIvSize uses non-standard nonce sizes
  File: src/crypto/SymmetricCipher.cpp ~line 222
  defaultIvSize(GCM)=16 and defaultIvSize(Salsa20)=12 are not the standard sizes. Polkit and Windows Hello use defaultIvSize and end up with 16-byte GCM nonces. Stale "AES-256-CBC" comment in WindowsHello.cpp:126 above GCM code.
  Fix: defaultIvSize returns standard sizes (GCM 12, ChaCha20 12, Salsa20 8); ivSize() left alone since it is used by ssh-agent; fix comment.

• KDBX4 outer header field length is unbounded
  File: src/format/Kdbx4Reader.cpp readHeaderField, ~line 167
  fieldLen is quint32, passed straight to device.read(fieldLen), which allocates the requested size regardless of file length. Outer header is parsed BEFORE any HMAC check or password prompt. A tiny malicious .kdbx with a huge fieldLen causes a multi-gigabyte allocation when the user opens the file.
  Fix: cap outer header fields to 1 MiB and inner header fields to 1 GiB. Same cap on HmacBlockStream/HashedBlockStream block sizes (64 MiB).

• TempFile save mode places encrypted DB in /tmp and deletes original first
  File: src/core/Database.cpp performSave, ~line 393
  QTemporaryFile() defaults to /tmp. The original is QFile::remove'd before the rename. If rename fails after the delete and BackupBeforeSave is off, the user has no DB.
  Fix: place the tempfile in the target directory; sideline the original to a unique name, rename the new file in, then delete the sideline; restore the original on failure.

Type of change

• ✅ Bug fix (non-breaking change that fixes an issue)

This is mostly a bugreport
Consider the patch to be a suggested solution. I am not familiar enough with the codebase of keepass

[phoerious]

I assume the findings and fixes were AI-assisted?

[curious-rabbit]

I assume the findings and fixes were AI-assisted?

Partly yes. Most were the result of static and dynamic code analysis tools which were automated using llms to speed up the process.

[curious-rabbit]

Anything left to do for this one?

[varjolintu]

Anything left to do for this one?

For example https://github.com/keepassxreboot/keepassxc/pull/13331/changes#r3242046872. The latest change introduced this m_socketBuffers but I have no idea what benefit it brings to the existing implementation you had. It looks like a complex code for nothing.

[curious-rabbit]

Anything left to do for this one?

For example https://github.com/keepassxreboot/keepassxc/pull/13331/changes#r3242046872. The latest change introduced this m_socketBuffers but I have no idea what benefit it brings to the existing implementation you had. It looks like a complex code for nothing.

All cleaned up now. Thanks for the review

[Copilot]

Copilot review overview

🟡 Changes recommended

Unescaped passkey JSON, incomplete KDBX bounds, save and IV compatibility concerns, and missing BrowserHost framing remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 3 · 2

Open (5)

• BrowserHost still lacks socket message framing · New
• JSON fields remain vulnerable to injection via unescaped values · New
• Inner Binary field allows excessive memory allocation · New
• Save cleanup can delete an existing .old file · New
• GCM IV default change breaks existing quick-unlock data · New

What changed in this PR

This pull request proposes security and robustness improvements across browser integration, KDBX parsing, cryptography, save handling, and KeeShare documentation.

Changes:

• Adds hostname-boundary matching tests and logic.
• Adds KDBX and stream size limits.
• Updates save staging, IV handling, hashing, and documentation.
• Reformats passkey JSON construction; BrowserHost message framing is not included.

File	Reviewed change
tests/​TestBrowser.h	Declares hostname regression tests.
tests/​TestBrowser.cpp	Tests exact, subdomain, and sibling-host matching.
src/​streams/​HmacBlockStream.cpp	Limits HMAC block sizes.
src/​streams/​HashedBlockStream.h	Defines the block-size limit.
src/​streams/​HashedBlockStream.cpp	Limits hashed block sizes.
src/​quickunlock/​WindowsHello.cpp	Corrects the cipher comment.
src/​format/​Kdbx4Reader.cpp	Limits header allocations; inner bounds and boundary coverage remain incomplete.
src/​crypto/​SymmetricCipher.cpp	Adjusts default IV sizes; persisted-data compatibility needs handling.
src/​crypto/​CryptoHash.cpp	Allows empty hash updates.
src/​core/​Database.cpp	Uses target-directory temporary saves and sideline handling.
src/​browser/​PasskeyUtils.cpp	Reformats client-data construction, but values remain unescaped.
src/​browser/​BrowserService.cpp	Enforces label-boundary hostname matching.
docs/​topics/​KeeShare.adoc	Documents that signatures are not verified.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[droidmonkey]

I removed the patches on the browser frame size since that was fixed elsewhere

[curious-rabbit]

Addressed the remaining review comments