From 3c0a9a95d9f1a73c04fb739c12c6e6335747e75d Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Thu, 29 Jul 2021 23:00:11 -0500 Subject: [PATCH] f - Check KeysInterface::get_shutdown_scriptpubkey Check that the user-provided shutdown_scriptpubkey is compatible with the counterparty's features (option_shutdown_anysegwit). TODO: Implement for ChannelManager::close_channel TODO: Add unit tests --- lightning/src/ln/channel.rs | 70 ++++++++++++++++++---------- lightning/src/ln/channelmanager.rs | 7 ++- lightning/src/ln/functional_tests.rs | 2 +- 3 files changed, 52 insertions(+), 27 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index b22f3756b..cfd0bec3d 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -567,7 +567,7 @@ impl Channel { } // Constructors: - pub fn new_outbound(fee_estimator: &F, keys_provider: &K, counterparty_node_id: PublicKey, channel_value_satoshis: u64, push_msat: u64, user_id: u64, config: &UserConfig) -> Result, APIError> + pub fn new_outbound(fee_estimator: &F, keys_provider: &K, counterparty_node_id: PublicKey, their_features: InitFeatures, channel_value_satoshis: u64, push_msat: u64, user_id: u64, config: &UserConfig) -> Result, APIError> where K::Target: KeysInterface, F::Target: FeeEstimator, { @@ -597,9 +597,13 @@ impl Channel { let shutdown_scriptpubkey = if config.channel_options.commit_upfront_shutdown_pubkey { Some(keys_provider.get_shutdown_scriptpubkey()) - } else { - None - }; + } else { None }; + + if let Some(shutdown_scriptpubkey) = &shutdown_scriptpubkey { + if !shutdown_scriptpubkey.is_compatible(&their_features) { + return Err(APIError::APIMisuseError { err: format!("Provided a scriptpubkey format not accepted by peer. script: ({})", shutdown_scriptpubkey.clone().into_inner().to_bytes().to_hex()) }); + } + } Ok(Channel { user_id, @@ -843,14 +847,18 @@ impl Channel { } } else { None }; - let mut secp_ctx = Secp256k1::new(); - secp_ctx.seeded_randomize(&keys_provider.get_secure_random_bytes()); - let shutdown_scriptpubkey = if config.channel_options.commit_upfront_shutdown_pubkey { Some(keys_provider.get_shutdown_scriptpubkey()) - } else { - None - }; + } else { None }; + + if let Some(shutdown_scriptpubkey) = &shutdown_scriptpubkey { + if !shutdown_scriptpubkey.is_compatible(&their_features) { + return Err(ChannelError::Close(format!("Provided a scriptpubkey format not accepted by peer. script: ({})", shutdown_scriptpubkey.clone().into_inner().to_bytes().to_hex()))); + } + } + + let mut secp_ctx = Secp256k1::new(); + secp_ctx.seeded_randomize(&keys_provider.get_secure_random_bytes()); let chan = Channel { user_id, @@ -3262,6 +3270,23 @@ impl Channel { self.counterparty_shutdown_scriptpubkey = Some(shutdown_scriptpubkey); } + // If we have any LocalAnnounced updates we'll probably just get back a update_fail_htlc + // immediately after the commitment dance, but we can send a Shutdown cause we won't send + // any further commitment updates after we set LocalShutdownSent. + let send_shutdown = (self.channel_state & ChannelState::LocalShutdownSent as u32) != ChannelState::LocalShutdownSent as u32; + + let shutdown_scriptpubkey = match self.shutdown_scriptpubkey { + Some(_) => None, + None => { + assert!(send_shutdown); + let shutdown_scriptpubkey = keys_provider.get_shutdown_scriptpubkey(); + if !shutdown_scriptpubkey.is_compatible(their_features) { + return Err(ChannelError::Close(format!("Provided a scriptpubkey format not accepted by peer. script: ({})", shutdown_scriptpubkey.clone().into_inner().to_bytes().to_hex()))); + } + Some(shutdown_scriptpubkey) + }, + }; + // From here on out, we may not fail! self.channel_state |= ChannelState::RemoteShutdownSent as u32; @@ -3282,15 +3307,9 @@ impl Channel { } }); - // If we have any LocalAnnounced updates we'll probably just get back a update_fail_htlc - // immediately after the commitment dance, but we can send a Shutdown cause we won't send - // any further commitment updates after we set LocalShutdownSent. - let send_shutdown = (self.channel_state & ChannelState::LocalShutdownSent as u32) != ChannelState::LocalShutdownSent as u32; - let monitor_update = match self.shutdown_scriptpubkey { - Some(_) => None, - None => { - assert!(send_shutdown); - self.shutdown_scriptpubkey = Some(keys_provider.get_shutdown_scriptpubkey()); + let monitor_update = match shutdown_scriptpubkey { + Some(shutdown_scriptpubkey) => { + self.shutdown_scriptpubkey = Some(shutdown_scriptpubkey); self.latest_monitor_update_id += 1; Some(ChannelMonitorUpdate { update_id: self.latest_monitor_update_id, @@ -3299,6 +3318,7 @@ impl Channel { }], }) }, + None => None, }; let shutdown = if send_shutdown { Some(msgs::Shutdown { @@ -5212,7 +5232,7 @@ mod tests { let node_a_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); let config = UserConfig::default(); - let node_a_chan = Channel::::new_outbound(&&fee_est, &&keys_provider, node_a_node_id, 10000000, 100000, 42, &config).unwrap(); + let node_a_chan = Channel::::new_outbound(&&fee_est, &&keys_provider, node_a_node_id, InitFeatures::known(), 10000000, 100000, 42, &config).unwrap(); // Now change the fee so we can check that the fee in the open_channel message is the // same as the old fee. @@ -5237,7 +5257,7 @@ mod tests { // Create Node A's channel pointing to Node B's pubkey let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); let config = UserConfig::default(); - let mut node_a_chan = Channel::::new_outbound(&&feeest, &&keys_provider, node_b_node_id, 10000000, 100000, 42, &config).unwrap(); + let mut node_a_chan = Channel::::new_outbound(&&feeest, &&keys_provider, node_b_node_id, InitFeatures::known(), 10000000, 100000, 42, &config).unwrap(); // Create Node B's channel by receiving Node A's open_channel message // Make sure A's dust limit is as we expect. @@ -5304,7 +5324,7 @@ mod tests { let node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); let config = UserConfig::default(); - let mut chan = Channel::::new_outbound(&&fee_est, &&keys_provider, node_id, 10000000, 100000, 42, &config).unwrap(); + let mut chan = Channel::::new_outbound(&&fee_est, &&keys_provider, node_id, InitFeatures::known(), 10000000, 100000, 42, &config).unwrap(); let commitment_tx_fee_0_htlcs = chan.commit_tx_fee_msat(0); let commitment_tx_fee_1_htlc = chan.commit_tx_fee_msat(1); @@ -5353,7 +5373,7 @@ mod tests { // Create Node A's channel pointing to Node B's pubkey let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); let config = UserConfig::default(); - let mut node_a_chan = Channel::::new_outbound(&&feeest, &&keys_provider, node_b_node_id, 10000000, 100000, 42, &config).unwrap(); + let mut node_a_chan = Channel::::new_outbound(&&feeest, &&keys_provider, node_b_node_id, InitFeatures::known(), 10000000, 100000, 42, &config).unwrap(); // Create Node B's channel by receiving Node A's open_channel message let open_channel_msg = node_a_chan.get_open_channel(chain_hash); @@ -5415,7 +5435,7 @@ mod tests { // Create a channel. let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); let config = UserConfig::default(); - let mut node_a_chan = Channel::::new_outbound(&&feeest, &&keys_provider, node_b_node_id, 10000000, 100000, 42, &config).unwrap(); + let mut node_a_chan = Channel::::new_outbound(&&feeest, &&keys_provider, node_b_node_id, InitFeatures::known(), 10000000, 100000, 42, &config).unwrap(); assert!(node_a_chan.counterparty_forwarding_info.is_none()); assert_eq!(node_a_chan.holder_htlc_minimum_msat, 1); // the default assert!(node_a_chan.counterparty_forwarding_info().is_none()); @@ -5479,7 +5499,7 @@ mod tests { let counterparty_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); let mut config = UserConfig::default(); config.channel_options.announced_channel = false; - let mut chan = Channel::::new_outbound(&&feeest, &&keys_provider, counterparty_node_id, 10_000_000, 100000, 42, &config).unwrap(); // Nothing uses their network key in this test + let mut chan = Channel::::new_outbound(&&feeest, &&keys_provider, counterparty_node_id, InitFeatures::known(), 10_000_000, 100000, 42, &config).unwrap(); // Nothing uses their network key in this test chan.holder_dust_limit_satoshis = 546; chan.counterparty_selected_channel_reserve_satoshis = Some(0); // Filled in in accept_channel diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 0ce337f60..971b4bc22 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1156,8 +1156,13 @@ impl ChannelMana return Err(APIError::APIMisuseError { err: format!("Channel value must be at least 1000 satoshis. It was {}", channel_value_satoshis) }); } + let their_features = { + let per_peer_state = self.per_peer_state.read().unwrap(); + let peer_state = per_peer_state.get(&their_network_key).unwrap().lock().unwrap(); + peer_state.latest_features.clone() + }; let config = if override_config.is_some() { override_config.as_ref().unwrap() } else { &self.default_configuration }; - let channel = Channel::new_outbound(&self.fee_estimator, &self.keys_manager, their_network_key, channel_value_satoshis, push_msat, user_id, config)?; + let channel = Channel::new_outbound(&self.fee_estimator, &self.keys_manager, their_network_key, their_features, channel_value_satoshis, push_msat, user_id, config)?; let res = channel.get_open_channel(self.genesis_hash.clone()); let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 21f871fa8..32eea83b3 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -7749,7 +7749,7 @@ fn test_user_configurable_csv_delay() { let nodes = create_network(2, &node_cfgs, &node_chanmgrs); // We test config.our_to_self > BREAKDOWN_TIMEOUT is enforced in Channel::new_outbound() - if let Err(error) = Channel::new_outbound(&&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }, &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), 1000000, 1000000, 0, &low_our_to_self_config) { + if let Err(error) = Channel::new_outbound(&&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }, &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), InitFeatures::known(), 1000000, 1000000, 0, &low_our_to_self_config) { match error { APIError::APIMisuseError { err } => { assert!(regex::Regex::new(r"Configured with an unreasonable our_to_self_delay \(\d+\) putting user funds at risks").unwrap().is_match(err.as_str())); }, _ => panic!("Unexpected event"), -- 2.39.5