From 6958c8fd35aa1fcec525c37c070c888946a73bf6 Mon Sep 17 00:00:00 2001 From: Christophe Pettus Date: Tue, 4 Aug 2026 18:20:32 -0700 Subject: [PATCH] fix: scrub the revoked key-slot table from every superblock slot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CRYPTO-1, the last code finding on #103. (SUPERBLOCK-RECOVERY-3 landed with #117's CRYPTO-4; SWIFT-6 cannot be fixed on this lineage and is now #155.) `rewrite_crypto_header` wrote the new key-slot table into exactly ONE superblock slot. The other N-1 kept their own state, and that state includes their copy of the table. The table is CLEARTEXT at bytes 332..1356 of every superblock page, and the per-DB DEK never changes — so a revoked credential's wrapped DEK stayed sitting in the LIVE file. The attack needs no tampering, no older file image and no rollback. An adversary holding the revoked credential and mere READ access parses the sibling slot's 128-byte record, runs derive_kek + unwrap_dek with slot.aad(), and recovers the DEK that still seals every current page and superblock body. `Chisel::open` refusing the old key is not protection when the material to bypass it is in the same file. This is distinct from the rollback exposure THEORY.md already documents, which needs a wholly older image; here the stale credential lives inside the CURRENT one. The residue cleared only when N-1 further commits happened to overwrite every sibling — for an idle database, never. `overwrite_slot_table` patches only the table region of a sibling and re-stamps the checksum, so each sibling keeps its own counter, roots and sealed body and remains a valid shadow-paging fallback. That is safe because the body's AAD (`sb_identity_aad`) covers magic, format_version, txn_counter and superblock_count — not the crypto header. The scrubs run after the target slot write and share its fsync, so a partial write can never cost the revocation itself; worst case a sibling stays un-scrubbed, which is no worse than the steady state this replaces, and the next key operation clears it. The test asserts on the bytes rather than on behaviour: it captures the old credential's wrapped_dek before revoking and then requires those 32 bytes to appear in NO key slot of NO superblock slot afterwards. With the scrub disabled it fails on superblock slot 0, key slot 0. `rotate_key` also gains the section its contract was missing. "After this returns, `old` no longer opens the database" was true and is now more true, but it is still revocation rather than cryptographic erasure: the DEK is unchanged, so anyone who captured it while `old` was valid keeps reading, and a backup taken before the revocation is still readable with `old`. Both are properties of not re-keying, not defects, and an operator reaching for this during an incident needs to know which one they are getting. NOT done here, deliberately: binding a per-header generation counter into `KeySlot::aad()`. On inspection that does not achieve what it was asked to — a slot spliced in from an older image carries its OWN generation in its own AAD input, so the AAD stays self-consistent and the unwrap still succeeds. Making it bite requires storing the authoritative generation in the SEALED body and cross-checking after decryption, which is the per-page temporal replay problem (#142) rather than this one. The all-slot scrub above closes the finding as filed, with no on-disk format change. --- src/lib.rs | 25 +++++++++++ src/recovery_tests.rs | 78 +++++++++++++++++++++++++++++++++ src/superblock/crypto_header.rs | 37 ++++++++++++++++ src/transaction/keys.rs | 36 +++++++++++++++ 4 files changed, 176 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index 5e78caf..928b04f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1060,6 +1060,27 @@ impl Chisel { /// database and `new` does. O(1) — the data key is unchanged, no page is /// re-encrypted. /// + /// # This is revocation, not cryptographic erasure + /// + /// The data-encryption key does not change, so `old` is being denied a way + /// IN — it is not being cut off from data it has already seen. Two limits + /// follow, and neither is a defect: + /// + /// * Anyone who captured the DEK while `old` was valid (a process memory + /// dump, say) keeps the ability to read every page, including pages + /// written afterwards. Recovering from a compromised DEK needs a full + /// re-encryption pass, which is not implemented — see the tracking issue + /// for bulk DEK rotation. + /// * An OLD COPY of the file — a backup taken before the revocation — is + /// still fully readable with `old`. Revocation rewrites the live file, not + /// copies of it. + /// + /// What this DOES guarantee, as of the fix for CRYPTO-1, is that no slot of + /// the live file still carries `old`'s wrapped DEK. Previously the + /// pre-revocation key-slot table survived verbatim in the sibling superblock + /// slots, so the revoked credential could recover the current DEK from the + /// current file with nothing but read access. + /// /// # Errors /// `EncryptionNotSupported` if the database has no encryption; /// `InvalidEncryptionKey` if `old` unlocks no slot; `NoFreeKeySlot` if all 8 @@ -1074,6 +1095,10 @@ impl Chisel { /// the only remaining credential. O(1) — the data key is unchanged, no /// page is re-encrypted. /// + /// Read the "This is revocation, not cryptographic erasure" section on + /// [`Chisel::rotate_key`] before relying on this for incident response: the + /// DEK is unchanged, so this closes a door rather than re-keying the data. + /// /// # Errors /// `EncryptionNotSupported` if the database has no encryption; /// `InvalidEncryptionKey` if `key` unlocks no slot; `LastKeySlot` if diff --git a/src/recovery_tests.rs b/src/recovery_tests.rs index d3524a9..614a22b 100644 --- a/src/recovery_tests.rs +++ b/src/recovery_tests.rs @@ -2397,3 +2397,81 @@ fn a_raw_key_database_writes_zero_argon2_params_as_the_format_documents() { "expected to inspect both key slots; only saw {checked}" ); } + +#[test] +fn a_revoked_credentials_wrapped_dek_survives_in_no_slot() { + // CRYPTO-1. `rewrite_crypto_header` wrote the new key-slot table into + // exactly ONE superblock slot; the other N-1 kept their own state, table + // included. Because the table is CLEARTEXT (bytes 332..1356 of every + // superblock page) and the per-DB DEK never changes, the revoked + // credential's wrapped DEK stayed sitting in the LIVE file. + // + // The attack needed no tampering, no older file image and no rollback: + // an adversary holding the revoked credential and mere READ access parses + // the sibling slot's 128-byte record, runs derive_kek + unwrap_dek with + // slot.aad(), and recovers the DEK that still seals every current page. + // `Chisel::open` refusing the old key is not protection when the material + // to bypass it is in the same file. And the residue only cleared when N-1 + // further commits happened to overwrite every sibling — for an idle + // database, never. + // + // This asserts the property directly on the bytes: after revocation, the + // old credential's wrap_tag must appear in NO slot of the file. + use crate::superblock::{CRYPTO_HEADER_OFFSET, KEY_SLOT_SIZE}; + + let tmp = NamedTempFile::new().unwrap(); + let old = || Key::Raw(Zeroizing::new(vec![0xA1u8; 32])); + let new = || Key::Raw(Zeroizing::new(vec![0xB2u8; 32])); + + let mut db = Chisel::open(tmp.path(), Options::default().encryption_key(old())).unwrap(); + db.begin().unwrap(); + db.allocate(b"sealed under a DEK that never changes") + .unwrap(); + db.commit().unwrap(); + + // Capture the old credential's wrapped DEK + tag before revoking, so we can + // look for those exact bytes afterwards. + let slot0_record = { + let mut f = fs::File::open(tmp.path()).unwrap(); + let mut unit = [0u8; crate::crypto::ENC_PAGE_SIZE]; + f.read_exact(&mut unit).unwrap(); + let base = CRYPTO_HEADER_OFFSET + 8; + let mut rec = [0u8; KEY_SLOT_SIZE]; + rec.copy_from_slice(&unit[base..base + KEY_SLOT_SIZE]); + rec + }; + // wrapped_dek is at 54..86 within the record — the bytes that must vanish. + let old_wrapped: Vec = slot0_record[54..86].to_vec(); + assert_ne!(old_wrapped, vec![0u8; 32], "fixture must have a real wrap"); + + db.rotate_key(&old(), &new()).unwrap(); + drop(db); + + // Scan EVERY superblock slot's EVERY key slot. + let mut f = fs::File::open(tmp.path()).unwrap(); + let mut unit = [0u8; crate::crypto::ENC_PAGE_SIZE]; + 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..crate::superblock::KEY_SLOT_COUNT { + let base = CRYPTO_HEADER_OFFSET + 8 + key_slot * KEY_SLOT_SIZE; + assert_ne!( + &unit[base + 54..base + 86], + old_wrapped.as_slice(), + "the revoked credential's wrapped DEK is still readable in \ + superblock slot {sb_slot}, key slot {key_slot}" + ); + } + } + + // And the documented contract still holds in both directions. + assert!( + Chisel::open(tmp.path(), Options::default().encryption_key(old())).is_err(), + "the revoked credential must not open the database" + ); + let db = Chisel::open(tmp.path(), Options::default().encryption_key(new())).unwrap(); + drop(db); +} diff --git a/src/superblock/crypto_header.rs b/src/superblock/crypto_header.rs index 4c7b7f5..0ed0b5f 100644 --- a/src/superblock/crypto_header.rs +++ b/src/superblock/crypto_header.rs @@ -164,6 +164,43 @@ impl CryptoHeader { }) } + /// Overwrite ONLY the key-slot table inside an already-serialized superblock + /// image, leaving every other field — counter, roots, sealed body — exactly + /// as it was, and re-stamp the page checksum. + /// + /// This exists for credential revocation. `rewrite_crypto_header` writes a + /// full new superblock into one slot; the other N-1 slots keep their own + /// (older, still-valid) state, and that state includes their copy of the + /// key-slot table. Since the table is CLEARTEXT and the DEK never changes, + /// a revoked credential's wrapped DEK stayed readable in a sibling slot of + /// the live file: an adversary holding the revoked key needed only read + /// access to recover the DEK that still seals every current page. No + /// tampering, no older file image, no rollback — the stale credential lived + /// inside the current file, and for an idle database it lived there forever, + /// since the residue only clears when N-1 further commits happen to + /// overwrite every sibling. + /// + /// Patching just the table is what makes this safe to apply to a sibling: + /// the body's AAD (`sb_identity_aad`) covers magic, format_version, + /// txn_counter and superblock_count — NOT the crypto header — so the + /// sibling's sealed body still authenticates afterwards, and its value as a + /// shadow-paging fallback is untouched. + /// + /// Returns false if `unit` does not currently hold a parseable encrypted + /// superblock (a torn or never-written slot), in which case there is no + /// stale table in it to scrub. + pub fn overwrite_slot_table(&self, unit: &mut [u8; PAGE_SIZE]) -> bool { + if super::Superblock::deserialize(unit).is_none() { + return false; + } + for (i, slot) in self.slots.iter().enumerate() { + let base = SLOT_TABLE_OFFSET + i * KEY_SLOT_SIZE; + slot.write_into(&mut unit[base..base + KEY_SLOT_SIZE]); + } + page::stamp_checksum(unit); + true + } + /// Count how many slots currently hold a wrapped DEK (state == active). /// Used by `remove_key` (Task 5.4) to guard against removing the last /// credential and locking the caller out of their own database. diff --git a/src/transaction/keys.rs b/src/transaction/keys.rs index 752064b..06702d3 100644 --- a/src/transaction/keys.rs +++ b/src/transaction/keys.rs @@ -113,6 +113,42 @@ impl TransactionManager { cache.io_mut().write_page_unit(inactive, &unit)?; } + // CRYPTO-1: scrub the key-slot table out of every OTHER slot too. + // + // Writing only the target slot left the PRE-revocation table intact in + // its siblings. The table is cleartext and the DEK never changes, so a + // revoked credential's wrapped DEK stayed recoverable from the LIVE file + // by anyone with read access — `Chisel::open` correctly refused the old + // key, but the bytes needed to bypass that were still sitting at offset + // 332 of a sibling slot. The public contract says the opposite: "After + // this returns, `key` no longer opens the database." + // + // Only the table region is patched (see `overwrite_slot_table`), so each + // sibling keeps its own counter, roots and sealed body and remains a + // valid shadow-paging fallback. + // + // Crash window: these writes share the target slot's fsync below, so a + // crash mid-way can leave a sibling un-scrubbed. That is no worse than + // the steady state this replaces, and the next key operation clears it. + // Ordering the scrubs after the target write means the revocation itself + // is never the thing lost to a partial write. + { + use crate::crypto::ENC_PAGE_SIZE; + let mut unit = [0u8; ENC_PAGE_SIZE]; + for slot in 0..self.superblock_count as u64 { + if slot == inactive { + continue; + } + cache.io_mut().read_page_unit_into(slot, &mut unit)?; + let mut image = [0u8; PAGE_SIZE]; + image.copy_from_slice(&unit[..PAGE_SIZE]); + if new_header.overwrite_slot_table(&mut image) { + unit[..PAGE_SIZE].copy_from_slice(&image); + cache.io_mut().write_page_unit(slot, &unit)?; + } + } + } + // Durability linearization point: the rewrite is crash-safe only after // this fsync returns. A crash before this leaves the old superblock // intact in the other slot; recovery picks it by highest txn_counter.