From 5b3928ce3cd92fd91e75784552a7cdb1edd9d7d0 Mon Sep 17 00:00:00 2001 From: Christophe Pettus Date: Tue, 4 Aug 2026 17:56:43 -0700 Subject: [PATCH] fix: collapse the duplicated key-slot wrap/unwrap and validate the algorithm byte MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #117; also closes #103's SUPERBLOCK-RECOVERY-3, which is the same defect as CRYPTO-4 filed twice. CRYPTO-7 / CRYPTO-6. The slot wrap and the slot unwrap-trial each existed twice, in the crate's highest-consequence code. `unlock`'s doc asserted its copy was "byte-identical" to `unwrap_first_matching_slot`; on the unwrap side that happened to be true (the copies differed only in how they spelled the kdf-id match, and the discriminants are 1 and 2). On the WRAP side it was already false: `wrap_into` wrote the zero argon2 params the on-disk format documents for HKDF slots, while `build_create_cipher` wrote the OWASP defaults (19456/2/1) into a slot whose kdf_id said HKDF. So a database created with Key::Raw disagreed byte-for-byte with a second raw credential added later through add_key, and neither matched the format doc — the exact drift a comment claiming equivalence cannot prevent and a shared function can. `open_existing` now calls `header.unlock(k)`; `build_create_cipher` builds an empty header and calls `wrap_into`, which gained an `argon2_override` parameter for the create path's `Options::argon2_params`. Existing databases are unaffected: `unlock` rebuilds the AAD from the STORED slot bytes and derives the KEK from the STORED params, and HKDF never reads those fields, so a legacy slot carrying 19456/2/1 still unwraps. The adversarial review confirmed that end to end against a file created by the old code. CRYPTO-4 / SUPERBLOCK-RECOVERY-3. The algorithm byte was written at create and never read back — `deserialize` gates only on zero ("plaintext"), so any nonzero value was accepted and every page 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 such a file OPENED and then failed later as DecryptionFailed, which is_fatal() and poisons: "this build cannot read that algorithm" reported to the user as unrecoverable data corruption. Rejected now with EncryptionNotSupported, whose doc already described this case. The check sits before the key-presence match, so an unopenable file says so whether or not a key was supplied, rather than first demanding a key that could never work. CRYPTO-9. Removed a comment explaining why a `try_into` was infallible; there is no try_into, and the stale reference removed the one hint a reader had that a length check ever guarded this. Replaced the prose-only pairing invariant with a debug_assert: read_page_unit_into fills only `stride` bytes of an 8232-byte stack buffer, so a cipher installed while the stride was still 8192 would read the last 40 bytes as zero-init rather than the real tag and nonce, and report a configuration mistake as ciphertext corruption. CRYPTO-8. `open_body` returns the Zeroizing buffer instead of `.to_vec()`-ing out of it — the old code allocated a plain copy and let the Zeroizing original be wiped, defeating the guarantee its own doc claims one call later. The review then pointed out the seal side does the same thing more often: `body_plaintext` builds root pointers, next_handle and the full named_roots table in a plain Vec dropped as a temporary on EVERY superblock write, so every commit of an encrypted DB left a copy in freed un-wiped heap. Fixed too; the claim now holds in both directions. Review findings folded in: * The new algorithm test forged "every slot" with a helper that seeks at page_id * PAGE_SIZE. On an encrypted file slots sit 8232 apart, so it forged only slot 0 (offset 0 either way) and silently TORE slot 1 — the write landing 40 bytes early, inside the previous unit. It passed only because a torn sibling can never win selection, i.e. it was testing torn-slot fallback. Added a stride-aware helper and switched both this test and the pre-existing hostile-m_cost test, which carried the same bug and the same untrue comment. * My own comments claimed the two unwrap copies "had already diverged in how they mapped kdf ids". They had not — that is the wrap side. Corrected, since a comment inventing a historical bug is the same defect class this issue exists to fix. --- src/crypto/mod.rs | 17 +++- src/page_cache.rs | 23 ++++- src/recovery_tests.rs | 150 +++++++++++++++++++++++++++++++- src/superblock/crypto_header.rs | 39 +++++++-- src/superblock/mod.rs | 16 +++- src/transaction/keys.rs | 18 +++- src/transaction/recovery.rs | 139 +++++++++++------------------ 7 files changed, 294 insertions(+), 108 deletions(-) diff --git a/src/crypto/mod.rs b/src/crypto/mod.rs index 8da5d45..6c6f4bb 100644 --- a/src/crypto/mod.rs +++ b/src/crypto/mod.rs @@ -424,8 +424,19 @@ impl PageCipher { nonce: &[u8; NONCE_LEN], tag: &[u8; TAG_LEN], ct: &[u8], - ) -> Result, CryptoError> { - open_detached(self.dek.as_bytes(), nonce, aad, ct, tag).map(|z| z.to_vec()) + ) -> Result>, 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] diff --git a/src/page_cache.rs b/src/page_cache.rs index 188134d..01ca4a2 100644 --- a/src/page_cache.rs +++ b/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 })? diff --git a/src/recovery_tests.rs b/src/recovery_tests.rs index 11704b1..d3524a9 100644 --- a/src/recovery_tests.rs +++ b/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}" + ); +} diff --git a/src/superblock/crypto_header.rs b/src/superblock/crypto_header.rs index 119abce..4c7b7f5 100644 --- a/src/superblock/crypto_header.rs +++ b/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, ) -> 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); diff --git a/src/superblock/mod.rs b/src/superblock/mod.rs index 383871f..3db14cb 100644 --- a/src/superblock/mod.rs +++ b/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 { - 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> { + 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()); diff --git a/src/transaction/keys.rs b/src/transaction/keys.rs index 0c5b333..752064b 100644 --- a/src/transaction/keys.rs +++ b/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). diff --git a/src/transaction/recovery.rs b/src/transaction/recovery.rs index d380be0..0706685 100644 --- a/src/transaction/recovery.rs +++ b/src/transaction/recovery.rs @@ -332,14 +332,49 @@ impl TransactionManager { // For an encrypted DB we must decrypt the sealed body (which holds // total_pages, named_roots, etc.) BEFORE the page-size and // total_pages checks that read those fields. + // Checked BEFORE the key-presence match, so an unopenable-algorithm file + // says so whether or not a key was supplied. Inside the match it would + // sit behind `(Some(_), None) => NoEncryptionKey`, telling the user to + // go find a key for a file that no key can open, and only revealing the + // real reason once they produced one. + if let Some(header) = &sb.encryption { + if header.algorithm != crate::superblock::ALGO_XCHACHA20POLY1305 { + return Err(ChiselError::EncryptionNotSupported); + } + } let cipher = match (&sb.encryption, &key) { (None, None) => None, (Some(_), None) => return Err(ChiselError::NoEncryptionKey), (None, Some(_)) => return Err(ChiselError::EncryptionNotSupported), (Some(header), Some(k)) => { + // CRYPTO-4 / SUPERBLOCK-RECOVERY-3: the algorithm byte was + // written but never read back. `deserialize` gates only on + // zero ("not encrypted"), so ANY nonzero value was accepted and + // fed to `PageCipher`, which is hardcoded to XChaCha20-Poly1305. + // + // The field exists precisely to prevent that. Without this check + // a file stamped `algorithm = 2` — a future cipher, or a forged + // byte — opens, and every page read then fails as + // `DecryptionFailed`, which `is_fatal()` and poisons: an + // algorithm mismatch reported to the user as data corruption. + // + // `EncryptionNotSupported` is the right variant rather than a new + // one: its doc already covers "an encrypted DB is opened by a + // build that the on-disk crypto-header algorithm id is unknown + // to". It is operational, not fatal, which is correct — the file + // is intact, this build simply cannot read it. (The check itself + // now runs just above, before the key-presence match.) // `raw` is provably the buffer the winner was deserialized from — // correct regardless of create-seed vs commit write-slot ordering. - let dek = unwrap_first_matching_slot(header, k)?; + // + // `unlock` is the same trial loop `add_key`/`rotate_key`/ + // `remove_key` use. This site used to carry a second copy + // (`unwrap_first_matching_slot`) that behaved identically but was + // separately maintained; see `unlock`'s doc for why that was + // worth collapsing even though this particular pair had not + // drifted. The slot index is discarded here — the open path only + // needs the DEK. + let (_slot, dek) = header.unlock(k)?; let cipher = crate::crypto::PageCipher::new(dek); // decrypt_body fills total_pages, named_roots, etc. // A tag failure here means corruption, not a wrong key @@ -591,59 +626,30 @@ struct CreateCrypto { } /// Build the session PageCipher for a freshly-created encrypted DB: generate a -/// random DEK + slot-0 salt, derive the KEK from the client key, wrap the DEK -/// into slot 0, and assemble the crypto-header. Returns the live PageCipher and -/// the header to stamp into every superblock slot. +/// random DEK, wrap it into slot 0 under a KEK derived from the client key, and +/// assemble the crypto-header. Returns the live PageCipher and the header to +/// stamp into every superblock slot. /// -/// The AAD passed to `wrap_dek` is `slot.aad()` — the same bytes that Task 2.4 -/// reconstructs at unwrap time from the persisted slot fields. Keeping the AAD -/// construction in one place (`KeySlot::aad`) ensures wrap and unwrap agree. +/// The wrap itself is `CryptoHeader::wrap_into`, the same function `add_key` +/// and `rotate_key` use. It used to be a second, hand-maintained implementation +/// here — and the duplication had already produced a real divergence (CRYPTO-6: +/// this copy wrote the OWASP argon2 defaults into HKDF slots, which the format +/// doc says must be zero, while `wrap_into` correctly wrote zeros). A comment +/// asserting two copies agree is not a mechanism that keeps them agreeing; one +/// function is. fn build_create_cipher( key: &crate::crypto::Key, argon2_override: Option, ) -> Result { - use crate::crypto::{ - derive_kek, random_array, random_dek, wrap_dek, Argon2Params, KdfId, NONCE_LEN, SALT_LEN, - }; use crate::superblock::{CryptoHeader, KeySlot, ALGO_XCHACHA20POLY1305, KEY_SLOT_COUNT}; - let dek = random_dek(); - let salt: [u8; SALT_LEN] = random_array(); - let wrap_nonce: [u8; NONCE_LEN] = random_array(); - - // KDF choice: Raw → HKDF (fast, key-material quality); Passphrase → Argon2id - // (memory-hard, brute-force resistant). The argon2_override is the - // caller-supplied cost params from Options::argon2_params; falls back to the - // OWASP baseline default. Raw keys use HKDF regardless, so the override only - // has effect for Passphrase. - let (kdf, params) = match key { - crate::crypto::Key::Raw(_) => (KdfId::Hkdf, Argon2Params::default()), - crate::crypto::Key::Passphrase(_) => (KdfId::Argon2id, argon2_override.unwrap_or_default()), - }; - let kek = derive_kek(key, kdf, &salt, ¶ms)?; - - // Populate slot 0: state=active, KDF metadata, the wrapped DEK. - // slot.aad() is the canonical AAD bytes; it MUST be the same value - // used by Task 2.4 to unwrap — both sides call KeySlot::aad() on - // the populated-but-pre-wrap slot so the bytes are identical. - let mut slot = KeySlot::EMPTY; - slot.state = 1; // active - slot.kdf_id = kdf as u8; - slot.argon2 = params; - slot.salt = salt; - slot.wrap_nonce = wrap_nonce; - let aad = slot.aad(); - let (wrapped, tag) = wrap_dek(&kek, &dek, &wrap_nonce, &aad); - slot.wrapped_dek = wrapped; - slot.wrap_tag = tag; - - let mut slots = [KeySlot::EMPTY; KEY_SLOT_COUNT]; - slots[0] = slot; - let header = CryptoHeader { + let dek = crate::crypto::random_dek(); + let mut header = CryptoHeader { algorithm: ALGO_XCHACHA20POLY1305, - stride: 8232, - slots, + stride: crate::crypto::ENC_PAGE_SIZE as u32, + slots: [KeySlot::EMPTY; KEY_SLOT_COUNT], }; + header.wrap_into(0, key, &dek, argon2_override)?; Ok(CreateCrypto { page_cipher: crate::crypto::PageCipher::new(dek), @@ -651,49 +657,6 @@ fn build_create_cipher( }) } -/// Try every ACTIVE key-slot in turn: derive the KEK from `key` + the slot's -/// salt/params, then attempt to unwrap the DEK. The first slot whose AEAD tag -/// verifies yields the DEK. If no slot matches, the caller's key is wrong. -/// -/// Trying every active slot (rather than a slot-index hint) is what makes -/// multi-key support possible: a DB may have the same DEK wrapped under -/// several KEKs (one per trusted key), and the caller's key matches exactly -/// one of them. -/// -/// The AAD passed to `unwrap_dek` is `slot.aad()` — the identical bytes -/// that `build_create_cipher` used at wrap time. Both sides call -/// `KeySlot::aad()` on the fully populated (but pre-wrap) slot, so the -/// bytes agree even if the slot layout changes in a future format version. -fn unwrap_first_matching_slot( - header: &crate::superblock::CryptoHeader, - key: &crate::crypto::Key, -) -> Result { - use crate::crypto::{derive_kek, unwrap_dek, KdfId}; - - for slot in header.slots.iter().filter(|s| s.is_active()) { - let kdf = match slot.kdf_id { - 1 => KdfId::Hkdf, - 2 => KdfId::Argon2id, - _ => continue, // unknown KDF id: skip, treat as non-matching - }; - let kek = match derive_kek(key, kdf, &slot.salt, &slot.argon2) { - Ok(k) => k, - Err(_) => continue, - }; - let aad = slot.aad(); - if let Ok(dek) = unwrap_dek( - &kek, - &slot.wrapped_dek, - &slot.wrap_tag, - &slot.wrap_nonce, - &aad, - ) { - return Ok(dek); - } - } - Err(ChiselError::InvalidEncryptionKey) -} - #[cfg(test)] mod tests { use super::*;