From d9c03f26f9d52ca96bc889eae64087a690ff1a22 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 24 Nov 2020 12:40:11 -0500 Subject: [PATCH] Move UTXO-lookup into pub utility function from RoutingMsgHandler This makes the public utility methods in `NetworkGraph` able to do UTXO lookups ala `NetworkMsgHandler`'s `RoutingMessageHandler` implementation, slightly simplifying the public interface. We also take this opportunity to verify signatures before calling out to UTXO lookups, under the "do actions in order of cheapest-to-most-expensive to reduce DoS surface" principle. --- fuzz/src/router.rs | 5 +- lightning/src/routing/network_graph.rs | 81 ++++++++++++-------------- 2 files changed, 39 insertions(+), 47 deletions(-) diff --git a/fuzz/src/router.rs b/fuzz/src/router.rs index ef7669f1e..95abf9718 100644 --- a/fuzz/src/router.rs +++ b/fuzz/src/router.rs @@ -175,14 +175,13 @@ pub fn do_test(data: &[u8], out: Out) { let msg = decode_msg_with_len16!(msgs::ChannelAnnouncement, 64*4, 32+8+33*4); node_pks.insert(msg.contents.node_id_1); node_pks.insert(msg.contents.node_id_2); - let _ = net_graph.update_channel_from_announcement::(&msg, None, None); + let _ = net_graph.update_channel_from_announcement::(&msg, &None, None); }, 2 => { let msg = decode_msg_with_len16!(msgs::ChannelAnnouncement, 64*4, 32+8+33*4); node_pks.insert(msg.contents.node_id_1); node_pks.insert(msg.contents.node_id_2); - let val = slice_to_be64(get_slice!(8)); - let _ = net_graph.update_channel_from_announcement::(&msg, Some(val), None); + let _ = net_graph.update_channel_from_announcement::(&msg, &Some(&FuzzChainSource { input: Arc::clone(&input) }), None); }, 3 => { let _ = net_graph.update_channel(&decode_msg!(msgs::ChannelUpdate, 136), None); diff --git a/lightning/src/routing/network_graph.rs b/lightning/src/routing/network_graph.rs index 932f3779d..5003ca512 100644 --- a/lightning/src/routing/network_graph.rs +++ b/lightning/src/routing/network_graph.rs @@ -125,40 +125,7 @@ impl RoutingMessageHandler for N } fn handle_channel_announcement(&self, msg: &msgs::ChannelAnnouncement) -> Result { - if msg.contents.node_id_1 == msg.contents.node_id_2 || msg.contents.bitcoin_key_1 == msg.contents.bitcoin_key_2 { - return Err(LightningError{err: "Channel announcement node had a channel with itself".to_owned(), action: ErrorAction::IgnoreError}); - } - - let utxo_value = match &self.chain_access { - &None => { - // Tentatively accept, potentially exposing us to DoS attacks - None - }, - &Some(ref chain_access) => { - match chain_access.get_utxo(&msg.contents.chain_hash, msg.contents.short_channel_id) { - Ok(TxOut { value, script_pubkey }) => { - let expected_script = Builder::new().push_opcode(opcodes::all::OP_PUSHNUM_2) - .push_slice(&msg.contents.bitcoin_key_1.serialize()) - .push_slice(&msg.contents.bitcoin_key_2.serialize()) - .push_opcode(opcodes::all::OP_PUSHNUM_2) - .push_opcode(opcodes::all::OP_CHECKMULTISIG).into_script().to_v0_p2wsh(); - if script_pubkey != expected_script { - return Err(LightningError{err: format!("Channel announcement key ({}) didn't match on-chain script ({})", script_pubkey.to_hex(), expected_script.to_hex()), action: ErrorAction::IgnoreError}); - } - //TODO: Check if value is worth storing, use it to inform routing, and compare it - //to the new HTLC max field in channel_update - Some(value) - }, - Err(chain::AccessError::UnknownChain) => { - return Err(LightningError{err: format!("Channel announced on an unknown chain ({})", msg.contents.chain_hash.encode().to_hex()), action: ErrorAction::IgnoreError}); - }, - Err(chain::AccessError::UnknownTx) => { - return Err(LightningError{err: "Channel announced without corresponding UTXO entry".to_owned(), action: ErrorAction::IgnoreError}); - }, - } - }, - }; - let result = self.network_graph.write().unwrap().update_channel_from_announcement(msg, utxo_value, Some(&self.secp_ctx)); + let result = self.network_graph.write().unwrap().update_channel_from_announcement(msg, &self.chain_access, Some(&self.secp_ctx)); log_trace!(self.logger, "Added channel_announcement for {}{}", msg.contents.short_channel_id, if !msg.contents.excess_data.is_empty() { " with excess uninterpreted data!" } else { "" }); result } @@ -605,17 +572,14 @@ impl NetworkGraph { /// RoutingMessageHandler implementation to call it indirectly. This may be useful to accept /// routing messages without checking their signatures. /// - /// If the channel has been confirmed to exist on chain (with correctly-formatted scripts on - /// chain), set utxo_value to the value of the output on chain, otherwise leave it as None. - /// The UTXO value is then used in routing calculation if we have no better information on the - /// maximum HTLC value that can be sent over the channel. - /// - /// Further, setting utxo_value to Some indicates that the announcement message is genuine, - /// allowing us to update existing channel data in the case of reorgs or to replace bogus - /// channel data generated by a DoS attacker. + /// If a `chain::Access` object is provided via `chain_access`, it will be called to verify + /// the corresponding UTXO exists on chain and is correctly-formatted. /// /// Announcement signatures are checked here only if Secp256k1 object is provided. - pub fn update_channel_from_announcement(&mut self, msg: &msgs::ChannelAnnouncement, utxo_value: Option, secp_ctx: Option<&Secp256k1>) -> Result { + pub fn update_channel_from_announcement + (&mut self, msg: &msgs::ChannelAnnouncement, chain_access: &Option, secp_ctx: Option<&Secp256k1>) + -> Result + where C::Target: chain::Access { if msg.contents.node_id_1 == msg.contents.node_id_2 || msg.contents.bitcoin_key_1 == msg.contents.bitcoin_key_2 { return Err(LightningError{err: "Channel announcement node had a channel with itself".to_owned(), action: ErrorAction::IgnoreError}); } @@ -628,8 +592,37 @@ impl NetworkGraph { secp_verify_sig!(sig_verifier, &msg_hash, &msg.bitcoin_signature_2, &msg.contents.bitcoin_key_2); } - let should_relay = msg.contents.excess_data.is_empty(); + let utxo_value = match &chain_access { + &None => { + // Tentatively accept, potentially exposing us to DoS attacks + None + }, + &Some(ref chain_access) => { + match chain_access.get_utxo(&msg.contents.chain_hash, msg.contents.short_channel_id) { + Ok(TxOut { value, script_pubkey }) => { + let expected_script = Builder::new().push_opcode(opcodes::all::OP_PUSHNUM_2) + .push_slice(&msg.contents.bitcoin_key_1.serialize()) + .push_slice(&msg.contents.bitcoin_key_2.serialize()) + .push_opcode(opcodes::all::OP_PUSHNUM_2) + .push_opcode(opcodes::all::OP_CHECKMULTISIG).into_script().to_v0_p2wsh(); + if script_pubkey != expected_script { + return Err(LightningError{err: format!("Channel announcement key ({}) didn't match on-chain script ({})", script_pubkey.to_hex(), expected_script.to_hex()), action: ErrorAction::IgnoreError}); + } + //TODO: Check if value is worth storing, use it to inform routing, and compare it + //to the new HTLC max field in channel_update + Some(value) + }, + Err(chain::AccessError::UnknownChain) => { + return Err(LightningError{err: format!("Channel announced on an unknown chain ({})", msg.contents.chain_hash.encode().to_hex()), action: ErrorAction::IgnoreError}); + }, + Err(chain::AccessError::UnknownTx) => { + return Err(LightningError{err: "Channel announced without corresponding UTXO entry".to_owned(), action: ErrorAction::IgnoreError}); + }, + } + }, + }; + let should_relay = msg.contents.excess_data.is_empty(); let chan_info = ChannelInfo { features: msg.contents.features.clone(), node_one: msg.contents.node_id_1.clone(), -- 2.39.5