encryption: bound the Argon2id cost parameters before they are honoured#255
Conversation
An exhaustive single-bit-flip sweep over a sealed envelope, written downstream to prove tamper detection, did not fail. It aborted the test process partway through. One flipped bit in the m_cost_kib header field asks Argon2id for a 4 TiB allocation. The allocation fails, and a failed allocation in Rust aborts rather than unwinding, so a single corrupt byte in stored data takes the process down. The header is authenticated — it is the AEAD's associated data, and that is why nothing checked it. But verifying the tag needs the key, and deriving the key means first running Argon2id with the parameters the blob just supplied. There is a window, before anything is proven, where an attacker-chosen cost decides how much memory this process reserves. Tamper detection works exactly as designed and the process still dies before it gets there. Authenticated-but-only-later is not the same as trusted. KdfParams::validate() gates m/t/p against a policy ceiling (1 GiB, 64 passes, 64 lanes) and runs in derive_key and in decode_header, before any allocation. Argon2 itself permits up to u32::MAX KiB; nothing legitimate asks for that. The tests assert the refusal is CHEAP, not merely that it happens — an expensive rejection is the attack. The bit-flip sweep is kept as the reproducer: it is the test that found this, and without the ceiling it does not report a failure, it kills the test binary. All 28 crate tests green, clippy clean, presets unaffected. Generated by [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mwq1QKpw4zRd6oaGRoJhF2
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_e6813474-6e22-4a6b-a5f1-6dfef0f0b40b) |
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe encryption crate adds Argon2id cost-policy validation, applies it before allocation during key derivation and envelope header decoding, and adds tests covering oversized parameters, invalid costs, and bit-level blob corruption. ChangesEncryption cost validation hardening
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b91586733
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Argon2 itself permits up to `u32::MAX` KiB — 4 TiB. Nothing legitimate | ||
| /// asks for that, and honouring it costs more than refusing it: see | ||
| /// [`KdfParams::validate`]. | ||
| pub const MAX_M_COST_KIB: u32 = 1024 * 1024; |
There was a problem hiding this comment.
When open processes an attacker-controlled envelope on a browser or memory-limited service, this ceiling still permits Argon2 to allocate 1 GiB before authenticating the header, and combining it with the accepted 64 passes can consume substantial CPU as well. This therefore preserves the denial-of-service/allocator-abort path the change is intended to close; even the new bit-flip sweep admits a 512 MiB cost derived from FAST. The pre-authentication limit needs to reflect a reliably safe platform budget (or be externally configured/allowlisted), rather than merely being smaller than Argon2's 4 TiB maximum.
Useful? React with 👍 / 👎.
…er roof
Codex P1 on the first cut, and it was right. A ceiling of 1 GiB and 64
passes was chosen as "below Argon2's 4 TiB maximum", which is a rounding
error rather than a limit: 1 GiB with 64 passes, reached before the header
is authenticated, is just as fatal on a browser tab or a small container,
and a few concurrent requests carrying it exhaust the host. The abort was
closed and the denial of service left open.
CostLimits makes the budget the caller's, explicitly:
DEFAULT 128 MiB / 4 passes / 2 lanes — twice the memory and
one pass more than the heaviest shipped preset, so a
future cost bump still opens old and new blobs
SHIPPED_PRESETS_ONLY 64 MiB / 3 / 1 — exactly what this crate emits, for
a service that mints every blob it opens
open_within / derive_key_within take it; open / derive_key use DEFAULT.
Measured, release, this box: the worst header DEFAULT admits costs 414 ms
and 128 MiB. That number is printed by an #[ignore]d test rather than
asserted from a guess. The bit-flip sweep fell from 13.5 s to 2.3 s once
the tighter cap began refusing flips it used to honour — the sweep had been
quietly running multi-hundred-MiB derivations, which is the same finding
one layer down.
Not taken: an allowlist of known profiles. It breaks cost bumps in the
other direction — a reader shipped before the writer would reject the new
profile — and forward compatibility is exactly what the self-describing
header exists for.
31 tests green, clippy clean.
Generated by [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mwq1QKpw4zRd6oaGRoJhF2
2 participants
What happened
An exhaustive single-bit-flip sweep over a sealed envelope — written downstream to prove tamper detection — did not fail. It aborted the test process partway through the sweep.
One flipped bit in the
m_cost_kibheader field asks Argon2id for a 4 TiB allocation. The allocation fails, and a failed allocation in Rust aborts rather than unwinding. A single corrupt byte in stored data takes the process down.Why the existing reasoning didn't cover it
The header is authenticated — it's the AEAD's associated data, and the module says so explicitly ("an attacker cannot lower
m_cost_kib… and still have the blob open"). That's true, and it's why nothing checked it.But verifying the tag needs the key, and deriving the key means first running Argon2id with the parameters the blob just supplied. So there's a window, before anything is proven, where an attacker-chosen cost decides how much memory the process reserves. Tamper detection works exactly as designed and the process dies before reaching it.
Authenticated-but-only-later is not the same as trusted.
The change
KdfParams::validate()gates m/t/p against a policy ceiling — 1 GiB, 64 passes, 64 lanes — and runs inderive_keyand indecode_header, i.e. before any allocation. Argon2 itself permits up tou32::MAXKiB (4 TiB); nothing legitimate asks for that. NewKdfError::CostOutOfPolicy; shipped presets are unaffected.The tests assert the refusal is cheap, not merely that it happens — an expensive rejection is itself the attack. The bit-flip sweep is kept as the reproducer, since without the ceiling it doesn't report a failure, it kills the test binary.
28 crate tests + doctest green,
clippy --all-targetsclean, blackboard updated.Note on scope
The ceiling is a crate-level constant, not configurable. If a caller ever legitimately needs >1 GiB it becomes a builder parameter — but the default must stay bounded. Flagging rather than deciding.
Branch note:
claude/review-medcare-rust-dt7MSwas stale (fully merged, predating this crate), so it was restarted fromorigin/masterper the branch protocol.🤖 Generated with Claude Code
https://claude.ai/code/session_01Mwq1QKpw4zRd6oaGRoJhF2
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Security