Skip to content

fix: bound Argon2id cost parameters read from an untrusted superblock - #128

Merged
Xof merged 1 commit into
mainfrom
fix/bound-argon2-params-from-disk
Jul 29, 2026
Merged

fix: bound Argon2id cost parameters read from an untrusted superblock#128
Xof merged 1 commit into
mainfrom
fix/bound-argon2-params-from-disk

Conversation

@Xof

@Xof Xof commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Fixes the issue behind draft advisory GHSA-2p8x-4r6r-8jpw.

The bug

Key-slot cost parameters are parsed verbatim out of the superblock's plaintext crypto header and handed to the KDF:

// src/superblock/crypto_header.rs:110 — no range check
m_cost: u32::from_le_bytes(slot[2..6].try_into().unwrap()),
// src/transaction/recovery.rs:649 — straight to the KDF
let kek = match derive_kek(key, kdf, &slot.salt, &slot.argon2) {

This runs at recovery.rs:342, before the format-version gate (:384) and before the page-size gate (:406), so no other validation intervenes. And argon2 enforces no ceiling of its own:

// argon2-0.5.3/src/params.rs
pub const MAX_M_COST: u32 = u32::MAX;
// "Note: we don't need to check `MAX_M_COST`, since it's `u32::MAX`"
pub const MAX_T_COST: u32 = u32::MAX;
// argon2-0.5.3/src/lib.rs:230 — infallible; aborts the process on OOM
let mut blocks = vec![Block::default(); self.params.block_count()];

m_cost is in KiB, so u32::MAX is a 4 TiB request that kills the host process rather than returning an error; a large t_cost is an unbounded hang instead. A failed slot just continues, so the cost is paid for each of up to 8 active slots.

Measured

An otherwise-valid encrypted database, with u32::MAX patched into slot 0's m_cost and the page checksum re-stamped. Opening it with the correct passphrase:

  PID    RSS      VSZ ELAPSED STAT
 1401 15097424 4705335920   00:12 R      # RSS 14 GB, VSZ 4.4 TiB
 1401 13871232 4705335920   00:20 R      # still climbing

Killed manually. Four bytes in a file. On any host smaller than this one it is an immediate OOM kill.

The fix

Bound the three parameters in derive_kek, before Params::new and therefore before any allocation.

derive_kek is chosen deliberately over the slot-parse site: it is the single point both paths route through — the open path (untrusted disk bytes) and the create path (Options::argon2_params). Guarding there also stops a caller from creating a database whose own parameters would make it unopenable. The HKDF arm is left unguarded on purpose: it never reads these fields, and HKDF-written slots leave them zero. That is pinned by a test.

Caps: m_cost ≤ 256 MiB, t_cost ≤ 16, p_cost ≤ 16. Generous against real configurations — 256 MiB is 13x the OWASP baseline this crate writes (19 MiB), and t/p of 16 are well above the recommended 2/1. The specific numbers matter less than the fact that the open path is bounded at all; raising them is a one-line change.

On the open path an over-cap slot is now skipped as non-matching, so open ends in the existing operational InvalidEncryptionKey — a typed error instead of a dead process.

Compatibility

Worth a reviewer's eye: a database created with Options::argon2_params above these caps would no longer open. Nothing in-tree writes such values (the only params this crate writes are the OWASP defaults), and the caps are far above any realistic setting, but it is a behaviour change rather than a pure hardening. The caps are re-exported from the crate root so the ceiling is discoverable rather than something you hit by failing.

Related rough edge, not addressed here: over-cap params on the create path surface as InvalidEncryptionKey via the blanket From<CryptoError>, which is a confusing error for "your cost parameters are too large". Worth a distinct variant, but that is a wider error-taxonomy change than this fix should carry.

Tests

Written before the fix and confirmed failing for the right reason:

  • derive_kek_rejects_argon2_params_above_the_cap — one KiB over the cap, deliberately not u32::MAX, so the pre-fix run fails rather than aborting the test runner
  • derive_kek_still_accepts_realistic_argon2_params — the OWASP default and the cap itself must still derive (passed before and after; pins that the guard is not over-tight)
  • hkdf_ignores_argon2_params_entirelyu32::MAX params with kdf_id=HKDF must still succeed
  • tampered_key_slot_cost_params_do_not_reach_the_allocator — end-to-end: build a real encrypted DB, patch u32::MAX into every superblock slot's key slot 0, re-stamp the checksums, reopen. Pre-fix this is the 4.4 TiB run above; post-fix it returns InvalidEncryptionKey in under two seconds.

cargo test 685 passed / 0 failed · cargo clippy --workspace -- -D warnings clean · cargo fmt --check clean.

Note for reviewers

Branched from main rather than the local development lineage, which shares no ancestry with it. The files involved are byte-identical across the two, so this applies cleanly either way — but it needs porting to the dev lineage separately. Independent of #127; they touch different files.

Found by the clean-slate deep review of 2026-07-29 (finding SUPERBLOCK-RECOVERY-1).

Key-slot cost parameters (m_cost/t_cost/p_cost) are parsed verbatim out of
the superblock's plaintext crypto header and handed to the KDF during
`open`. That happens before the format-version gate and before the
page-size gate, so nothing stands between the file's bytes and the
allocator.

The argon2 crate enforces no ceiling of its own: Params::MAX_M_COST and
MAX_T_COST are both u32::MAX, with a source comment stating no upper check
is needed. Its working buffer is an infallible
`vec![Block::default(); block_count()]` at 1 KiB per block, so allocation
failure aborts the process instead of returning an error. An m_cost of
u32::MAX is a 4 TiB request; a large t_cost is an unbounded hang. Both are
retried for each of up to 8 active slots.

Measured before the fix, with u32::MAX patched into slot 0 of an otherwise
valid database: the opening process reserved 4.4 TiB of address space and
had 14 GB resident and climbing before it was killed. Four bytes in a file.

Bounds the three parameters in `derive_kek`, which is the single point both
the open path and the create path route through, so a hostile slot and a
self-inflicted `Options::argon2_params` are both covered. The caps are
generous against real configurations — 256 MiB is 13x the OWASP baseline
this crate writes, and t/p of 16 are well above the recommended 2/1 — but
the open path must be bounded by something. The HKDF arm is deliberately
left unguarded: it never reads these fields.

On the open path an over-cap slot is skipped as non-matching and `open`
ends in the existing operational InvalidEncryptionKey, so a hostile file
now produces a typed error rather than a dead process.

The caps are re-exported from the crate root because exceeding them is a
hard failure that a caller tuning cost parameters needs to be able to see.
@Xof
Xof merged commit 8a5ae13 into main Jul 29, 2026
10 checks passed
@github-actions

Copy link
Copy Markdown

🚦 Bench results: PR vs main

⚠️ 5 regression(s) detected across 3 scenario/mode pair(s)

Scenario Mode Δ throughput Worst Δ
mutation-log chisel-mem -9.0% p50 +12.6% ⚠️
ycsb-a sqlite-strict -1.1% p99 +11.4% ⚠️
ycsb-b chisel-strict -6.7% throughput -6.7% ⚠️
document-store chisel-mem -1.9%
document-store chisel-strict +0.6%
document-store redb-strict +1.7%
document-store sqlite-strict -0.6%
mutation-log chisel-strict +1.5%
mutation-log redb-strict +0.3%
mutation-log sqlite-strict -0.4%
ycsb-a chisel-mem +5.2%
ycsb-a chisel-strict +1.5%
ycsb-a redb-strict -0.1%
ycsb-b chisel-mem +0.9%
ycsb-b redb-strict +1.9%
ycsb-b sqlite-strict -0.4%
Per-scenario detail (4 metrics × cells)

document-store

Mode Throughput p50 p95 p99
chisel-mem 34114 ops/s → 33483 ops/s (-1.9%) 5.1 µs → 5.2 µs (+1.0%) 66.6 µs → 66.4 µs (-0.2%) 292.2 µs → 302.0 µs (+3.4%)
chisel-strict 3493 ops/s → 3513 ops/s (+0.6%) 12.4 µs → 12.4 µs (+0.4%) 1.04 ms → 1.03 ms (-1.7%) 1.71 ms → 1.86 ms (+8.4%)
redb-strict 7024 ops/s → 7145 ops/s (+1.7%) 17.7 µs → 18.1 µs (+2.2%) 581.5 µs → 562.9 µs (-3.2%) 1.19 ms → 1.22 ms (+2.1%)
sqlite-strict 7180 ops/s → 7136 ops/s (-0.6%) 19.9 µs → 20.2 µs (+1.4%) 411.4 µs → 407.9 µs (-0.8%) 1.21 ms → 1.24 ms (+2.3%)

mutation-log

Mode Throughput p50 p95 p99
chisel-mem 53675 ops/s → 48829 ops/s (-9.0%) ⚠️ 23.2 µs → 26.1 µs (+12.6%) ⚠️ 27.7 µs → 29.5 µs (+6.7%) 36.0 µs → 39.1 µs (+8.4%)
chisel-strict 2267 ops/s → 2300 ops/s (+1.5%) 424.3 µs → 411.2 µs (-3.1%) 881.7 µs → 875.3 µs (-0.7%) 1.24 ms → 1.13 ms (-8.9%)
redb-strict 3754 ops/s → 3766 ops/s (+0.3%) 178.9 µs → 183.0 µs (+2.3%) 263.4 µs → 264.5 µs (+0.4%) 544.8 µs → 545.2 µs (+0.1%)
sqlite-strict 7118 ops/s → 7087 ops/s (-0.4%) 123.7 µs → 124.4 µs (+0.5%) 333.7 µs → 335.7 µs (+0.6%) 451.4 µs → 449.9 µs (-0.3%)

ycsb-a

Mode Throughput p50 p95 p99
chisel-mem 41105 ops/s → 43254 ops/s (+5.2%) 40.9 µs → 38.8 µs (-5.1%) 56.9 µs → 53.7 µs (-5.7%) 72.5 µs → 70.0 µs (-3.5%)
chisel-strict 2739 ops/s → 2780 ops/s (+1.5%) 334.2 µs → 329.3 µs (-1.5%) 970.5 µs → 966.4 µs (-0.4%) 1.22 ms → 1.25 ms (+2.0%)
redb-strict 5601 ops/s → 5594 ops/s (-0.1%) 155.5 µs → 155.2 µs (-0.2%) 232.6 µs → 240.4 µs (+3.4%) 338.9 µs → 363.1 µs (+7.1%)
sqlite-strict 147714 ops/s → 146035 ops/s (-1.1%) 7.0 µs → 7.0 µs (+0.0%) 8.8 µs → 8.9 µs (+1.5%) 10.5 µs → 11.7 µs (+11.4%) ⚠️

ycsb-b

Mode Throughput p50 p95 p99
chisel-mem 255002 ops/s → 257216 ops/s (+0.9%) 1.4 µs → 1.4 µs (+2.1%) 48.9 µs → 40.6 µs (-16.9%) 54.3 µs → 52.5 µs (-3.3%)
chisel-strict 25391 ops/s → 23680 ops/s (-6.7%) ⚠️ 2.7 µs → 2.9 µs (+5.6%) ⚠️ 328.8 µs → 352.8 µs (+7.3%) 864.2 µs → 916.9 µs (+6.1%)
redb-strict 56057 ops/s → 57132 ops/s (+1.9%) 2.6 µs → 2.6 µs (-0.8%) 153.7 µs → 156.4 µs (+1.7%) 220.5 µs → 214.9 µs (-2.5%)
sqlite-strict 180692 ops/s → 179902 ops/s (-0.4%) 5.5 µs → 5.6 µs (+0.4%) 7.4 µs → 7.5 µs (+1.1%) 9.6 µs → 9.7 µs (+0.8%)
Generated by chisel-bench-diff at 2026-07-29T19:22:38Z. Compares PR HEAD against main. Never blocks merge — signal, not gate. Thresholds: throughput 5%, p50 5%, p95 10%, p99 10%.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant