security improvements#13331
Conversation
|
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. |
phoerious
added
the
pr: ai-assisted
|
Anything left to do for this one? |
For example https://github.com/keepassxreboot/keepassxc/pull/13331/changes#r3242046872. The latest change introduced this |
All cleaned up now. Thanks for the review |
4 participants
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 disagrees with ivSize
File: src/crypto/SymmetricCipher.cpp ~line 222
defaultIvSize(GCM)=16 but ivSize(GCM)=12; same for ChaCha20/Salsa20 (12 vs 8). Polkit and Windows Hello use defaultIvSize and end up with 16-byte GCM nonces. Botan accepts it; not exploitable. Stale "AES-256-CBC" comment in WindowsHello.cpp:126 above GCM code.
Fix: make defaultIvSize call ivSize; fix comment.
BrowserHost local socket has no message framing
File: src/browser/BrowserHost.cpp readProxyMessage, ~line 70
readyRead may deliver partial JSON or two coalesced messages. fromJson returns null, message dropped silently, browser extension hangs.
Fix: per-socket buffer, parse one document at a time using JSON parser as the framing oracle.
KDBX4 outer header field length is unbounded
File: src/format/Kdbx4Reader.cpp readHeaderField, ~line 167
fieldLen is quint32, passed straight to device.read(fieldLen). Verified that QFile::read in Qt5 allocates the requested size regardless of file length. Outer header is parsed BEFORE any HMAC check or password prompt. A 49-byte malicious .kdbx with fieldLen=0xFFFFFFFE causes ~4 GiB allocation when the user opens the file. KDBX3 not affected (uses quint16).
Fix: cap fieldLen to ~1 MiB. Same cap on inner header and on HmacBlockStream/HashedBlockStream block sizes.
TempFile save mode places encrypted DB in /tmp and deletes original first
File: src/core/Database.cpp performSave, ~line 393
QTemporaryFile() defaults to /tmp. Original is QFile::remove'd before the rename. If rename fails after delete and BackupBeforeSave is off, the user has no DB. Not the default mode (atomic is) but a real footgun for users who turned atomic off.
Fix: place tempfile in the target directory; sideline the original to .old, rename, then delete sideline.
CryptoHash::addData skips empty input
File: src/crypto/CryptoHash.cpp ~line 65
Early-returns if data is empty. Botan handles zero-length update fine. No current caller depends on this, but it's a foot-gun for any future streaming hash construction with optionally-empty chunks.
Fix: drop the early return.
Type of change
This is mostly a bugreport
Consider the patch to be a suggested solution. I am not familiar enough with the codebase of keepass