From 7d61fa2863e06ce84aedcd97fa971d6feecda477 Mon Sep 17 00:00:00 2001 From: hellno Date: Sun, 21 Jun 2026 10:11:38 +0200 Subject: [PATCH] keystore: fail closed on OS CSPRNG failure instead of panicking (#85) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every key-derived random fill in the trust core went through the infallible `RngCore::fill_bytes` / `SliceRandom::shuffle`, which *panic* if the OS CSPRNG is unavailable. They fail closed (a panic can't yield a weak key), so this is a no-panic / fail-closed reliability fix, not a key-strength fix — but a panic in the create/seal path still aborts the process mid-onboarding with no clean error. - fill_secure()/fill_secure_with(): one entropy-acquisition chokepoint built on the fallible `try_fill_bytes`; the OS error is preserved as the source (the buffer is output-only, carries no key material) and the top line stays plain. - create/seal gain private `_with_rng` seams so a failing-RNG test can drive the new Err branch (real OsRng can't be made to fail). Public signatures unchanged; OsRng stays the only production entropy source — the seam is deliberately private so nothing can substitute a non-OsRng source into the trust core. - random_word_positions: same latent panic (shuffle -> fill_bytes). Reworked to Fisher-Yates over try_fill_bytes returning Result, with rejection sampling (bounded_index) so there is no modulo bias. Caller (shell.rs create flow) handles the error via auth_error instead of unwrapping. - clippy.toml: ban RngCore::fill_bytes and SliceRandom::shuffle so the no-panic invariant can't silently regress (enforced by `just check` -D warnings). Tests: create/seal fail closed under an injected FailingRng; random_word_positions stays distinct/sorted/in-range and caps k at word_count. --- clippy.toml | 2 + crates/deckard-app/src/shell.rs | 17 ++-- crates/deckard-core/src/keystore.rs | 150 ++++++++++++++++++++++++++-- 3 files changed, 152 insertions(+), 17 deletions(-) diff --git a/clippy.toml b/clippy.toml index 3b39c62..fa872bf 100644 --- a/clippy.toml +++ b/clippy.toml @@ -32,4 +32,6 @@ disallowed-methods = [ { path = "std::mem::forget", reason = "skips Drop, so skips zeroize scrubbing of key material; use drop()" }, { path = "core::mem::forget", reason = "same as std::mem::forget (re-export) — skips zeroize scrubbing; use drop()" }, { path = "rand::thread_rng", reason = "use OsRng for anything key-derived (keystore.rs already does)" }, + { path = "rand::RngCore::fill_bytes", reason = "panics if the OS CSPRNG fails; in deckard-core use keystore::fill_secure() (try_fill_bytes) and propagate the error" }, + { path = "rand::seq::SliceRandom::shuffle", reason = "calls fill_bytes internally (latent panic on CSPRNG failure); use a try_fill_bytes-based selection instead" }, ] diff --git a/crates/deckard-app/src/shell.rs b/crates/deckard-app/src/shell.rs index 4675efe..19558cd 100644 --- a/crates/deckard-app/src/shell.rs +++ b/crates/deckard-app/src/shell.rs @@ -780,12 +780,17 @@ impl Shell { match res { Ok((vault, phrase, pass)) => { let wc = phrase.split_whitespace().count(); - this.confirm_positions = deckard_core::random_word_positions(wc, 3); - this.pending_vault = Some(vault); - this.pending_phrase = Some(phrase); - this.pending_pass = Some(pass); - this.reveal_seed = false; - this.auth = AuthStep::CreateBackup; + match deckard_core::random_word_positions(wc, 3) { + Ok(positions) => { + this.confirm_positions = positions; + this.pending_vault = Some(vault); + this.pending_phrase = Some(phrase); + this.pending_pass = Some(pass); + this.reveal_seed = false; + this.auth = AuthStep::CreateBackup; + } + Err(e) => this.auth_error = Some(short_err(e)), + } } Err(e) => this.auth_error = Some(short_err(e)), } diff --git a/crates/deckard-core/src/keystore.rs b/crates/deckard-core/src/keystore.rs index 0de1f0a..d6ce0e6 100644 --- a/crates/deckard-core/src/keystore.rs +++ b/crates/deckard-core/src/keystore.rs @@ -9,6 +9,7 @@ use std::path::Path; +use anyhow::Context; use argon2::{Algorithm, Argon2, Params, Version}; use chacha20poly1305::aead::{Aead, KeyInit, Payload}; use chacha20poly1305::{Key, XChaCha20Poly1305, XNonce}; @@ -178,13 +179,26 @@ impl Vault { passphrase: &str, words: WordCount, kdf: KdfParams, + ) -> anyhow::Result<(Vault, Zeroizing)> { + Self::create_with_rng(&mut OsRng, passphrase, words, kdf) + } + + /// Inner of [`Vault::create`], generic over the RNG. Production passes `OsRng`; tests inject + /// a failing RNG to exercise the no-entropy error path. Kept private on purpose: the trust + /// core must only ever draw key material from the OS CSPRNG, so we never expose a way to + /// substitute a non-`OsRng` source. + fn create_with_rng( + rng: &mut R, + passphrase: &str, + words: WordCount, + kdf: KdfParams, ) -> anyhow::Result<(Vault, Zeroizing)> { let n = match words { WordCount::Twelve => 16, WordCount::TwentyFour => 32, }; let mut entropy = Zeroizing::new(vec![0u8; n]); - OsRng.fill_bytes(&mut entropy); + fill_secure_with(rng, &mut entropy)?; let phrase = entropy_to_phrase(&entropy)?; let kind = if n == 16 { @@ -192,7 +206,7 @@ impl Vault { } else { SecretKind::Entropy32 }; - let vault = Self::seal(kind, &entropy, passphrase, kdf)?; + let vault = Self::seal_with_rng(rng, kind, &entropy, passphrase, kdf)?; Ok((vault, phrase)) } @@ -231,6 +245,22 @@ impl Vault { secret: &[u8], passphrase: &str, kdf: KdfParams, + ) -> anyhow::Result { + Self::seal_with_rng(&mut OsRng, kind, secret, passphrase, kdf) + } + + /// Inner of [`Vault::seal`], generic over the RNG (see [`Vault::create_with_rng`] for why it + /// stays private). `try_fill_bytes` leaves a buffer in an unspecified state on error, but + /// every fill aborts the seal on `Err` before any `Vault` is constructed, and the only secret + /// buffers (`dek` here, and `entropy` in `create_with_rng`) are `Zeroizing`, so a partial fill + /// is scrubbed on the early return. `vault_id`/`salt`/`wrap_nonce`/`entropy_nonce` are public + /// header fields (stored in cleartext), so a partial fill of them leaks nothing. + fn seal_with_rng( + rng: &mut R, + kind: SecretKind, + secret: &[u8], + passphrase: &str, + kdf: KdfParams, ) -> anyhow::Result { kdf.validate()?; @@ -239,11 +269,11 @@ impl Vault { let mut wrap_nonce = [0u8; NONCE_LEN]; let mut entropy_nonce = [0u8; NONCE_LEN]; let mut dek = Zeroizing::new([0u8; DEK_LEN]); - OsRng.fill_bytes(&mut vault_id); - OsRng.fill_bytes(&mut salt); - OsRng.fill_bytes(&mut wrap_nonce); - OsRng.fill_bytes(&mut entropy_nonce); - OsRng.fill_bytes(dek.as_mut_slice()); + fill_secure_with(rng, &mut vault_id)?; + fill_secure_with(rng, &mut salt)?; + fill_secure_with(rng, &mut wrap_nonce)?; + fill_secure_with(rng, &mut entropy_nonce)?; + fill_secure_with(rng, dek.as_mut_slice())?; let header = Header { secret_kind: kind, @@ -572,15 +602,53 @@ fn unlock_failed() -> anyhow::Error { anyhow::anyhow!("could not unlock — wrong passphrase, or the vault was tampered with") } +/// Fill `buf` with cryptographically secure bytes from the OS CSPRNG, or fail. +/// The single entropy-acquisition chokepoint for the trust core: production code routes every +/// random fill through here (or [`fill_secure_with`]) rather than `RngCore::fill_bytes` / +/// `SliceRandom::shuffle`, which panic on CSPRNG failure and are banned in `clippy.toml`. +fn fill_secure(buf: &mut [u8]) -> anyhow::Result<()> { + fill_secure_with(&mut OsRng, buf) +} + +/// [`fill_secure`] generic over the RNG, so tests can inject a failing RNG to exercise the +/// error path. Production only ever passes `OsRng`. The OS error is preserved as the source +/// (it carries no key material — the buffer is output-only); the top-line message stays plain. +fn fill_secure_with(rng: &mut R, buf: &mut [u8]) -> anyhow::Result<()> { + rng.try_fill_bytes(buf) + .context("could not gather secure random bytes from the OS") +} + /// Pick `k` distinct word positions (0-indexed, sorted) to quiz during backup /// confirmation, chosen with the OS CSPRNG so a user can't pre-learn which to write. -pub fn random_word_positions(word_count: usize, k: usize) -> Vec { - use rand::seq::SliceRandom; +/// Returns an error if the OS CSPRNG is unavailable (fails closed rather than panicking). +pub fn random_word_positions(word_count: usize, k: usize) -> anyhow::Result> { + // Fisher–Yates over fallible OS entropy (rand 0.8 has no fallible `shuffle`), with + // rejection sampling so there's no modulo bias — negligible at word_count <= 24, but we + // avoid modelling a biased `% n` pattern that could be copied somewhere it matters. let mut idx: Vec = (0..word_count).collect(); - idx.shuffle(&mut OsRng); + for i in (1..word_count).rev() { + let j = bounded_index(i + 1)?; + idx.swap(i, j); + } let mut chosen: Vec = idx.into_iter().take(k.min(word_count)).collect(); chosen.sort_unstable(); - chosen + Ok(chosen) +} + +/// Uniform random index in `0..n` (`n >= 1`) from the OS CSPRNG, rejection-sampled to remove +/// modulo bias. Used only for small `n` (word positions), so the reject rate is ~0. +fn bounded_index(n: usize) -> anyhow::Result { + let n = n as u32; + // Reject the final partial bucket so every residue class is equally likely. + let zone = u32::MAX - (u32::MAX % n); + loop { + let mut b = [0u8; 4]; + fill_secure(&mut b)?; + let v = u32::from_le_bytes(b); + if v < zone { + return Ok((v % n) as usize); + } + } } /// BIP-39 entropy bytes → mnemonic phrase (in a zeroizing buffer). @@ -656,6 +724,66 @@ mod tests { const PW: &str = "correct horse battery staple"; + /// An RNG that always fails its fallible API — stands in for an unavailable OS CSPRNG so we + /// can prove create/seal fail closed (return `Err`, never a `Vault`). Real `OsRng` can't be + /// made to fail on demand, so this private seam is the only way to exercise that path. + struct FailingRng; + impl RngCore for FailingRng { + fn next_u32(&mut self) -> u32 { + 0 + } + fn next_u64(&mut self) -> u64 { + 0 + } + fn fill_bytes(&mut self, _dest: &mut [u8]) { + // Production code must use the fallible `try_fill_bytes`; reaching the infallible + // path is a bug, so fail the test loudly. + panic!("FailingRng: production code must use try_fill_bytes, not fill_bytes"); + } + fn try_fill_bytes(&mut self, _dest: &mut [u8]) -> Result<(), rand::Error> { + Err(rand::Error::from( + core::num::NonZeroU32::new(0xDEAD_BEEF).unwrap(), + )) + } + } + + #[test] + fn create_fails_closed_when_entropy_unavailable() { + // The new fallible RNG path: an OS CSPRNG failure surfaces as an error and never yields + // a Vault — so the caller has nothing to write to disk. + let r = + Vault::create_with_rng(&mut FailingRng, PW, WordCount::Twelve, KdfParams::FAST_TEST); + assert!(r.is_err()); + } + + #[test] + fn seal_fails_closed_when_entropy_unavailable() { + let r = Vault::seal_with_rng( + &mut FailingRng, + SecretKind::Entropy16, + &[0u8; 16], + PW, + KdfParams::FAST_TEST, + ); + assert!(r.is_err()); + } + + #[test] + fn random_word_positions_distinct_sorted_in_range() { + let pos = random_word_positions(12, 3).unwrap(); + assert_eq!(pos.len(), 3); + let mut dedup = pos.clone(); + dedup.dedup(); + assert_eq!(dedup.len(), 3, "positions must be distinct"); + assert!( + pos.windows(2).all(|w| w[0] < w[1]), + "positions must be sorted ascending" + ); + assert!(pos.iter().all(|&p| p < 12), "positions must be in range"); + // k is capped at word_count. + assert_eq!(random_word_positions(2, 5).unwrap().len(), 2); + } + #[test] fn create_unlock_round_trip_and_stable_address() { let (vault, phrase) = Vault::create(PW, WordCount::Twelve, KdfParams::FAST_TEST).unwrap();