From 81e2dd30736939fc8d699943a462f584c1e2554f Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Sat, 24 Oct 2026 23:03:14 -0400 Subject: [PATCH] Keep a replayed RNG sequence from repeating a FROST nonce Every fill a checked draw hands out is now hashed, after the health criteria have judged the raw bytes, with a process-wide fill counter, the process id and the clock. A source that replays the same sequence from one call to the next passes every comparison inside each call, and before this it committed to the same nonce pair in every session. Nothing drawn is kept between calls. The guarantee holds within one process; across process starts only the pid and the clock separate fills, which is best effort. An RNG health failure is now its own KeepError variant instead of a stringly-typed encryption error, so callers can tell a degraded source from a bad ciphertext. --- keep-core/Cargo.toml | 2 +- keep-core/src/crypto.rs | 11 +- keep-core/src/error.rs | 12 +- keep-core/src/frost/checked_rng.rs | 230 ++++++++++++++++++++++++++--- keep-frost-net/src/node/signing.rs | 7 +- keep-mobile/src/lib.rs | 1 + keep-nip46/src/error.rs | 4 +- 7 files changed, 234 insertions(+), 33 deletions(-) diff --git a/keep-core/Cargo.toml b/keep-core/Cargo.toml index bc6b5eaa..aa4a0e32 100644 --- a/keep-core/Cargo.toml +++ b/keep-core/Cargo.toml @@ -24,7 +24,7 @@ chacha20 = { version = "0.10", features = ["xchacha"] } aes = "0.9" cbc = { version = "0.2", features = ["alloc"] } blake2 = "0.11" -sha2 = "0.11" +sha2 = { version = "0.11", features = ["zeroize"] } hkdf = "0.13" hmac = "0.13" k256 = { version = "0.13", features = ["schnorr", "ecdh"] } diff --git a/keep-core/src/crypto.rs b/keep-core/src/crypto.rs index 98686e9a..ca1a113e 100644 --- a/keep-core/src/crypto.rs +++ b/keep-core/src/crypto.rs @@ -1318,15 +1318,20 @@ mod tests { } #[test] - fn entropy_health_error_maps_to_keep_encryption_error() { + fn entropy_health_error_keeps_its_identity_as_a_keep_error() { // The RNG-failure path returns a recoverable error rather than panicking - // (#685); it surfaces as a crypto/encryption-class KeepError. + // (#685), and keeps which failure it was, so a caller can stop signing + // on a degraded source without matching on a message. for e in [ crate::entropy::EntropyHealthError::Degraded, crate::entropy::EntropyHealthError::Unavailable, ] { let err = crate::error::KeepError::from(e); - assert!(matches!(err, crate::error::KeepError::Encryption(_))); + assert!( + matches!(err, crate::error::KeepError::Entropy(kind) if kind == e), + "{err:?}" + ); + assert_eq!(err.to_string(), e.to_string()); } } diff --git a/keep-core/src/error.rs b/keep-core/src/error.rs index bd774757..3fa2a08d 100644 --- a/keep-core/src/error.rs +++ b/keep-core/src/error.rs @@ -513,6 +513,12 @@ pub enum KeepError { AuditWriteFailed(String), #[error("Encryption error: {0}")] Encryption(String), + /// The RNG behind a secret failed its health check, and what was built + /// from the draw was dropped rather than returned. Its own variant rather + /// than `Encryption`, so a caller can tell a bad source from a bad + /// ciphertext, and one that holds signing state can stop on `Degraded`. + #[error(transparent)] + Entropy(#[from] crate::entropy::EntropyHealthError), #[error("Database error: {0}")] Database(String), /// The vault's redb file is already opened (exclusive lock) by another @@ -558,12 +564,6 @@ pub enum KeepError { Other(String), } -impl From for KeepError { - fn from(e: crate::entropy::EntropyHealthError) -> Self { - KeepError::Encryption(e.to_string()) - } -} - #[allow(missing_docs)] impl KeepError { pub fn code(&self) -> Option { diff --git a/keep-core/src/frost/checked_rng.rs b/keep-core/src/frost/checked_rng.rs index e8b9af79..9e3b7a36 100644 --- a/keep-core/src/frost/checked_rng.rs +++ b/keep-core/src/frost/checked_rng.rs @@ -33,23 +33,37 @@ //! that no whole block could be formed from are refused outright instead of //! being certified by those extra draws. //! +//! What the criteria cannot catch, because the judge is built per call: a +//! source that replays the same sequence from one call to the next. Every +//! comparison inside each call passes, and without more the same nonce pair +//! would come back in every session, which is the case that recovers the +//! share. Comparing across calls would mean keeping drawn bytes for the life of +//! the process, so that case is closed differently: every fill is hashed, +//! after it has been judged, together with a process-wide fill counter, the +//! process id and the clock (see [`separate`]). What a secret is built from is +//! then never the same twice in one process, whatever the source does, and the +//! criteria still see the source's own bytes rather than the hash, which would +//! pass them all. +//! //! What it does not catch, stated here so the check is not trusted past its -//! reach. The judge is built per call, so a source that replays the same -//! sequence across calls emits the same nonce pair in every session and passes -//! every comparison made here. That is the case that recovers the share, and it -//! is the gap tracked as follow-up work; within one call, hiding and binding -//! matching does not by itself recover anything. It also cannot see a source -//! that is compromised rather than broken: bytes an attacker chose but that -//! look statistically healthy pass every criterion, and no test at the draw can -//! do better. At a site that makes a single draw the only comparison is against -//! bytes drawn after the secret, which catches a source that is still degraded -//! when probed but not one that emits a single chosen value and then recovers. +//! reach. Two copies of one process restored from a snapshot share the counter +//! and the pid, and if the source replays across them too, only the clock +//! tells their fills apart, which separates them in practice and guarantees +//! nothing. It also cannot see a source that is compromised rather than broken: +//! bytes an attacker chose but that look statistically healthy pass every +//! criterion, and no test at the draw can do better. At a site that makes a +//! single draw the only comparison is against bytes drawn after the secret, +//! which catches a source that is still degraded when probed but not one that +//! emits a single chosen value and then recovers. // The Ed25519 half is gated on keep-core's `ed25519` feature. This file is also // compiled into the enclave, which does not declare that feature. #![allow(unexpected_cfgs)] use frost_secp256k1_tr::rand_core::{impls, CryptoRng, Error as RngError, OsRng, RngCore}; +use sha2::{Digest, Sha256}; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::time::{SystemTime, UNIX_EPOCH}; use zeroize::{Zeroize, Zeroizing}; /// Why a draw could not be trusted. @@ -110,12 +124,14 @@ pub(crate) const MAX_FROZEN_POSITIONS: usize = 3; /// /// Its coverage: it caught a frozen extreme byte, and any number of them, but /// never a frozen byte of any other value. A source that fills part of a buffer -/// and leaves the rest as it was is the case that matters, because several -/// draws turn bytes straight into a scalar with no hash between: the dealer's -/// key, every polynomial coefficient, refresh and repair deltas, and the DKG -/// proof-of-knowledge nonce. Twelve frozen bytes there leave 160 bits in known -/// positions, about 2^80 work to recover, and the Hamming floor does not catch -/// it until roughly half the bytes are frozen. +/// and leaves the rest as it was is the case that matters, because what it +/// hands out carries fewer unpredictable bits than its width claims, and that +/// shortfall reaches every secret built from it: the dealer's key, every +/// polynomial coefficient, refresh and repair deltas, and the DKG +/// proof-of-knowledge nonce. Hashing each fill before it is used (see +/// [`separate`]) mixes the frozen bytes in but cannot replace them, and the +/// Hamming floor does not catch such a source until roughly half the bytes are +/// frozen. /// /// Counting positions frozen at any value, and refusing above /// [`MAX_FROZEN_POSITIONS`], fixes both. A position matches across three @@ -308,8 +324,8 @@ impl CheckedRng { // A source that fills only part of what it is asked for leaves the rest // as it was, so the same byte positions hold the same value in every // block. The pairwise checks miss that until about half the bytes are - // frozen, and several draws here take bytes straight into a scalar, - // so it is counted across every block judged. The threshold and why it + // frozen, and every secret built here inherits the shortfall, so it is + // counted across every block judged. The threshold and why it // does not latch a healthy node are on `frozen_positions`. if self.fault.is_none() && self.judged.len() >= MIN_JUDGED { let blocks = Zeroizing::new(self.judged.iter().map(|b| **b).collect::>()); @@ -320,6 +336,67 @@ impl CheckedRng { } } +/// Fills handed out by every [`CheckedRng`] in this process, so no two are +/// ever separated with the same count. Holds a count and nothing drawn. +static FILLS_HANDED_OUT: AtomicU64 = AtomicU64::new(0); + +/// Replace a judged fill with a hash of it that no other fill in this process +/// can share. +/// +/// Each 32-byte block of the output is SHA-256 over a domain tag, the fill's +/// number in this process, the process id, the clock, the block's index and +/// the whole fill as drawn. The count makes the input unique within a process +/// for as long as it runs, so a source replaying a sequence from one call to +/// the next can no longer hand frost-core the same bytes twice: frost-core +/// derives a nonce as `H3(random_bytes || signing_share)` (frost-core 3.0.0, +/// `round1.rs:77-90`), and a repeat there is what recovers the share. The pid +/// does the same for a forked child, which inherits the count. +/// +/// That guarantee holds within one process and nowhere else. A new process +/// starts the count again, so every run of a one-shot command, a restarted +/// daemon and a copy restored from a snapshot are told apart from each other +/// only by the pid and the clock. A pid is often the same on every start in a +/// container or an enclave, which leaves the clock, and that separates them in +/// practice without guaranteeing anything. +/// +/// The output is no more predictable than the fill: with SHA-256 treated as a +/// random function, a healthy fill comes out indistinguishable from uniform, +/// and the count, pid and clock add no entropy worth counting. That is why it +/// runs after the fill is judged and never before, since the hash of a stuck +/// source looks healthy to every criterion. +fn separate(dest: &mut [u8]) { + if dest.is_empty() { + return; + } + let fill = FILLS_HANDED_OUT.fetch_add(1, Ordering::Relaxed); + // A clock set before 1970 is still a clock, so it is hashed by distance + // and side rather than collapsed to one constant. + let clock = match SystemTime::now().duration_since(UNIX_EPOCH) { + Ok(after) => (1u8, after.as_nanos()), + Err(before) => (0u8, before.duration().as_nanos()), + }; + let drawn = Zeroizing::new(dest.to_vec()); + for (index, block) in dest.chunks_mut(32).enumerate() { + let mut hasher = Sha256::new(); + hasher.update(b"keep/checked-rng/separate/v1\0"); + hasher.update(fill.to_le_bytes()); + hasher.update(std::process::id().to_le_bytes()); + hasher.update([clock.0]); + hasher.update(clock.1.to_le_bytes()); + hasher.update((index as u64).to_le_bytes()); + hasher.update((drawn.len() as u64).to_le_bytes()); + hasher.update(&drawn[..]); + let mut digest = hasher.finalize(); + block.copy_from_slice(&digest[..block.len()]); + // Hygiene to the same standard as `judged`: frost-core keeps its own + // copy of these bytes unscrubbed. keep-core's SHA-256 also clears its + // buffer on drop; the enclave's sha2 0.10 cannot, so the raw fill may + // stay in that hasher's freed buffer there. + let digest: &mut [u8] = &mut digest; + digest.zeroize(); + } +} + impl Drop for CheckedRng { fn drop(&mut self) { self.judged.zeroize(); @@ -339,6 +416,7 @@ impl RngCore for CheckedRng { fn fill_bytes(&mut self, dest: &mut [u8]) { self.inner.fill_bytes(dest); self.judge(dest); + separate(dest); } /// Nothing on the nonce path reaches this: frost-core fills through @@ -348,6 +426,7 @@ impl RngCore for CheckedRng { fn try_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), RngError> { self.inner.try_fill_bytes(dest)?; self.judge(dest); + separate(dest); Ok(()) } } @@ -558,6 +637,22 @@ mod tests { OsRng.fill_bytes(dest); }); + /// Healthy-looking from block to block, and the same sequence from the + /// start every time one is made. Built fresh per call, it is a source that + /// replays itself across calls, which no comparison inside a call can see. + struct ReplayingRng(u64); + + impl_rng!(ReplayingRng, |s, dest| { + for block in dest.chunks_mut(32) { + let digest = Sha256::new() + .chain_update(b"replaying source") + .chain_update(s.0.to_le_bytes()) + .finalize(); + block.copy_from_slice(&digest[..block.len()]); + s.0 += 1; + } + }); + impl_rng!(CreepingRng, |s, dest| { dest.fill(0x5a); dest[0] = s.0; @@ -937,4 +1032,103 @@ mod tests { Some(EntropyHealthError::Degraded) ); } + /// The share-recovering case the per-call criteria cannot see: a source + /// that hands every call the same sequence. Each call passes, because + /// nothing inside it repeats, and before fills were separated both calls + /// committed to the same nonce pair, so two signatures over different + /// messages gave up the share. + #[test] + fn a_source_replaying_its_sequence_across_calls_never_repeats_a_nonce() { + let mut first_raw = [0u8; 64]; + let mut second_raw = [0u8; 64]; + ReplayingRng(0).fill_bytes(&mut first_raw); + ReplayingRng(0).fill_bytes(&mut second_raw); + assert_eq!(first_raw, second_raw, "the source must really replay"); + + let share = test_share(9); + let commit = || { + checked_draw_from(ReplayingRng(0), |rng| { + #[expect(clippy::disallowed_methods)] // drawn through the checked source + frost_secp256k1_tr::round1::commit(&share, rng) + }) + .expect("each call on its own looks healthy, so it passes") + }; + let (first, _) = commit(); + let (second, _) = commit(); + assert_ne!(first.hiding().serialize(), second.hiding().serialize()); + assert_ne!(first.binding().serialize(), second.binding().serialize()); + } + + /// The same for Ed25519, whose nonces come from the same kind of fill. + #[cfg(feature = "ed25519")] + #[test] + fn an_ed25519_source_replaying_across_calls_never_repeats_a_nonce() { + let (shares, _) = checked_draw(|rng| { + #[expect(clippy::disallowed_methods)] // drawn through the checked source + frost_ed25519::keys::generate_with_dealer( + 3, + 2, + frost_ed25519::keys::IdentifierList::Default, + rng, + ) + }) + .expect("the OS source must pass") + .expect("dealer generation must succeed"); + let secret = shares.values().next().expect("three shares"); + let commit = || { + checked_draw_from(ReplayingRng(0), |rng| { + #[expect(clippy::disallowed_methods)] // drawn through the checked source + frost_ed25519::round1::commit(secret.signing_share(), rng) + }) + .expect("each call on its own looks healthy, so it passes") + }; + let (first, _) = commit(); + let (second, _) = commit(); + assert_ne!(first.hiding().serialize(), second.hiding().serialize()); + assert_ne!(first.binding().serialize(), second.binding().serialize()); + } + + /// Two identical fills never come out the same, at any width, including + /// one with a ragged tail shorter than a block. + #[test] + fn identical_fills_are_separated_at_every_width() { + // Nothing narrower than 16: a one-byte output matches by chance once in + // 256, which would make this fail on its own about one run in a hundred. + for width in [16usize, 32, 48, 64] { + let mut first = vec![0x42u8; width]; + let mut second = first.clone(); + separate(&mut first); + separate(&mut second); + assert_ne!(first, second, "width {width}"); + assert_ne!(first, vec![0x42u8; width], "width {width}"); + } + } + + /// Every way out of the judged source is separated, not only the + /// `fill_bytes` that frost-core happens to use today. + #[test] + fn every_path_out_of_a_replaying_source_is_separated() { + // Whole blocks' worth, since narrower draws are refused as unjudged. + let via_u64 = || { + checked_draw_from(ReplayingRng(0), |rng| [(); 4].map(|()| rng.next_u64())) + .expect("a block of u64s is judged and passes") + }; + assert_ne!(via_u64(), via_u64()); + + let via_u32 = || { + checked_draw_from(ReplayingRng(0), |rng| [(); 8].map(|()| rng.next_u32())) + .expect("a block of u32s is judged and passes") + }; + assert_ne!(via_u32(), via_u32()); + + let via_try = || { + checked_draw_from(ReplayingRng(0), |rng| { + let mut block = [0u8; 32]; + rng.try_fill_bytes(&mut block).map(|()| block) + }) + .expect("a full block is judged and passes") + .expect("the source never refuses") + }; + assert_ne!(via_try(), via_try()); + } } diff --git a/keep-frost-net/src/node/signing.rs b/keep-frost-net/src/node/signing.rs index ed7ccce2..b28f74f6 100644 --- a/keep-frost-net/src/node/signing.rs +++ b/keep-frost-net/src/node/signing.rs @@ -231,10 +231,9 @@ impl KfpNode { let key_package = self.share.key_package()?; let mut fresh = Vec::with_capacity(deficit); for _ in 0..deficit { - // Drawn through the entropy module rather than the crypto wrapper - // so the health failure keeps its own type. The wrapper flattens it - // into a stringly-typed encryption error, and deciding to stop - // signing on a string match is exactly the guess this refuses to make. + // Drawn through the entropy module so the health failure reaches + // `entropy_refusal` as its own type, which is what the decision to + // stop signing branches on. let nonce_id: NonceId = keep_core::entropy::try_random_bytes::<32>() .map_err(|e| self.entropy_refusal("nonce id", e))?; let (nonces, commitment) = self.commit_nonces(key_package.signing_share())?; diff --git a/keep-mobile/src/lib.rs b/keep-mobile/src/lib.rs index 969bdc6b..85f9addf 100644 --- a/keep-mobile/src/lib.rs +++ b/keep-mobile/src/lib.rs @@ -3899,6 +3899,7 @@ fn nip46_to_mobile_error(e: keep_core::error::KeepError) -> KeepMobileError { KeepError::StorageErr(e) => KeepMobileError::StorageError { msg: e.to_string() }, KeepError::CryptoErr(e) => KeepMobileError::FrostError { msg: e.to_string() }, KeepError::Encryption(msg) => KeepMobileError::EncryptionError { msg }, + KeepError::Entropy(e) => KeepMobileError::EncryptionError { msg: e.to_string() }, KeepError::Runtime(msg) => KeepMobileError::NotSupported { msg }, other => KeepMobileError::NetworkError { msg: other.to_string(), diff --git a/keep-nip46/src/error.rs b/keep-nip46/src/error.rs index 6f590766..4e965eef 100644 --- a/keep-nip46/src/error.rs +++ b/keep-nip46/src/error.rs @@ -16,7 +16,9 @@ pub(crate) fn sanitize_error_for_client(e: &KeepError) -> &'static str { KeepError::Locked => "Signer locked", KeepError::AlreadyExists(_) | KeepError::NotFound(_) => "Resource error", KeepError::InvalidNetwork(_) => "Invalid network", - KeepError::Encryption(_) | KeepError::CryptoErr(_) => "Cryptographic operation failed", + KeepError::Encryption(_) | KeepError::Entropy(_) | KeepError::CryptoErr(_) => { + "Cryptographic operation failed" + } KeepError::Database(_) | KeepError::VaultAlreadyOpen(_) | KeepError::Migration(_)