Cleanups in the on-disk encryption

[Xof]

This issue groups 5 related findings.

CRYPTO-4 — The crypto-header algorithm byte is written but never validated; any nonzero value is treated as XChaCha20-Poly1305

Location: src/superblock/crypto_header.rs:146, src/transaction/recovery.rs:265, src/transaction/recovery.rs:612 · Severity: SMELL · Category: correctness

What the code does. CryptoHeader::deserialize gates only on zero: let algorithm = buf[CRYPTO_HEADER_OFFSET]; if algorithm == 0 { return None; } (crypto_header.rs:146-149) and then returns Some(CryptoHeader { algorithm, ... }) for any other value. The declared contract is narrower: "Algorithm id stored in the header. 0 means "no encryption" ... the only supported nonzero value today is 1 = XChaCha20-Poly1305" (crypto_header.rs:43-44). open_existing validates the sibling field — if stride != crate::crypto::ENC_PAGE_SIZE { return Err(ChiselError::CorruptSuperblock ... ) } (recovery.rs:265-269) — but never compares algorithm against ALGO_XCHACHA20POLY1305. A grep of the whole tree shows ALGO_XCHACHA20POLY1305 appears only at write sites (recovery.rs:613, plus tests); it is never read back for comparison.

Why it is a problem. A file stamped algorithm = 2 (a future AES-GCM or a corrupted/forged byte) is accepted as encrypted and every page is fed to PageCipher::open, which is hardcoded to XChaCha20-Poly1305. If a future format ever uses a second algorithm id, today's binary will not reject those files with a clean UnsupportedFormatVersion; it will attempt XChaCha20 decryption and surface DecryptionFailed, which is_fatal() (error.rs:226) — poisoning the handle and presenting an algorithm-mismatch as data corruption. The field exists precisely to prevent this and is dead on the read path.

Direction of a fix. In open_existing, next to the existing stride check, reject header.algorithm != ALGO_XCHACHA20POLY1305 with a typed error (UnsupportedFormatVersion or a dedicated variant), before any PageCipher is constructed.

CRYPTO-6 — The documented "argon2 params are zero for HKDF" slot invariant is violated by the create path, which writes the OWASP defaults into HKDF slots

Location: src/transaction/recovery.rs:590, src/superblock/crypto_header.rs:19, src/superblock/crypto_header.rs:244 · Severity: SMELL · Category: docs-vs-reality · Status: NEW

What the code does. The on-disk record layout is documented twice, identically: "2..14 argon2 params: m_cost(u32) | t_cost(u32) | p_cost(u32) (zero for HKDF)" (crypto_header.rs:19) and ARCHITECTURE.md:663 "2..14 | argon2 params: m_cost(u32) | t_cost(u32) | p_cost(u32) (zero for HKDF)". wrap_into honors it: crate::crypto::Key::Raw(_) => (KdfId::Hkdf, Argon2Params { m_cost: 0, t_cost: 0, p_cost: 0 }) (crypto_header.rs:244-251). build_create_cipher does not: crate::crypto::Key::Raw(_) => (KdfId::Hkdf, Argon2Params::default()) (recovery.rs:590), and slot.argon2 = params; (recovery.rs:602) then stamps 19456/2/1 into a slot whose kdf_id is HKDF.

Why it is a problem. A database created with Key::Raw has slot 0 carrying m=19456,t=2,p=1 with kdf_id=1, while a second raw credential added later via add_key lands in a slot carrying zeros with the same kdf_id — the two paths produce different bytes for identical semantics, and neither matches what the format doc promises. Any external tool, forensic script, or future validator that enforces the documented "zero for HKDF" rule (a reasonable integrity check, since the field is meaningless for HKDF) will reject every raw-key database Chisel has ever created. Because KeySlot::aad() covers these bytes, the discrepancy is inert for unwrap today — which is exactly why it will stay unnoticed until something depends on it.

Direction of a fix. Make build_create_cipher write zeros for the HKDF branch (or make wrap_into write the defaults) so the two paths agree, and pick whichever the doc states. Better: delete the duplication by having build_create_cipher call CryptoHeader::wrap_into — see CRYPTO-7.

CRYPTO-7 — The slot wrap and slot unwrap-trial logic each exist twice, and the doc asserting the two unwrap copies are "byte-identical" is unverified — the wrap copies already diverge

Location: src/superblock/crypto_header.rs:185, src/superblock/crypto_header.rs:192, src/transaction/recovery.rs:637, src/transaction/recovery.rs:571 · Severity: SMELL · Category: idiomaticity

What the code does. CryptoHeader::unlock (crypto_header.rs:192-224) and the free function unwrap_first_matching_slot (recovery.rs:637-665) implement the same trial loop. unlock's doc even says so: "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" (crypto_header.rs:185-187). They are not textually identical — unlock maps kdf ids via x if x == KdfId::Hkdf as u8 (crypto_header.rs:205-207) while unwrap_first_matching_slot hardcodes 1 => / 2 => (recovery.rs:645-646). The wrap side is likewise duplicated: CryptoHeader::wrap_into (crypto_header.rs:236-276) and build_create_cipher (recovery.rs:571-622) both derive a KEK, populate a slot, and call wrap_dek. Only the open path uses unwrap_first_matching_slot (recovery.rs:342); only the key-management path uses unlock (keys.rs:146, 176, 209).

Why it is a problem. The duplication has already produced a real divergence — the argon2-params discrepancy in CRYPTO-6 exists precisely because build_create_cipher and wrap_into are separate implementations of the same operation. A comment asserting equivalence is not a mechanism that enforces it: any future change to the AAD layout, the kdf-id mapping, or the slot-skipping policy must now be made in two places on each side, with nothing failing if only one is updated. This is the highest-consequence code in the crate to have four hand-maintained near-copies of.

Direction of a fix. Delete unwrap_first_matching_slot and have open_existing call header.unlock(k) (discarding the index); delete the inline wrap in build_create_cipher and have it construct an empty CryptoHeader and call wrap_into(0, key, &dek), threading the argon2_override through a parameter on wrap_into. That also closes CRYPTO-6 and gives add_key/rotate_key the cost-parameter control they currently lack.

CRYPTO-9 — Cold-load comment describes a try_into that no longer exists, and the equality it claims (stride == ENC_PAGE_SIZE) is asserted in prose but checked nowhere

Location: src/page_cache.rs:1023 · Severity: SMELL · Category: comment-accuracy · Status: NEW

What the code does. page_cache.rs:1022-1027 reads: "// The on-disk unit is exactly ENC_PAGE_SIZE at this stride; // the try_into is infallible (ENC_PAGE_SIZE == 8232 == stride). let unit: [u8; ENC_PAGE_SIZE] = on_disk;". There is no try_into — the line is a plain move of a Copy array. The invariant it names (cipher present ⟹ self.io.stride() == ENC_PAGE_SIZE) is stated only in prose here and in set_cipher's doc, "The caller MUST have already called self.io_mut().set_stride(ENC_PAGE_SIZE)" (page_cache.rs:900-902); nothing in set_cipher (page_cache.rs:902-904) or on this path checks it. Three lines above, read_page_unit_into(page_id, &mut on_disk[..stride]) (page_cache.rs:1017-1019) fills only stride bytes of the 8232-byte stack buffer.

Why it is a problem. If the pairing invariant is ever broken by a refactor — cipher installed while stride is still 8192 — the last 40 bytes of unit are the stack buffer's zero-initialization rather than the real tag/nonce, PageCipher::open fails, and the caller sees ChiselError::DecryptionFailed { page_id }, which is_fatal() (error.rs:226) and poisons the handle. A configuration mistake would be reported to the user as unrecoverable ciphertext corruption. The stale try_into reference removes the one hint a reader has that a length check ever guarded this.

Direction of a fix. Delete the try_into sentence and replace the prose invariant with a debug_assert_eq!(stride, ENC_PAGE_SIZE) on the cipher branch (or fold the stride switch into set_cipher so the two cannot be set independently).

CRYPTO-8 — open_body strips the Zeroizing wrapper the layer below it deliberately added, so decrypted superblock plaintext is handed to the allocator unscrubbed

Location: src/crypto/mod.rs:382, src/crypto/mod.rs:224, src/superblock/mod.rs:456 · Severity: NIT · Category: security · Status: NEW

What the code does. open_detached documents its return type as a security measure: "The returned buffer is Zeroizing so the decrypted plaintext (key material) is wiped on drop rather than handed back to the allocator un-scrubbed" (crypto/mod.rs:224-226), and returns Result<Zeroizing<Vec<u8>>, CryptoError>. PageCipher::open_body immediately undoes it: open_detached(self.dek.as_bytes(), nonce, aad, ct, tag).map(|z| z.to_vec()) (crypto/mod.rs:382) — to_vec() allocates a fresh non-zeroizing Vec and copies the plaintext into it; the Zeroizing original is dropped and wiped, the copy is not. The consumer keeps it alive: let body = cipher.open_body(&aad, &nonce, &tag, ct)?; self.load_body(&body); (superblock/mod.rs:456-457), where body is a plain Vec<u8> freed unscrubbed at end of scope.

Why it is a problem. Every Chisel::open of an encrypted database leaves a 300-byte heap allocation containing the decrypted superblock body — root page pointers, total_pages, next_handle, freemap_depth, and the full named_roots table (user-chosen names, which THEORY.md:160 explicitly identifies as "real user data" and the stated reason the body is encrypted at all) — in freed, un-wiped heap memory for the remaining lifetime of the process. That is the exact failure mode the Zeroizing return type was introduced to prevent, defeated one call later. It also makes the open_detached comment false for the only variable-length caller.

Direction of a fix. Change open_body's signature to return Zeroizing<Vec<u8>> (or take a &mut output buffer the caller owns as Zeroizing) and let decrypt_body bind it as such — load_body already takes &[u8], so it needs no change. If the wrapper genuinely is not wanted here, delete the claim from open_detached's doc instead of leaving both.

---

_{Filed from the clean-slate deep review of 2026-07-29. Full context, verification notes, and the delta against ISSUES.md are in docs/reviews/review-20260729-183138.md. Baseline at review time: 681 tests passing, clippy and fmt clean — none of these are toolchain-visible.}