From 6497465762ec566f6d422f155d60864ca0834aff Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 23 Aug 2020 17:06:33 -0400 Subject: [PATCH] Simplify + clarify random-bytes-fetching from KeysInterface Due to a desire to be able to override temporary channel IDs and onion keys, KeysInterface had two separate fetch-random-32-bytes interfaces - an onion-key specific version which fetched 2 random 32 byte strings and a temporary-channel-id specific version. It turns out, we never actually need to override both at once (as creating a new channel and sending an outbound payment are always separate top-level calls), so there's no reason to add two functions to the interface when both really do the same thing. --- fuzz/src/chanmon_consistency.rs | 17 +++------- fuzz/src/full_stack.rs | 8 +---- lightning/src/chain/keysinterface.rs | 50 ++++++++-------------------- lightning/src/ln/channel.rs | 5 ++- lightning/src/ln/channelmanager.rs | 3 +- lightning/src/ln/functional_tests.rs | 2 +- lightning/src/util/test_utils.rs | 23 +++++++------ 7 files changed, 37 insertions(+), 71 deletions(-) diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index 9749361fd..16e8cdf65 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -142,8 +142,7 @@ impl channelmonitor::ManyChannelMonitor for TestChannelMonitor { struct KeyProvider { node_id: u8, - session_id: atomic::AtomicU8, - channel_id: atomic::AtomicU8, + rand_bytes_id: atomic::AtomicU8, } impl KeysInterface for KeyProvider { type ChanKeySigner = EnforcingChannelKeys; @@ -179,14 +178,8 @@ impl KeysInterface for KeyProvider { )) } - fn get_onion_rand(&self) -> (SecretKey, [u8; 32]) { - let id = self.session_id.fetch_add(1, atomic::Ordering::Relaxed); - (SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, id, 10, self.node_id]).unwrap(), - [0; 32]) - } - - fn get_channel_id(&self) -> [u8; 32] { - let id = self.channel_id.fetch_add(1, atomic::Ordering::Relaxed); + fn get_secure_random_bytes(&self) -> [u8; 32] { + let id = self.rand_bytes_id.fetch_add(1, atomic::Ordering::Relaxed); [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, id, 11, self.node_id] } } @@ -202,7 +195,7 @@ pub fn do_test(data: &[u8], out: Out) { let watch = Arc::new(ChainWatchInterfaceUtil::new(Network::Bitcoin)); let monitor = Arc::new(TestChannelMonitor::new(watch.clone(), broadcast.clone(), logger.clone(), fee_est.clone())); - let keys_manager = Arc::new(KeyProvider { node_id: $node_id, session_id: atomic::AtomicU8::new(0), channel_id: atomic::AtomicU8::new(0) }); + let keys_manager = Arc::new(KeyProvider { node_id: $node_id, rand_bytes_id: atomic::AtomicU8::new(0) }); let mut config = UserConfig::default(); config.channel_options.fee_proportional_millionths = 0; config.channel_options.announced_channel = true; @@ -218,7 +211,7 @@ pub fn do_test(data: &[u8], out: Out) { let watch = Arc::new(ChainWatchInterfaceUtil::new(Network::Bitcoin)); let monitor = Arc::new(TestChannelMonitor::new(watch.clone(), broadcast.clone(), logger.clone(), fee_est.clone())); - let keys_manager = Arc::new(KeyProvider { node_id: $node_id, session_id: atomic::AtomicU8::new(0), channel_id: atomic::AtomicU8::new(0) }); + let keys_manager = Arc::new(KeyProvider { node_id: $node_id, rand_bytes_id: atomic::AtomicU8::new(0) }); let mut config = UserConfig::default(); config.channel_options.fee_proportional_millionths = 0; config.channel_options.announced_channel = true; diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs index 51e399412..543357d39 100644 --- a/fuzz/src/full_stack.rs +++ b/fuzz/src/full_stack.rs @@ -295,13 +295,7 @@ impl KeysInterface for KeyProvider { }) } - fn get_onion_rand(&self) -> (SecretKey, [u8; 32]) { - let ctr = self.counter.fetch_add(1, Ordering::Relaxed) as u8; - (SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 13, ctr]).unwrap(), - [0; 32]) - } - - fn get_channel_id(&self) -> [u8; 32] { + fn get_secure_random_bytes(&self) -> [u8; 32] { let ctr = self.counter.fetch_add(1, Ordering::Relaxed); [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (ctr >> 8*7) as u8, (ctr >> 8*6) as u8, (ctr >> 8*5) as u8, (ctr >> 8*4) as u8, (ctr >> 8*3) as u8, (ctr >> 8*2) as u8, (ctr >> 8*1) as u8, 14, (ctr >> 8*0) as u8] diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs index fbc6c9bc6..e564fa7ca 100644 --- a/lightning/src/chain/keysinterface.rs +++ b/lightning/src/chain/keysinterface.rs @@ -341,12 +341,10 @@ pub trait KeysInterface: Send + Sync { /// Get a new set of ChannelKeys for per-channel secrets. These MUST be unique even if you /// restarted with some stale data! fn get_channel_keys(&self, inbound: bool, channel_value_satoshis: u64) -> Self::ChanKeySigner; - /// Get a secret and PRNG seed for constructing an onion packet - fn get_onion_rand(&self) -> (SecretKey, [u8; 32]); - /// Get a unique temporary channel id. Channels will be referred to by this until the funding - /// transaction is created, at which point they will use the outpoint in the funding - /// transaction. - fn get_channel_id(&self) -> [u8; 32]; + /// Gets a unique, cryptographically-secure, random 32 byte value. This is used for encrypting + /// onion packets and for temporary channel IDs. There is no requirement that these be + /// persisted anywhere, though they must be unique across restarts. + fn get_secure_random_bytes(&self) -> [u8; 32]; } #[derive(Clone)] @@ -666,10 +664,8 @@ pub struct KeysManager { shutdown_pubkey: PublicKey, channel_master_key: ExtendedPrivKey, channel_child_index: AtomicUsize, - session_master_key: ExtendedPrivKey, - session_child_index: AtomicUsize, - channel_id_master_key: ExtendedPrivKey, - channel_id_child_index: AtomicUsize, + rand_bytes_master_key: ExtendedPrivKey, + rand_bytes_child_index: AtomicUsize, seed: [u8; 32], starting_time_secs: u64, @@ -678,7 +674,7 @@ pub struct KeysManager { impl KeysManager { /// Constructs a KeysManager from a 32-byte seed. If the seed is in some way biased (eg your - /// RNG is busted) this may panic (but more importantly, you will possibly lose funds). + /// CSRNG is busted) this may panic (but more importantly, you will possibly lose funds). /// starting_time isn't strictly required to actually be a time, but it must absolutely, /// without a doubt, be unique to this instance. ie if you start multiple times with the same /// seed, starting_time must be unique to each run. Thus, the easiest way to achieve this is to @@ -715,8 +711,7 @@ impl KeysManager { Err(_) => panic!("Your RNG is busted"), }; let channel_master_key = master_key.ckd_priv(&secp_ctx, ChildNumber::from_hardened_idx(3).unwrap()).expect("Your RNG is busted"); - let session_master_key = master_key.ckd_priv(&secp_ctx, ChildNumber::from_hardened_idx(4).unwrap()).expect("Your RNG is busted"); - let channel_id_master_key = master_key.ckd_priv(&secp_ctx, ChildNumber::from_hardened_idx(5).unwrap()).expect("Your RNG is busted"); + let rand_bytes_master_key = master_key.ckd_priv(&secp_ctx, ChildNumber::from_hardened_idx(4).unwrap()).expect("Your RNG is busted"); KeysManager { secp_ctx, @@ -725,10 +720,8 @@ impl KeysManager { shutdown_pubkey, channel_master_key, channel_child_index: AtomicUsize::new(0), - session_master_key, - session_child_index: AtomicUsize::new(0), - channel_id_master_key, - channel_id_child_index: AtomicUsize::new(0), + rand_bytes_master_key, + rand_bytes_child_index: AtomicUsize::new(0), seed: *seed, starting_time_secs, @@ -821,29 +814,14 @@ impl KeysInterface for KeysManager { self.derive_channel_keys(channel_value_satoshis, ix_and_nanos, self.starting_time_secs) } - fn get_onion_rand(&self) -> (SecretKey, [u8; 32]) { + fn get_secure_random_bytes(&self) -> [u8; 32] { let mut sha = self.derive_unique_start(); - let child_ix = self.session_child_index.fetch_add(1, Ordering::AcqRel); - let child_privkey = self.session_master_key.ckd_priv(&self.secp_ctx, ChildNumber::from_hardened_idx(child_ix as u32).expect("key space exhausted")).expect("Your RNG is busted"); - sha.input(&child_privkey.private_key.key[..]); - - let mut rng_seed = sha.clone(); - // Not exactly the most ideal construction, but the second value will get fed into - // ChaCha so it is another step harder to break. - rng_seed.input(b"RNG Seed Salt"); - sha.input(b"Session Key Salt"); - (SecretKey::from_slice(&Sha256::from_engine(sha).into_inner()).expect("Your RNG is busted"), - Sha256::from_engine(rng_seed).into_inner()) - } - - fn get_channel_id(&self) -> [u8; 32] { - let mut sha = self.derive_unique_start(); - - let child_ix = self.channel_id_child_index.fetch_add(1, Ordering::AcqRel); - let child_privkey = self.channel_id_master_key.ckd_priv(&self.secp_ctx, ChildNumber::from_hardened_idx(child_ix as u32).expect("key space exhausted")).expect("Your RNG is busted"); + let child_ix = self.rand_bytes_child_index.fetch_add(1, Ordering::AcqRel); + let child_privkey = self.rand_bytes_master_key.ckd_priv(&self.secp_ctx, ChildNumber::from_hardened_idx(child_ix as u32).expect("key space exhausted")).expect("Your RNG is busted"); sha.input(&child_privkey.private_key.key[..]); + sha.input(b"Unique Secure Random Bytes Salt"); Sha256::from_engine(sha).into_inner() } } diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 2bea55a57..6079c47ee 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -491,7 +491,7 @@ impl Channel { user_id: user_id, config: config.channel_options.clone(), - channel_id: keys_provider.get_channel_id(), + channel_id: keys_provider.get_secure_random_bytes(), channel_state: ChannelState::OurInitSent as u32, channel_outbound: true, secp_ctx: Secp256k1::new(), @@ -4555,8 +4555,7 @@ mod tests { fn get_channel_keys(&self, _inbound: bool, _channel_value_satoshis: u64) -> InMemoryChannelKeys { self.chan_keys.clone() } - fn get_onion_rand(&self) -> (SecretKey, [u8; 32]) { panic!(); } - fn get_channel_id(&self) -> [u8; 32] { [0; 32] } + fn get_secure_random_bytes(&self) -> [u8; 32] { [0; 32] } } fn public_from_secret_hex(secp_ctx: &Secp256k1, hex: &str) -> PublicKey { diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index a17eb1a61..ccd0b0044 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1243,7 +1243,8 @@ impl // Only public for testing, this should otherwise never be called direcly pub(crate) fn send_payment_along_path(&self, path: &Vec, payment_hash: &PaymentHash, payment_secret: &Option, total_value: u64, cur_height: u32) -> Result<(), APIError> { log_trace!(self.logger, "Attempting to send payment for path with next hop {}", path.first().unwrap().short_channel_id); - let (session_priv, prng_seed) = self.keys_manager.get_onion_rand(); + let prng_seed = self.keys_manager.get_secure_random_bytes(); + let session_priv = SecretKey::from_slice(&self.keys_manager.get_secure_random_bytes()[..]).expect("RNG is busted"); let onion_keys = onion_utils::construct_onion_keys(&self.secp_ctx, &path, &session_priv) .map_err(|_| APIError::RouteError{err: "Pubkey along hop was maliciously selected"})?; diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index aaf4fa956..d21f558a6 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -6126,7 +6126,7 @@ fn test_onion_failure() { let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]); let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); for node in nodes.iter() { - *node.keys_manager.override_session_priv.lock().unwrap() = Some(SecretKey::from_slice(&[3; 32]).unwrap()); + *node.keys_manager.override_session_priv.lock().unwrap() = Some([3; 32]); } let channels = [create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()), create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known())]; let (_, payment_hash) = get_payment_preimage_hash!(nodes[0]); diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index d684e9c07..559277d2a 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -350,7 +350,7 @@ impl Logger for TestLogger { pub struct TestKeysInterface { backing: keysinterface::KeysManager, - pub override_session_priv: Mutex>, + pub override_session_priv: Mutex>, pub override_channel_id_priv: Mutex>, } @@ -364,18 +364,19 @@ impl keysinterface::KeysInterface for TestKeysInterface { EnforcingChannelKeys::new(self.backing.get_channel_keys(inbound, channel_value_satoshis)) } - fn get_onion_rand(&self) -> (SecretKey, [u8; 32]) { - match *self.override_session_priv.lock().unwrap() { - Some(key) => (key.clone(), [0; 32]), - None => self.backing.get_onion_rand() + fn get_secure_random_bytes(&self) -> [u8; 32] { + let override_channel_id = self.override_channel_id_priv.lock().unwrap(); + let override_session_key = self.override_session_priv.lock().unwrap(); + if override_channel_id.is_some() && override_session_key.is_some() { + panic!("We don't know which override key to use!"); } - } - - fn get_channel_id(&self) -> [u8; 32] { - match *self.override_channel_id_priv.lock().unwrap() { - Some(key) => key.clone(), - None => self.backing.get_channel_id() + if let Some(key) = &*override_channel_id { + return *key; + } + if let Some(key) = &*override_session_key { + return *key; } + self.backing.get_secure_random_bytes() } } -- 2.39.5