fix: collapse the duplicated key-slot wrap/unwrap and validate the algorithm byte (#117)

src/crypto/mod.rs

@@ -424,8 +424,19 @@ impl PageCipher {
         nonce: &[u8; NONCE_LEN],
         tag: &[u8; TAG_LEN],
         ct: &[u8],
-    ) -> Result<Vec<u8>, CryptoError> {
-        open_detached(self.dek.as_bytes(), nonce, aad, ct, tag).map(|z| z.to_vec())
+    ) -> Result<Zeroizing<Vec<u8>>, CryptoError> {
+        // Return the Zeroizing buffer `open_detached` produced rather than
+        // `.to_vec()`-ing out of it. The old code allocated a fresh plain Vec and
+        // copied the plaintext in; the Zeroizing original was then dropped and
+        // wiped while the copy — the one the caller actually keeps — was freed
+        // un-scrubbed. That defeated the exact guarantee `open_detached`'s doc
+        // claims, one call later, and it is the only variable-length caller.
+        //
+        // What is in this buffer is the decrypted superblock body: root page
+        // pointers, total_pages, next_handle, freemap_depth, and the full
+        // named_roots table — user-chosen names that THEORY.md identifies as
+        // real user data and gives as the reason the body is sealed at all.
+        open_detached(self.dek.as_bytes(), nonce, aad, ct, tag)
     }
 }
 
@@ -695,7 +706,7 @@ mod tests {
         let (nonce, tag, ct) = pc.seal_body(aad, &body);
         assert_eq!(ct.len(), body.len(), "body cipher is length-preserving");
         let out = pc.open_body(aad, &nonce, &tag, &ct).unwrap();
-        assert_eq!(out, body);
+        assert_eq!(out.as_slice(), body);
     }
 
     #[test]

src/page_cache.rs

@@ -1031,8 +1031,27 @@ impl PageCache {
 
         let plaintext: [u8; PAGE_SIZE] = match &self.cipher {
             Some(c) => {
-                // The on-disk unit is exactly ENC_PAGE_SIZE at this stride;
-                // the try_into is infallible (ENC_PAGE_SIZE == 8232 == stride).
+                // The pairing invariant — cipher installed implies the stride was
+                // switched to ENC_PAGE_SIZE — was stated only in prose, here and
+                // in `set_cipher`'s doc, and checked nowhere. Assert it, because
+                // the failure mode is badly misleading: `read_page_unit_into`
+                // above fills only `stride` bytes of this 8232-byte stack buffer,
+                // so a cipher installed while the stride was still 8192 leaves
+                // the last 40 bytes as the buffer's zero-init rather than the
+                // real tag and nonce. `PageCipher::open` then fails, the caller
+                // gets `DecryptionFailed`, and `is_fatal()` poisons the handle —
+                // reporting a configuration mistake as unrecoverable ciphertext
+                // corruption.
+                //
+                // (The comment that used to sit here explained why a `try_into`
+                // was infallible. There is no `try_into`; the line below is a
+                // plain move of a Copy array, and the stale reference removed the
+                // one hint a reader had that a length check ever guarded this.)
+                debug_assert_eq!(
+                    stride, ENC_PAGE_SIZE,
+                    "cipher is installed but the IO stride is {stride}, not ENC_PAGE_SIZE — \
+                     set_cipher's contract was violated and the tag/nonce bytes are unread"
+                );
                 let unit: [u8; ENC_PAGE_SIZE] = on_disk;
                 c.open(page_id, &unit)
                     .map_err(|_| ChiselError::DecryptionFailed { page_id })?

src/recovery_tests.rs

@@ -64,6 +64,45 @@ fn rewrite_page_with_valid_checksum(
     f.sync_all().unwrap();
 }
 
+/// The same, for a superblock slot of an ENCRYPTED database.
+///
+/// `rewrite_page_with_valid_checksum` above seeks at `page_id * PAGE_SIZE`,
+/// which is wrong for an encrypted file: every unit — superblock slots included
+/// — sits at the 8232-byte ENC_PAGE_SIZE stride. Slot 0 lands at offset 0
+/// either way, so a test that forges "every slot" with the plaintext helper
+/// silently forges only slot 0 and CORRUPTS the rest: the write lands 40 bytes
+/// per slot too early, inside the previous unit, and the checksum restamp over
+/// a misaligned window leaves that slot unable to deserialize at all.
+///
+/// Such a test still passes, because the collaterally-torn siblings can never
+/// win selection — but it is testing torn-slot fallback rather than whatever it
+/// claims to test, and it would keep passing if the guard under test were
+/// removed and slot 0 stopped being the winner.
+///
+/// The superblock IMAGE is 8192 bytes at the start of its 8232-byte unit; the
+/// trailing 40 bytes stay zero (they are the seal footprint a data page would
+/// use). So read/write 8192 bytes AT the stride-derived offset.
+fn rewrite_encrypted_superblock_slot(
+    path: &std::path::Path,
+    slot: u64,
+    mutate: impl FnOnce(&mut [u8; PAGE_SIZE]),
+) {
+    let mut f = fs::OpenOptions::new()
+        .read(true)
+        .write(true)
+        .open(path)
+        .unwrap();
+    let off = slot * crate::crypto::ENC_PAGE_SIZE as u64;
+    let mut buf = [0u8; PAGE_SIZE];
+    f.seek(SeekFrom::Start(off)).unwrap();
+    f.read_exact(&mut buf).unwrap();
+    mutate(&mut buf);
+    page::stamp_checksum(&mut buf);
+    f.seek(SeekFrom::Start(off)).unwrap();
+    f.write_all(&buf).unwrap();
+    f.sync_all().unwrap();
+}
+
 /// Read the superblock slots (default layout: 2) and return the winning one,
 /// so a corruption test can target the LIVE root page rather than a stale COW
 /// copy that `find_page_of_type` might return.
@@ -1695,7 +1734,10 @@ fn tampered_key_slot_cost_params_do_not_reach_the_allocator() {
     // 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| {
+        // Stride-aware: this is an ENCRYPTED file, so slots sit at 8232 apart.
+        // The plaintext helper used to be used here and silently forged only
+        // slot 0 while tearing slot 1 — see rewrite_encrypted_superblock_slot.
+        rewrite_encrypted_superblock_slot(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());
         });
@@ -2249,3 +2291,109 @@ fn a_database_forged_at_the_boundary_refuses_to_commit_rather_than_writing_past_
         .expect("a refused commit must leave the file exactly as it was");
     assert_eq!(db.read(handle).unwrap(), b"still here afterwards");
 }
+
+#[test]
+fn an_unknown_crypto_algorithm_is_refused_rather_than_decrypted_as_xchacha20() {
+    // CRYPTO-4 / SUPERBLOCK-RECOVERY-3. The algorithm byte was written at create
+    // and never read back: `CryptoHeader::deserialize` gates only on zero
+    // ("plaintext"), so ANY nonzero value was accepted and every page was fed to
+    // PageCipher, which is hardcoded to XChaCha20-Poly1305.
+    //
+    // The DEK unwrap is a separate primitive that does not depend on the page
+    // algorithm, so the open SUCCEEDED — and the failure surfaced later as
+    // DecryptionFailed, which is_fatal() and poisons the handle. A future
+    // algorithm id, or a forged byte, was therefore reported to the user as
+    // unrecoverable data corruption rather than "this build cannot read that".
+    use crate::superblock::CRYPTO_HEADER_OFFSET;
+
+    let tmp = NamedTempFile::new().unwrap();
+    let raw = || Key::Raw(Zeroizing::new(vec![0x7Au8; 32]));
+
+    let mut db = Chisel::open(tmp.path(), Options::default().encryption_key(raw())).unwrap();
+    db.begin().unwrap();
+    db.allocate(b"payload").unwrap();
+    db.commit().unwrap();
+    drop(db);
+
+    // Stamp a future algorithm id into every slot so the forgery wins whichever
+    // slot recovery selects. 2 is "some cipher this build does not have".
+    for sb_slot in 0..crate::DEFAULT_SUPERBLOCK_COUNT as u64 {
+        rewrite_encrypted_superblock_slot(tmp.path(), sb_slot, |buf| {
+            buf[CRYPTO_HEADER_OFFSET] = 2;
+        });
+    }
+
+    match Chisel::open(tmp.path(), Options::default().encryption_key(raw())) {
+        // Operational, not fatal: the file is intact, this build just cannot
+        // read it. That distinction is the entire point of the fix.
+        Err(e @ ChiselError::EncryptionNotSupported) => {
+            assert!(!e.is_fatal(), "an algorithm mismatch must not poison");
+        }
+        Ok(_) => panic!("an unknown algorithm id must not open"),
+        Err(other) => panic!("expected EncryptionNotSupported, got {other:?}"),
+    }
+}
+
+#[test]
+fn a_raw_key_database_writes_zero_argon2_params_as_the_format_documents() {
+    // CRYPTO-6, and the reason CRYPTO-7's dedup was worth doing. The on-disk
+    // layout is documented twice — crypto_header.rs and ARCHITECTURE.md — as
+    // "argon2 params ... (zero for HKDF)". `wrap_into` honoured that;
+    // `build_create_cipher` was a second, hand-maintained implementation of the
+    // same operation and wrote Argon2Params::default() instead.
+    //
+    // So a database created with Key::Raw carried m=19456,t=2,p=1 in a slot
+    // whose kdf_id said HKDF, while a second raw credential added later through
+    // add_key carried zeros for identical semantics: two paths, different bytes,
+    // neither matching the format doc. Now both run through wrap_into.
+    use crate::superblock::{CRYPTO_HEADER_OFFSET, KEY_SLOT_SIZE};
+
+    let tmp = NamedTempFile::new().unwrap();
+    let first = || Key::Raw(Zeroizing::new(vec![0x11u8; 32]));
+    let second = || Key::Raw(Zeroizing::new(vec![0x22u8; 32]));
+
+    let mut db = Chisel::open(tmp.path(), Options::default().encryption_key(first())).unwrap();
+    db.begin().unwrap();
+    db.allocate(b"payload").unwrap();
+    db.commit().unwrap();
+    // Slot 1 comes from add_key — the path that was always correct. Both slots
+    // must now agree, which is the property that stops the two implementations
+    // drifting apart again.
+    db.add_key(&first(), &second()).unwrap();
+    drop(db);
+
+    let mut f = fs::File::open(tmp.path()).unwrap();
+    let mut unit = [0u8; crate::crypto::ENC_PAGE_SIZE];
+    // Read every superblock slot and check whichever ones carry an active key
+    // slot; the header is cleartext in all of them.
+    let mut checked = 0;
+    for sb_slot in 0..crate::DEFAULT_SUPERBLOCK_COUNT as u64 {
+        f.seek(SeekFrom::Start(
+            sb_slot * crate::crypto::ENC_PAGE_SIZE as u64,
+        ))
+        .unwrap();
+        f.read_exact(&mut unit).unwrap();
+        for key_slot in 0..2usize {
+            let base = CRYPTO_HEADER_OFFSET + 8 + key_slot * KEY_SLOT_SIZE;
+            if unit[base] != 1 {
+                continue; // slot not active
+            }
+            assert_eq!(
+                unit[base + 1],
+                1,
+                "both credentials are raw, so kdf_id=HKDF"
+            );
+            assert_eq!(
+                &unit[base + 2..base + 14],
+                &[0u8; 12],
+                "superblock slot {sb_slot} key slot {key_slot}: HKDF slots must \
+                 carry zero argon2 params, as the format doc states"
+            );
+            checked += 1;
+        }
+    }
+    assert!(
+        checked >= 2,
+        "expected to inspect both key slots; only saw {checked}"
+    );
+}

src/superblock/crypto_header.rs

@@ -182,9 +182,18 @@ impl CryptoHeader {
     /// dek)` for the first slot whose AEAD tag verifies. If no slot matches,
     /// returns `Err(InvalidEncryptionKey)`.
     ///
-    /// This is byte-identical to the inline trial in `recovery.rs`
-    /// (`unwrap_first_matching_slot`) — both call `slot.aad()` on the
-    /// fully-populated slot before passing it to `unwrap_dek`.
+    /// This is the ONLY unwrap-trial implementation: the open path
+    /// (`recovery.rs`) and the key-management paths (`keys.rs`) both call it.
+    ///
+    /// `recovery.rs` used to carry a second copy, whose doc asserted the two
+    /// were "byte-identical". They did in fact behave identically — the copies
+    /// differed only in spelling (`1 =>` / `2 =>` against `x if x == KdfId::Hkdf
+    /// as u8`, and the discriminants are 1 and 2). The reason to collapse them
+    /// was not that this pair had drifted but that the WRAP pair already had:
+    /// `build_create_cipher` wrote the OWASP argon2 defaults into HKDF slots
+    /// where `wrap_into` wrote the zeros the format documents (CRYPTO-6). A
+    /// comment asserting two implementations agree is not a mechanism for
+    /// keeping them agreeing, and the wrap side is the proof.
     ///
     /// # Errors
     /// Returns `ChiselError::InvalidEncryptionKey` if no active slot's tag
@@ -238,8 +247,20 @@ impl CryptoHeader {
         slot: usize,
         key: &crate::crypto::Key,
         dek: &crate::crypto::Dek,
+        argon2_override: Option<Argon2Params>,
     ) -> Result<(), crate::crypto::CryptoError> {
         use crate::crypto::{self, KdfId};
+        // HKDF slots write ZERO cost params, which is what the on-disk layout
+        // doc (and ARCHITECTURE.md) promise: "argon2 params ... (zero for
+        // HKDF)". The create path used to write `Argon2Params::default()` here
+        // instead, so a database created with `Key::Raw` carried m=19456,t=2,p=1
+        // in a slot whose kdf_id said HKDF, while a second raw credential added
+        // later through `add_key` carried zeros for identical semantics. Both
+        // paths now run through this function, so they cannot drift again.
+        //
+        // `argon2_override` is the caller's `Options::argon2_params`. It only
+        // has effect on the Argon2id branch — a raw key uses HKDF regardless,
+        // and HKDF never reads these fields.
         let (kdf_id, argon2) = match key {
             crate::crypto::Key::Raw(_) => (
                 KdfId::Hkdf,
@@ -249,7 +270,9 @@ impl CryptoHeader {
                     p_cost: 0,
                 },
             ),
-            crate::crypto::Key::Passphrase(_) => (KdfId::Argon2id, Argon2Params::default()),
+            crate::crypto::Key::Passphrase(_) => {
+                (KdfId::Argon2id, argon2_override.unwrap_or_default())
+            }
         };
         let salt: [u8; SALT_LEN] = crypto::random_array();
         let wrap_nonce: [u8; NONCE_LEN] = crypto::random_array();
@@ -293,7 +316,7 @@ mod crypto_header_tests {
             stride: crypto::ENC_PAGE_SIZE as u32,
             slots: [KeySlot::EMPTY; KEY_SLOT_COUNT],
         };
-        h.wrap_into(0, key, dek)
+        h.wrap_into(0, key, dek, None)
             .expect("wrap_into with valid key must succeed");
         h
     }
@@ -306,7 +329,7 @@ mod crypto_header_tests {
 
         // Add a second credential into slot 3 wrapping the SAME dek.
         let k1 = raw(0xB2);
-        h.wrap_into(3, &k1, &dek)
+        h.wrap_into(3, &k1, &dek, None)
             .expect("wrap_into with valid key must succeed");
 
         let (idx0, d0) = h.unlock(&k0).expect("k0 must unlock");
@@ -349,7 +372,7 @@ mod crypto_header_tests {
 
         // Fill every remaining slot.
         for i in 1..KEY_SLOT_COUNT {
-            h.wrap_into(i, &raw(i as u8 + 1), &dek)
+            h.wrap_into(i, &raw(i as u8 + 1), &dek, None)
                 .expect("wrap_into with valid key must succeed");
         }
         assert_eq!(h.active_count(), KEY_SLOT_COUNT);
@@ -367,7 +390,7 @@ mod crypto_header_tests {
             stride: crypto::ENC_PAGE_SIZE as u32,
             slots: [KeySlot::EMPTY; KEY_SLOT_COUNT],
         };
-        h.wrap_into(5, &key, &dek)
+        h.wrap_into(5, &key, &dek, None)
             .expect("wrap_into with valid key must succeed");
         let (idx, recovered) = h.unlock(&key).expect("wrap_into then unlock must succeed");
         assert_eq!(idx, 5);

src/superblock/mod.rs

@@ -34,6 +34,7 @@
 use crate::crypto::{CryptoError, NONCE_LEN, TAG_LEN};
 use crate::page::{self, MAGIC, PAGE_SIZE};
 use std::fmt;
+use zeroize::Zeroizing;
 
 mod crypto_header;
 // Re-export the crypto-header API for consumers (open/create code in later
@@ -418,8 +419,19 @@ impl Superblock {
 
     /// Assemble the plaintext body for sealing: all sensitive fields in the
     /// canonical order defined by BODY_LEN. Called only for encrypted DBs.
-    fn body_plaintext(&self) -> Vec<u8> {
-        let mut b = Vec::with_capacity(BODY_LEN);
+    /// Zeroizing for the same reason `open_body` returns one: this buffer holds
+    /// root page pointers, `total_pages`, `next_handle`, `freemap_depth` and the
+    /// full `named_roots` table — user-chosen names that THEORY.md calls out as
+    /// real user data and gives as the reason the body is sealed at all.
+    ///
+    /// The read side was fixed first (CRYPTO-8), but the write side leaks the
+    /// identical plaintext and does it more often: `serialize_encrypted` calls
+    /// this on EVERY superblock write, so every commit of an encrypted database
+    /// left a copy in freed, un-wiped heap. `seal_detached` itself is fine — it
+    /// encrypts in place, overwriting the plaintext with ciphertext — but the
+    /// buffer handed to it was a plain `Vec` dropped as a temporary.
+    fn body_plaintext(&self) -> Zeroizing<Vec<u8>> {
+        let mut b = Zeroizing::new(Vec::with_capacity(BODY_LEN));
         b.extend_from_slice(&self.root_handle_table_page.to_le_bytes());
         b.extend_from_slice(&self.root_freemap_page.to_le_bytes());
         b.extend_from_slice(&self.root_membership_index_page.to_le_bytes());

src/transaction/keys.rs

@@ -153,7 +153,12 @@ impl TransactionManager {
         let free = header.free_slot().ok_or(ChiselError::NoFreeKeySlot)?;
         let mut new_header = *header;
         new_header
-            .wrap_into(free, new, &dek)
+            // `None` = the OWASP defaults, which is what this path has always
+            // used and what `Options::argon2_params` documents ("No effect ...
+            // on reopen"). `wrap_into` now takes the override because the create
+            // path needs it; threading a caller-chosen cost into add_key/
+            // rotate_key would be a public API change, not a cleanup.
+            .wrap_into(free, new, &dek, None)
             .map_err(|_| ChiselError::InvalidEncryptionKey)?;
         self.rewrite_crypto_header(new_header)
     }
@@ -183,7 +188,12 @@ impl TransactionManager {
         let free = header.free_slot().ok_or(ChiselError::NoFreeKeySlot)?;
         let mut new_header = *header;
         new_header
-            .wrap_into(free, new, &dek)
+            // `None` = the OWASP defaults, which is what this path has always
+            // used and what `Options::argon2_params` documents ("No effect ...
+            // on reopen"). `wrap_into` now takes the override because the create
+            // path needs it; threading a caller-chosen cost into add_key/
+            // rotate_key would be a public API change, not a cleanup.
+            .wrap_into(free, new, &dek, None)
             .map_err(|_| ChiselError::InvalidEncryptionKey)?;
         // Clear the old slot in the same header snapshot — single atomic rewrite.
         new_header.slots[old_idx] = crate::superblock::KeySlot::EMPTY;
@@ -333,7 +343,7 @@ mod tests {
             .unlock(&raw(0x11))
             .expect("slot 0 unlocks with key 0x11");
         new_hdr
-            .wrap_into(1, &raw(0x22), &dek)
+            .wrap_into(1, &raw(0x22), &dek, None)
             .expect("wrap_into with valid key must succeed");
 
         db.rewrite_crypto_header(new_hdr).unwrap();
@@ -435,7 +445,7 @@ mod tests {
         let mut new_hdr = db.crypto_header.unwrap();
         let (_, dek) = new_hdr.unlock(&raw(0x11)).unwrap();
         new_hdr
-            .wrap_into(1, &raw(0x22), &dek)
+            .wrap_into(1, &raw(0x22), &dek, None)
             .expect("wrap_into with valid key must succeed");
         db.rewrite_crypto_header(new_hdr).unwrap();
         // Drop to flush OS buffers (fsync already called).