fix: bound Argon2id cost parameters read from an untrusted superblock

src/crypto/mod.rs

@@ -139,6 +139,37 @@ use sha2::Sha256;
 /// future KDF revision can coexist.
 const KEK_INFO: &[u8] = b"chisel-kek-v1";
 
+// Upper bounds on Argon2id cost parameters.
+//
+// These exist because the parameters are ATTACKER-CONTROLLED: they are read
+// verbatim out of a key slot in the superblock's plaintext crypto header
+// (`KeySlot::read_from`) and handed to the KDF during `open`, before the
+// format-version gate and before the page-size gate. The argon2 crate provides
+// no ceiling of its own — `Params::MAX_M_COST` and `MAX_T_COST` are both
+// `u32::MAX`, with a source comment saying no upper check is needed — and its
+// working buffer is an infallible `vec![Block::default(); block_count()]`
+// (1 KiB per block). So an `m_cost` of `u32::MAX` is a 4 TiB request that
+// aborts the process rather than returning an error, and a large `t_cost` is
+// an unbounded hang. Four bytes in a file, and the host process dies.
+//
+// The cap is generous relative to 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. Raising them is a one-line change, but the open path must
+// stay bounded by SOMETHING — an unbounded allocator driven by untrusted bytes
+// is the vulnerability, not any particular ceiling.
+//
+// Enforced in `derive_kek` rather than at the slot-parse site because that is
+// the single point every KDF call routes through — both the open path
+// (recovery.rs, untrusted disk bytes) and the create path (recovery.rs, via
+// `Options::argon2_params`). Guarding there also stops a caller from creating
+// a database whose own parameters would make it unopenable.
+/// Maximum Argon2id memory cost in KiB (256 MiB).
+pub const MAX_ARGON2_M_COST: u32 = 262_144;
+/// Maximum Argon2id iteration count.
+pub const MAX_ARGON2_T_COST: u32 = 16;
+/// Maximum Argon2id parallelism (lanes).
+pub const MAX_ARGON2_P_COST: u32 = 16;
+
 /// Derive a 256-bit KEK from the client key and a slot's salt/params.
 ///
 /// Dispatch is on `kdf`, NOT on the `Key` variant: the slot records which KDF
@@ -150,10 +181,13 @@ const KEK_INFO: &[u8] = b"chisel-kek-v1";
 /// truth, so we never guess from the variant.)
 ///
 /// # Errors
-/// Returns `CryptoError::Kdf` if the KDF primitive rejects its parameters
-/// (e.g. Argon2id with zero memory cost). Returns `CryptoError::BadKeyLength`
-/// if the supplied key material is empty (an empty `Raw` key or empty
-/// `Passphrase`), regardless of `kdf`.
+/// Returns `CryptoError::Kdf` if the Argon2id cost parameters are outside
+/// `..=MAX_ARGON2_M_COST` / `..=MAX_ARGON2_T_COST` / `..=MAX_ARGON2_P_COST`
+/// (they arrive from an untrusted key slot on the open path, so they are
+/// bounded here before any allocation), or if the KDF primitive itself rejects
+/// them (e.g. Argon2id with zero memory cost). Returns
+/// `CryptoError::BadKeyLength` if the supplied key material is empty (an empty
+/// `Raw` key or empty `Passphrase`), regardless of `kdf`.
 pub fn derive_kek(
     key: &Key,
     kdf: KdfId,
@@ -182,6 +216,18 @@ pub fn derive_kek(
                 .map_err(|_| CryptoError::Kdf)?;
         }
         KdfId::Argon2id => {
+            // Refuse out-of-range cost parameters BEFORE constructing Params.
+            // `Params::new` accepts anything up to u32::MAX, and the allocation
+            // it authorizes is infallible, so this check is the only thing
+            // standing between a hostile key slot and a process abort. Note the
+            // HKDF arm above is deliberately unguarded: it never reads these
+            // fields, and slots written by the HKDF path leave them zero.
+            if params.m_cost > MAX_ARGON2_M_COST
+                || params.t_cost > MAX_ARGON2_T_COST
+                || params.p_cost > MAX_ARGON2_P_COST
+            {
+                return Err(CryptoError::Kdf);
+            }
             let p = Params::new(params.m_cost, params.t_cost, params.p_cost, Some(32))
                 .map_err(|_| CryptoError::Kdf)?;
             // Version::V0x13 and the 32-byte output length (`Some(32)` above) are
@@ -721,4 +767,96 @@ mod tests {
             "Argon2id output changed — KDF config or format break; update golden and bump FORMAT_VERSION"
         );
     }
+
+    // --- Bounds on cost parameters read from an untrusted superblock ---
+    //
+    // The argon2 crate deliberately enforces no upper bound: Params::MAX_M_COST
+    // and MAX_T_COST are both u32::MAX, and its block buffer is an infallible
+    // `vec![Block::default(); block_count()]`, so an oversized m_cost aborts the
+    // process rather than returning an error. Key-slot parameters come straight
+    // off disk, so the ceiling has to be ours. Literals here rather than the
+    // MAX_ARGON2_* constants so the intent is legible without chasing a const.
+
+    #[test]
+    fn derive_kek_rejects_argon2_params_above_the_cap() {
+        let key = Key::Passphrase(zeroize::Zeroizing::new("pw".to_string()));
+        let salt = [0u8; SALT_LEN];
+        let cases = [
+            // m_cost is in KiB: 262144 KiB = 256 MiB is the cap, so this is one
+            // KiB over. Deliberately not u32::MAX — pre-fix this test must FAIL,
+            // not abort the test runner by allocating 4 TiB.
+            (
+                "m_cost over cap",
+                Argon2Params {
+                    m_cost: 262_145,
+                    t_cost: 2,
+                    p_cost: 1,
+                },
+            ),
+            (
+                "t_cost over cap",
+                Argon2Params {
+                    m_cost: 19_456,
+                    t_cost: 17,
+                    p_cost: 1,
+                },
+            ),
+            (
+                "p_cost over cap",
+                Argon2Params {
+                    m_cost: 19_456,
+                    t_cost: 2,
+                    p_cost: 17,
+                },
+            ),
+        ];
+        for (what, bad) in cases {
+            assert!(
+                matches!(
+                    derive_kek(&key, KdfId::Argon2id, &salt, &bad),
+                    Err(CryptoError::Kdf)
+                ),
+                "{what}: out-of-range cost params must be refused before reaching argon2"
+            );
+        }
+    }
+
+    #[test]
+    fn derive_kek_still_accepts_realistic_argon2_params() {
+        let key = Key::Passphrase(zeroize::Zeroizing::new("pw".to_string()));
+        let salt = [1u8; SALT_LEN];
+        // The OWASP default this crate writes, and the cap itself, must both
+        // derive. Cheap t/p so the 256 MiB case stays quick.
+        for ok in [
+            Argon2Params {
+                m_cost: 8,
+                t_cost: 1,
+                p_cost: 1,
+            },
+            Argon2Params::default(),
+            Argon2Params {
+                m_cost: 262_144,
+                t_cost: 1,
+                p_cost: 1,
+            },
+        ] {
+            assert!(
+                derive_kek(&key, KdfId::Argon2id, &salt, &ok).is_ok(),
+                "legitimate params {ok:?} must still derive"
+            );
+        }
+    }
+
+    #[test]
+    fn hkdf_ignores_argon2_params_entirely() {
+        // Raw keys use HKDF, which never reads the cost params. A slot carrying
+        // garbage params with kdf_id=HKDF must not be refused by the new bound.
+        let key = Key::Raw(zeroize::Zeroizing::new(vec![7u8; 32]));
+        let wild = Argon2Params {
+            m_cost: u32::MAX,
+            t_cost: u32::MAX,
+            p_cost: u32::MAX,
+        };
+        assert!(derive_kek(&key, KdfId::Hkdf, &[0u8; SALT_LEN], &wild).is_ok());
+    }
 }

src/lib.rs

@@ -83,7 +83,11 @@ pub use page::format_major;
 // Key and Argon2Params are public API (callers need them to open encrypted DBs).
 // Crypto internals (PageCipher, CryptoError, raw constants) are pub(crate) in
 // their source modules and not re-exported here.
-pub use crypto::{Argon2Params, Key};
+// The MAX_ARGON2_* caps are public because exceeding them is now a hard
+// failure: `Options::argon2_params` above any of them makes `open` return
+// InvalidEncryptionKey at create time. A caller tuning cost parameters needs to
+// be able to see the ceiling rather than discover it by failing.
+pub use crypto::{Argon2Params, Key, MAX_ARGON2_M_COST, MAX_ARGON2_P_COST, MAX_ARGON2_T_COST};
 
 use std::path::Path;
 
@@ -270,6 +274,12 @@ impl Options {
     /// Set the Argon2id cost parameters used when deriving a KEK from a
     /// passphrase on database creation. No effect for raw keys or on reopen
     /// (the stored slot carries its own params). See [`Options::argon2_params`].
+    ///
+    /// Values are capped at [`MAX_ARGON2_M_COST`], [`MAX_ARGON2_T_COST`] and
+    /// [`MAX_ARGON2_P_COST`]; exceeding any of them makes `open` fail with
+    /// `InvalidEncryptionKey` rather than creating a database. The same cap is
+    /// what stops a hostile file's key slot from driving the Argon2 allocator
+    /// at open time, so it is enforced for both directions.
     pub fn argon2_params(mut self, params: Argon2Params) -> Self {
         self.argon2_params = Some(params);
         self

src/recovery_tests.rs

@@ -1650,3 +1650,69 @@ fn corrupt_crypto_header_stride_errors_not_panic() {
         Ok(_) => panic!("forged-stride open unexpectedly succeeded (should have failed)"),
     }
 }
+
+/// A hostile key slot must not be able to drive the Argon2 allocator.
+///
+/// The key-slot cost parameters (`m_cost`/`t_cost`/`p_cost`) are read verbatim
+/// out of the superblock's plaintext crypto header and handed to the KDF, and
+/// this happens BEFORE the format-version gate and the page-size gate — so no
+/// other validation stands between the file's bytes and the allocator. The
+/// argon2 crate enforces no ceiling of its own (`MAX_M_COST == u32::MAX`) and
+/// its block buffer is an infallible `vec![Block::default(); block_count()]`,
+/// which aborts the process on allocation failure rather than returning an
+/// error. An attacker who can hand the victim a `.chsl` file therefore gets a
+/// process kill, not a typed error.
+///
+/// Pre-fix this test aborts or hangs the runner (a 4 TiB request); post-fix it
+/// returns a normal operational error promptly.
+#[test]
+fn tampered_key_slot_cost_params_do_not_reach_the_allocator() {
+    use crate::superblock::{CRYPTO_HEADER_OFFSET, KEY_SLOT_SIZE};
+
+    let tmp = NamedTempFile::new().unwrap();
+    let pass = || Key::Passphrase(Zeroizing::new("correct horse battery".to_string()));
+
+    // Cheap-but-valid params so creating the fixture is fast; the values are
+    // overwritten below anyway.
+    let mut db = Chisel::open(
+        tmp.path(),
+        Options::default()
+            .encryption_key(pass())
+            .argon2_params(crate::Argon2Params {
+                m_cost: 8,
+                t_cost: 1,
+                p_cost: 1,
+            }),
+    )
+    .unwrap();
+    db.begin().unwrap();
+    db.allocate(b"payload").unwrap();
+    db.commit().unwrap();
+    drop(db);
+
+    // m_cost of key slot 0 lives at CRYPTO_HEADER_OFFSET + 8 (the slot table)
+    // + 2 (state byte, kdf_id byte), 4 bytes LE. Patch every superblock slot so
+    // the tampered header wins regardless of which slot recovery selects.
+    let m_cost_at = CRYPTO_HEADER_OFFSET + 8 + 2;
+    for sb_slot in 0..crate::DEFAULT_SUPERBLOCK_COUNT as u64 {
+        rewrite_page_with_valid_checksum(tmp.path(), sb_slot, |buf| {
+            // u32::MAX KiB = 4 TiB. Nothing legitimate is anywhere near this.
+            buf[m_cost_at..m_cost_at + 4].copy_from_slice(&u32::MAX.to_le_bytes());
+        });
+    }
+    // Slot stride sanity: this test hard-codes slot 0's offset, so a layout
+    // change that moved the table must fail loudly rather than silently
+    // patching the wrong bytes.
+    assert_eq!(
+        KEY_SLOT_SIZE, 128,
+        "key-slot stride changed; update m_cost_at"
+    );
+
+    let err = Chisel::open(tmp.path(), Options::default().encryption_key(pass()))
+        .err()
+        .expect("a slot with absurd cost params must not unwrap");
+    assert!(
+        matches!(err, ChiselError::InvalidEncryptionKey),
+        "expected InvalidEncryptionKey (the slot is skipped as non-matching), got {err:?}"
+    );
+}