fix: collapse the duplicated key-slot wrap/unwrap and validate the algorithm byte (#117) - #154
Merged
Conversation
…gorithm byte 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #117. Also closes #103's SUPERBLOCK-RECOVERY-3, which is the same defect as CRYPTO-4 filed twice. Stacked on #153.
CRYPTO-7 / CRYPTO-6 — four hand-maintained near-copies, in the crate's highest-consequence code
unlock's doc asserted its copy was "byte-identical" tounwrap_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(used byadd_key/rotate_key)0/0/0— what the format doc promisesbuild_create_cipher(used at create)19456/2/1— the OWASP defaultsSo a database created with
Key::Rawdisagreed byte-for-byte with a second raw credential added to it later, and neither matched the documented layout. That is exactly the drift a comment claiming equivalence cannot prevent and a shared function can.open_existingnow callsheader.unlock(k);build_create_cipherbuilds an empty header and callswrap_into, which gained anargon2_overrideparameter for the create path'sOptions::argon2_params.Backward compatibility — the headline risk, affirmatively closed.
unlockrebuilds 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 carrying19456/2/1still unwraps. The adversarial review verified this end to end: it created a database with the old code, applied this patch, then opened it, read the payload, added a second credential, and reopened under both keys.CRYPTO-4 / SUPERBLOCK-RECOVERY-3 — the algorithm byte was write-only
deserializegates only on zero ("plaintext"), so any nonzero value was accepted and every page fed toPageCipher, which is hardcoded to XChaCha20-Poly1305. Because the DEK unwrap is a separate primitive that does not depend on the page algorithm, such a file opened — and then failed later asDecryptionFailed, whichis_fatal()and poisons. "This build cannot read that algorithm" was reported to the user as unrecoverable data corruption.Now rejected with
EncryptionNotSupported, whose doc already described this exact 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 and CRYPTO-8
try_intowas infallible; there is notry_into, and the stale reference removed the one hint that a length check ever guarded this. Replaced the prose-only pairing invariant with adebug_assert:read_page_unit_intofills onlystridebytes of an 8232-byte stack buffer, so a cipher installed while the stride was still 8192 reads the last 40 bytes as zero-init rather than the real tag and nonce.open_bodyreturns theZeroizingbuffer instead of.to_vec()-ing out of it, which defeated its own doc's guarantee one call later. The review then found the seal side does the same thing more often:body_plaintextbuilt root pointers,next_handleand the fullnamed_rootstable in a plainVecdropped as a temporary on every superblock write. Fixed too — the claim now holds in both directions.Review findings folded in
Verification
718 tests pass, clippy and fmt clean. Both new tests verified non-vacuous by reverting their guards.