Repository navigation
Security: constant-time comparisons, bounded decompression and streaming export with resource limits - #11
Merged
Conversation
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…wallowing them The iostream layer silently converts exceptions thrown from streambuf underflow into short reads, so a stream exceeding its resource budget used to end as a truncated/empty payload (or, in the KDBX4 HMAC path, a spurious IoError). Record the violation on the streambuf (BudgetExceeded()) and have the importers raise a FormatError once the read finishes. Adds robustness tests for every budget: gzip bomb, HMAC block size/count, hashed framing, XML depth, group/entry/history counts and binary/string field sizes.
Fixes the failing Lint job: several files were flagged by clang-format (column overflows, include ordering, member-init alignment).
Resolve all 9 warnings-as-errors that the CI lint workflow flags on this branch while leaving the pinned clang-format 22 output byte-identical: - make KdbxXml::ParseProtectedString const (readability-make-member-function-const) - uppercase integer literal suffixes off-test/lqueue u->U and ull->ULL - return convert-string by value instead of by const-ref (bugprone-return-const-ref-from-parameter) - build prefix keys via insert() instead of literal concatenation (performance-inefficient-string-concatenation) - make the argonn2 memory product's widening explicit (bugprone-implicit-widening-of-multiplication-result) - construct the test ToBytes() payload into a name vector instead of restating the return type (modernize-return-braced-init-list) Verified locally: the same nine clang-tidy 22 diagnostics no longer reproduce, all 18 ctest binaries pass, and clang-format is drift-free.
dkruempe
changed the title
- Drop unused Str overload and StrVal helper in property.cc - Reword framing comment flagged as commented-out code - Document the RunAudit function in output.cc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR contains two bundled security-forward workstreams tracked in
ROADMAP.md:P0 — constant-time comparisons everywhere secrets are compared. All mock
secret and integrity-critical comparisons now funnel through
keepass::detail::constant_time_eq(backed byCRYPTO_memcmp,src/include/libkeepass/detail/constant_time.hh), covering the KDBX4 storedheader HMAC, the KDBX3/4 header hash + content start byte + per-block
verification (
src/kdbx_import.cc,src/stream.cc), the KDB contenthash/password check (
src/kdb.cc),secure_stringequality(
src/secure.cc) and the derived-key zero check (src/key.cc). Unit-testedin
test/constant_time.cc.P1 — bound decompressed payload size (zip-bomb protection). gzip/hashed/
HMAC block streams enforce configurable caps on total decompressed bytes,
block count and per-block size, checked before any allocation or MAC
verification; violations surface as
FormatError. Hostile fixtures intest/robustness.cccover gzip bombs, HMAC block size/count and hashedframing.
All of the above was previously split verbatim from the monolith codec commit;
this PR sources the split output and the streaming export with explicit
resource limits (
src/export.cc,cli/export.cc).Related issue
Security coverage tracked in
ROADMAP.md:test/robustness.cc)Type of change
Checklist
ctest --test-dir build --output-on-failureclang-format --dry-run --Werror $(git ls-files '*.cc' '*.hh')src/include/libkeepass/CHANGELOG.md(Keep a Changelog format)Security information
This branch (feature/p0-constant-time-comparisons) hardens every point where a
secret is compared for equality, and bounds decompression/streaming resources
so hostile payloads cannot be forced to allocate (or keep alive) arbitrary
amounts of memory before any MAC/hash check runs:
constant_time_eq(src/include/libkeepass/detail/constant_time.hh)wraps
CRYPTO_memcmpand is used for the KDBX4 header HMAC, the KDBX3/4header hash + per-block integrity, the KDB content-hash/password check, all
secure_stringequality comparisons, and the derived-key zero check. Theseare the only places secret material is compared; no plaintext
==ormemcmpremains on any secret path (verified bytest/constant_time.ccandrepo-wide grep).
max_total_bytes,max_block_countand per-block size caps before anyallocation or MAC verification; exceeding them raises
FormatError(notswallowed), so export/import cannot be turned into a zip-bomb or an
unbounded-memory parser.
ctest18/18 passes; hostile fixtures (test/robustness.cc,test/property.cc) and constant-time tests (test/constant_time.cc) all pass;clang-format and clang-tidy 22.1.8 are clean on this HEAD.