fix(crypto): bound key derivation by wall clock (fuzz-found DoS) - #4
Merged
Conversation
The nightly `unlock` fuzz target does not crash; it stops:
==4250== ERROR: libFuzzer: timeout after 1780 seconds
MS: 2 ChangeBit-CMP- DE: "sha1"-
`derive_key` takes the iteration count straight from the LUKS keyslot, so it
is attacker-controlled, and floors it without ever capping it:
let iters = iterations.max(1);
A crafted header can therefore ask for up to 4_294_967_295 PBKDF2 iterations
and an examiner opening that image waits forever. Denial of service in a tool
whose whole job is reading hostile input.
Two tests, because the fix has two ways to be wrong:
* `absurd_iteration_count_is_refused_rather_than_run` - u32::MAX must come
back as an error. It runs the derivation on a worker thread behind a
60-second watchdog, so today it fails in a minute instead of hanging the
suite; a red that never terminates is not a useful red.
* `a_realistic_iteration_count_still_derives` - 200_000 iterations, which is
squarely inside what cryptsetup writes, must go through untouched and give
a stable key. A bound that rejects real containers would be a worse bug
than the one being fixed.
The second test already passes. The first fails with "the derivation is
unbounded".
…budget
`derive_key` now measures this machine's PBKDF2 rate on a short calibration
run, projects the cost of the header's full iteration count, and refuses
before starting anything that would exceed a 30-second budget. u32::MAX
iterations returns `DerivationBudgetExceeded` in milliseconds instead of
running for hours.
The bound is on projected wall clock rather than on the iteration count
because "too many rounds" is a property of the machine, not a number that can
be fixed in advance -- a count that is absurd on a laptop can be routine on a
workstation. PBKDF2 is exactly linear in its iteration count, so scaling a
20_000-iteration measurement is a sound estimate.
Deliberately NOT a deadline checked inside the iteration loop, which is the
obvious shape: reaching inside the loop means hand-rolling PBKDF2 around a raw
HMAC, and this module's first line of documentation is that every primitive
comes from an audited RustCrypto crate. A worker thread with a timeout was the
other candidate and was rejected too -- it returns promptly but leaves a thread
spinning on the malicious input for as long as the process lives.
Refusal, never a silent clamp. Deriving with fewer rounds than the header
specifies yields a different key, so clamping would report a wrong passphrase
for a container that would have opened -- fabricating a failed unlock, which
is worse than declining to try. The error names the iteration count, the
projection and the budget.
One consequence is stated in the doc comment rather than hidden: acceptance is
machine-dependent, so a container near the budget can be refused on a slow host
and accepted on a fast one. That is inherent to bounding wall clock and is why
the error reports the projection.
Both tests pass, and the assertion checks the *reason* -- matching
`DerivationBudgetExceeded { iterations: u32::MAX, .. }` rather than `is_err()`,
which would have passed just as happily on an unrelated failure. The suite now
finishes in 3.4s instead of timing out at 60.
Not addressed here: `derive_key_argon2` takes memory and time costs from a
LUKS2 keyslot and is exposed the same way, bounded only by what `Params::new`
rejects. Worth its own change.
The coverage gate found the one line the fix leaves unexecuted: the `Err(_) => panic!(...)` arm of the 60-second watchdog. It is unreachable precisely because the budget works -- derivation now returns in milliseconds, so `recv_timeout` never expires. Annotating it `// cov:unreachable` was the obvious move and the wrong one: rustfmt relocates a trailing comment inside the `panic!` invocation, so the marker no longer sits on the line the gate measures, and the exemption would silently stop applying. `.expect()` removes the arm instead of exempting it -- which is what clippy suggested when the same code tripped `match_wild_err_arm`. The diagnosis is unchanged: if the bound ever regresses, the message still says so. CI's own gate (`cargo llvm-cov --workspace --all-features` + the lcov scan) now reports zero uncovered lines needing an exemption.
The follow-up this branch's PBKDF2 commit flagged. `derive_key_argon2` took the
time and memory costs straight from the keyslot and passed them to
`Params::new`, whose ceilings are `MAX_T_COST = u32::MAX` and
`MAX_M_COST = u32::MAX` **1 KiB blocks** — so upstream rejects nothing at the
top end and a crafted header can ask for 4 TiB of memory.
The failure is worse than the PBKDF2 hang. Writing the red test did not produce
a slow return or a panic; it produced
process didn't exit successfully: (signal: 9, SIGKILL: kill)
The OS killed the test binary. On an examiner's machine the tool does not
stall, it dies — and takes the rest of the run with it.
That also settles the design. Memory cannot be bounded by wall clock the way
the time cost is: *attempting* the allocation is itself the harm, and there is
no point at which a deadline could fire. So the two axes get different
treatment:
* memory is capped outright at 4 GiB, refused before anything allocates.
cryptsetup benchmarks LUKS2 keyslots to at most about 1 GiB, so that is
generous headroom over any genuine container. A dedicated
`DerivationMemoryExceeded` names the requested value and the ceiling.
* time is the Argon2 analogue of the iteration count and is bounded the same
way as PBKDF2 — measure one pass, project, refuse over DERIVATION_BUDGET.
Argon2 is linear in `t_cost`, so scaling a single pass is sound.
Order matters and is load-bearing: the memory ceiling is enforced first because
the calibration pass runs at the requested memory cost, so the measurement must
itself be safe.
Three tests, one per way the fix can be wrong: the 4 TiB memory cost is refused
(and the error names the value, so an examiner sees which cost the header
asked for), a u32::MAX time cost is refused behind a watchdog rather than run,
and a realistic 64 MiB / t=4 cost still derives reproducibly — a bound that
rejected real containers would be worse than the bug.
The memory assertion deliberately avoids `matches!` with a literal field
pattern: that generates a never-taken region the coverage gate reports as an
uncovered line, and exempting it would have annotated an artifact rather than
a defensive arm.
h4x0r
marked this pull request as ready for review
August 4, 2026 04:01
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.
The finding
The nightly
unlockfuzz target does not crash — it stops:derive_keytakes its iteration count straight from the LUKS keyslot, so it isattacker-controlled, and floors it without ever capping it:
A crafted header can ask for up to 4,294,967,295 PBKDF2 rounds. An examiner
opening that image waits forever: denial of service in a tool whose entire job
is reading hostile input.
The fix
Measure this machine's PBKDF2 rate on a short calibration run, project the cost
of the full count, and refuse before starting anything over a 30-second budget.
u32::MAXnow returnsDerivationBudgetExceededin milliseconds.The bound is on projected wall clock, not on the iteration count, because
"too many rounds" is a property of the machine rather than a number fixable in
advance — a count that is absurd on a laptop can be routine on a workstation.
PBKDF2 is exactly linear in its iteration count, so scaling a 20,000-iteration
measurement is sound.
Refusal, never a silent clamp. Deriving with fewer rounds than the header
specifies yields a different key, so clamping would report a wrong passphrase
for a container that would in fact have opened — fabricating a failed unlock,
which is worse than declining to try. The error names the iteration count, the
projection and the budget.
Two designs deliberately rejected
one first sketched. Reaching inside the loop means hand-rolling PBKDF2 around
a raw HMAC, and this module's opening line is that every primitive comes from
an audited RustCrypto crate.
spinning on the malicious input for the life of the process.
Known consequence
Acceptance is machine-dependent: a container near the budget can be refused on a
slow host and accepted on a fast one. That is inherent to bounding wall clock,
and is why the error reports the projection rather than just failing.
Verification
absurd_iteration_count_is_refused_rather_than_runasserts the reason —DerivationBudgetExceeded { iterations: u32::MAX, .. }, notis_err(), whichwould pass just as happily on an unrelated failure
rather than hanging the suite — a red that never terminates is not useful
a_realistic_iteration_count_still_derivespins 200,000 iterations (insidewhat cryptsetup writes) as untouched and stable, so the bound cannot quietly
start rejecting real containers
-D warnings, and the full workspace suite are clean; the suitenow finishes in 3.4s instead of timing out at 60
Not addressed here
derive_key_argon2takes memory and time costs from a LUKS2 keyslot and isexposed the same way, bounded only by what
Params::newhappens to reject.Worth its own change.
Update: the Argon2 half is now fixed too
The "Not addressed here" note below is resolved in this PR.
derive_key_argon2took both cost axes straight from the LUKS2 keyslot and handed them to
argon2::Params, whose ceilings areMAX_T_COST = u32::MAXandMAX_M_COST = u32::MAX1 KiB blocks — upstream rejects nothing at the topend, so a crafted header can request 4 TiB of memory.
That failure is worse than the PBKDF2 hang. The red test did not produce a slow
return or a panic:
The OS killed the test binary. On an examiner's machine the tool doesn't stall
— it dies, taking the rest of the run with it.
That also settles the design. Memory cannot be bounded by wall clock the way
the time cost is: attempting the allocation is itself the harm, and there is
no moment at which a deadline could fire. So the axes get different treatment:
dedicated
DerivationMemoryExceedednaming the requested value and theceiling. cryptsetup benchmarks LUKS2 keyslots to about 1 GiB at most, so this
is generous headroom over any genuine container.
PBKDF2 is: measure one pass, project, refuse over
DERIVATION_BUDGET. Argon2is linear in
t_cost, so scaling a single pass is sound.Order is load-bearing: the memory ceiling is enforced first, because the
calibration pass runs at the requested memory cost — the measurement must
itself be safe.
Verification
needs to see which cost the header asked for
u32::MAXtime cost refused behind a watchdog rather than runt=4cost still derives reproducibly — a bound thatrejected real containers would be worse than the bug
-D warnings· 7 suites, 0 failures · coverage gate 0uncovered lines
One note on the tests: the memory assertion deliberately avoids
matches!witha literal field pattern. That generates a never-taken region which the coverage
gate reports as an uncovered line, and exempting it with
// cov:unreachablewould have annotated a macro artifact rather than a defensive arm.