diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index 85ba3d11f3b..48c9064c649 100644 --- a/fuzz/Cargo.toml +++ b/fuzz/Cargo.toml @@ -64,6 +64,10 @@ path = "fuzz_targets/msg_pong_target.rs" name = "msg_error_message_target" path = "fuzz_targets/msg_error_message_target.rs" +[[bin]] +name = "msg_update_add_htlc_target" +path = "fuzz_targets/msg_update_add_htlc_target.rs" + [[bin]] name = "msg_accept_channel_target" path = "fuzz_targets/msg_targets/msg_accept_channel_target.rs" @@ -100,10 +104,6 @@ path = "fuzz_targets/msg_targets/msg_revoke_and_ack_target.rs" name = "msg_shutdown_target" path = "fuzz_targets/msg_targets/msg_shutdown_target.rs" -[[bin]] -name = "msg_update_add_htlc_target" -path = "fuzz_targets/msg_targets/msg_update_add_htlc_target.rs" - [[bin]] name = "msg_update_fail_malformed_htlc_target" path = "fuzz_targets/msg_targets/msg_update_fail_malformed_htlc_target.rs" diff --git a/fuzz/fuzz_targets/channel_target.rs b/fuzz/fuzz_targets/channel_target.rs index 04a3caccadc..ed5fa13b09c 100644 --- a/fuzz/fuzz_targets/channel_target.rs +++ b/fuzz/fuzz_targets/channel_target.rs @@ -269,7 +269,7 @@ pub fn do_test(data: &[u8]) { 0 => { test_err!(channel.send_htlc(slice_to_be64(get_slice!(8)), [42; 32], slice_to_be32(get_slice!(4)), msgs::OnionPacket { version: get_slice!(1)[0], - public_key: get_pubkey!(), + public_key: PublicKey::from_slice(&secp_ctx, get_slice!(33)), hop_data: [0; 20*65], hmac: [0; 32], })); diff --git a/fuzz/fuzz_targets/msg_error_message_target.rs b/fuzz/fuzz_targets/msg_error_message_target.rs index fa021e231db..ff3719559b2 100644 --- a/fuzz/fuzz_targets/msg_error_message_target.rs +++ b/fuzz/fuzz_targets/msg_error_message_target.rs @@ -1,6 +1,3 @@ -// This file is auto-generated by gen_target.sh based on msg_target_template.txt -// To modify it, modify msg_target_template.txt and run gen_target.sh instead. - extern crate lightning; use lightning::ln::msgs; diff --git a/fuzz/fuzz_targets/msg_targets/gen_target.sh b/fuzz/fuzz_targets/msg_targets/gen_target.sh index 4a5dc2fc556..249c0ce1f34 100755 --- a/fuzz/fuzz_targets/msg_targets/gen_target.sh +++ b/fuzz/fuzz_targets/msg_targets/gen_target.sh @@ -1,4 +1,4 @@ -for target in CommitmentSigned FundingCreated FundingLocked FundingSigned OpenChannel RevokeAndACK Shutdown UpdateAddHTLC UpdateFailHTLC UpdateFailMalformedHTLC UpdateFee UpdateFulfillHTLC AcceptChannel ClosingSigned ChannelReestablish; do +for target in CommitmentSigned FundingCreated FundingLocked FundingSigned OpenChannel RevokeAndACK Shutdown UpdateFailHTLC UpdateFailMalformedHTLC UpdateFee UpdateFulfillHTLC AcceptChannel ClosingSigned ChannelReestablish; do tn=$(echo $target | sed 's/\([a-z0-9]\)\([A-Z]\)/\1_\2/g') fn=msg_$(echo $tn | tr '[:upper:]' '[:lower:]')_target.rs cat msg_target_template.txt | sed s/MSG_TARGET/$target/ > $fn diff --git a/fuzz/fuzz_targets/msg_targets/msg_update_add_htlc_target.rs b/fuzz/fuzz_targets/msg_update_add_htlc_target.rs similarity index 74% rename from fuzz/fuzz_targets/msg_targets/msg_update_add_htlc_target.rs rename to fuzz/fuzz_targets/msg_update_add_htlc_target.rs index 07a0f819655..5616047c25e 100644 --- a/fuzz/fuzz_targets/msg_targets/msg_update_add_htlc_target.rs +++ b/fuzz/fuzz_targets/msg_update_add_htlc_target.rs @@ -1,6 +1,3 @@ -// This file is auto-generated by gen_target.sh based on msg_target_template.txt -// To modify it, modify msg_target_template.txt and run gen_target.sh instead. - extern crate lightning; use lightning::ln::msgs; @@ -8,12 +5,14 @@ use lightning::util::reset_rng_state; use lightning::ln::msgs::{MsgEncodable, MsgDecodable}; -mod utils; - #[inline] pub fn do_test(data: &[u8]) { reset_rng_state(); - test_msg!(msgs::UpdateAddHTLC, data); + if let Ok(msg) = msgs::UpdateAddHTLC::decode(data){ + let enc = msg.encode(); + assert_eq!(&data[0..85], &enc[0..85]); + assert_eq!(&data[85+33..enc.len()], &enc[85+33..]); + } } #[cfg(feature = "afl")] diff --git a/src/ln/channel.rs b/src/ln/channel.rs index d6e23a28392..44875fcd698 100644 --- a/src/ln/channel.rs +++ b/src/ln/channel.rs @@ -16,7 +16,7 @@ use crypto::hkdf::{hkdf_extract,hkdf_expand}; use ln::msgs; use ln::msgs::{ErrorAction, HandleError, MsgEncodable}; use ln::channelmonitor::ChannelMonitor; -use ln::channelmanager::{PendingHTLCStatus, PendingForwardHTLCInfo, HTLCFailReason}; +use ln::channelmanager::{PendingHTLCStatus, PendingForwardHTLCInfo, HTLCFailReason, HTLCFailureMsg}; use ln::chan_utils::{TxCreationKeys,HTLCOutputInCommitment,HTLC_SUCCESS_TX_WEIGHT,HTLC_TIMEOUT_TX_WEIGHT}; use ln::chan_utils; use chain::chaininterface::{FeeEstimator,ConfirmationTarget}; @@ -1643,6 +1643,7 @@ impl Channel { update_add_htlcs, update_fulfill_htlcs, update_fail_htlcs, + update_fail_malformed_htlcs: Vec::new(), commitment_signed, }, monitor_update))) }, @@ -1680,7 +1681,8 @@ impl Channel { let mut to_forward_infos = Vec::new(); let mut revoked_htlcs = Vec::new(); - let mut failed_htlcs = Vec::new(); + let mut update_fail_htlcs = Vec::new(); + let mut update_fail_malformed_htlcs = Vec::new(); let mut require_commitment = false; let mut value_to_self_msat_diff: i64 = 0; // We really shouldnt have two passes here, but retain gives a non-mutable ref (Rust bug) @@ -1708,7 +1710,10 @@ impl Channel { PendingHTLCStatus::Fail(fail_msg) => { htlc.state = HTLCState::LocalRemoved; require_commitment = true; - failed_htlcs.push(fail_msg); + match fail_msg { + HTLCFailureMsg::Relay(msg) => update_fail_htlcs.push(msg), + HTLCFailureMsg::Malformed(msg) => update_fail_malformed_htlcs.push(msg), + } }, PendingHTLCStatus::Forward(forward_info) => { to_forward_infos.push(forward_info); @@ -1727,10 +1732,14 @@ impl Channel { match self.free_holding_cell_htlcs()? { Some(mut commitment_update) => { - commitment_update.0.update_fail_htlcs.reserve(failed_htlcs.len()); - for fail_msg in failed_htlcs.drain(..) { + commitment_update.0.update_fail_htlcs.reserve(update_fail_htlcs.len()); + for fail_msg in update_fail_htlcs.drain(..) { commitment_update.0.update_fail_htlcs.push(fail_msg); } + commitment_update.0.update_fail_malformed_htlcs.reserve(update_fail_malformed_htlcs.len()); + for fail_msg in update_fail_malformed_htlcs.drain(..) { + commitment_update.0.update_fail_malformed_htlcs.push(fail_msg); + } Ok((Some(commitment_update.0), to_forward_infos, revoked_htlcs, commitment_update.1)) }, None => { @@ -1739,7 +1748,8 @@ impl Channel { Ok((Some(msgs::CommitmentUpdate { update_add_htlcs: Vec::new(), update_fulfill_htlcs: Vec::new(), - update_fail_htlcs: failed_htlcs, + update_fail_htlcs, + update_fail_malformed_htlcs, commitment_signed }), to_forward_infos, revoked_htlcs, monitor_update)) } else { diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index 3a98cd5a176..6275713b589 100644 --- a/src/ln/channelmanager.rs +++ b/src/ln/channelmanager.rs @@ -50,11 +50,17 @@ mod channel_held_info { pub(super) outgoing_cltv_value: u32, } + #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug + pub enum HTLCFailureMsg { + Relay(msgs::UpdateFailHTLC), + Malformed(msgs::UpdateFailMalformedHTLC), + } + /// Stores whether we can't forward an HTLC or relevant forwarding info #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug pub enum PendingHTLCStatus { Forward(PendingForwardHTLCInfo), - Fail(msgs::UpdateFailHTLC), + Fail(HTLCFailureMsg), } #[cfg(feature = "fuzztarget")] @@ -619,7 +625,7 @@ impl ChannelManager { Ok(msgs::OnionPacket{ version: 0, - public_key: onion_keys.first().unwrap().ephemeral_pubkey, + public_key: Ok(onion_keys.first().unwrap().ephemeral_pubkey), hop_data: packet_data, hmac: hmac_res, }) @@ -675,10 +681,7 @@ impl ChannelManager { ChannelManager::encrypt_failure_packet(shared_secret, &failure_packet.encode()[..]) } - fn decode_update_add_htlc_onion(&self, msg: &msgs::UpdateAddHTLC) -> (PendingHTLCStatus, SharedSecret, MutexGuard) { - let shared_secret = SharedSecret::new(&self.secp_ctx, &msg.onion_routing_packet.public_key, &self.our_network_key); - let (rho, mu) = ChannelManager::gen_rho_mu_from_shared_secret(&shared_secret); - + fn decode_update_add_htlc_onion(&self, msg: &msgs::UpdateAddHTLC) -> (PendingHTLCStatus, Option, MutexGuard) { macro_rules! get_onion_hash { () => { { @@ -691,6 +694,19 @@ impl ChannelManager { } } + if let Err(_) = msg.onion_routing_packet.public_key { + log_info!(self, "Failed to accept/forward incoming HTLC with invalid ephemeral pubkey"); + return (PendingHTLCStatus::Fail(HTLCFailureMsg::Malformed(msgs::UpdateFailMalformedHTLC { + channel_id: msg.channel_id, + htlc_id: msg.htlc_id, + sha256_of_onion: get_onion_hash!(), + failure_code: 0x8000 | 0x4000 | 6, + })), None, self.channel_state.lock().unwrap()); + } + + let shared_secret = SharedSecret::new(&self.secp_ctx, &msg.onion_routing_packet.public_key.unwrap(), &self.our_network_key); + let (rho, mu) = ChannelManager::gen_rho_mu_from_shared_secret(&shared_secret); + let mut channel_state = None; macro_rules! return_err { ($msg: expr, $err_code: expr, $data: expr) => { @@ -699,11 +715,11 @@ impl ChannelManager { if channel_state.is_none() { channel_state = Some(self.channel_state.lock().unwrap()); } - return (PendingHTLCStatus::Fail(msgs::UpdateFailHTLC { + return (PendingHTLCStatus::Fail(HTLCFailureMsg::Relay(msgs::UpdateFailHTLC { channel_id: msg.channel_id, htlc_id: msg.htlc_id, reason: ChannelManager::build_first_hop_failure_packet(&shared_secret, $err_code, $data), - }), shared_secret, channel_state.unwrap()); + })), Some(shared_secret), channel_state.unwrap()); } } } @@ -770,7 +786,7 @@ impl ChannelManager { chacha.process(&msg.onion_routing_packet.hop_data[65..], &mut new_packet_data[0..19*65]); chacha.process(&ChannelManager::ZERO[0..65], &mut new_packet_data[19*65..]); - let mut new_pubkey = msg.onion_routing_packet.public_key.clone(); + let mut new_pubkey = msg.onion_routing_packet.public_key.unwrap(); let blinding_factor = { let mut sha = Sha256::new(); @@ -780,26 +796,19 @@ impl ChannelManager { sha.result(&mut res); match SecretKey::from_slice(&self.secp_ctx, &res) { Err(_) => { - // Return temporary node failure as its technically our issue, not the - // channel's issue. - return_err!("Blinding factor is an invalid private key", 0x2000 | 2, &[0;0]); + return_err!("Blinding factor is an invalid private key", 0x8000 | 0x4000 | 6, &get_onion_hash!()); }, Ok(key) => key } }; - match new_pubkey.mul_assign(&self.secp_ctx, &blinding_factor) { - Err(_) => { - // Return temporary node failure as its technically our issue, not the - // channel's issue. - return_err!("New blinding factor is an invalid private key", 0x2000 | 2, &[0;0]); - }, - Ok(_) => {} - }; + if let Err(_) = new_pubkey.mul_assign(&self.secp_ctx, &blinding_factor) { + return_err!("New blinding factor is an invalid private key", 0x8000 | 0x4000 | 6, &get_onion_hash!()); + } let outgoing_packet = msgs::OnionPacket { version: 0, - public_key: new_pubkey, + public_key: Ok(new_pubkey), hop_data: new_packet_data, hmac: next_hop_data.hmac.clone(), }; @@ -846,7 +855,7 @@ impl ChannelManager { } } - (pending_forward_info, shared_secret, channel_state.unwrap()) + (pending_forward_info, Some(shared_secret), channel_state.unwrap()) } /// only fails if the channel does not yet have an assigned short_id @@ -958,6 +967,7 @@ impl ChannelManager { update_add_htlcs: vec![update_add], update_fulfill_htlcs: Vec::new(), update_fail_htlcs: Vec::new(), + update_fail_malformed_htlcs: Vec::new(), commitment_signed, }, }); @@ -1102,6 +1112,7 @@ impl ChannelManager { update_add_htlcs: add_htlc_msgs, update_fulfill_htlcs: Vec::new(), update_fail_htlcs: Vec::new(), + update_fail_malformed_htlcs: Vec::new(), commitment_signed: commitment_msg, }, })); @@ -1225,6 +1236,7 @@ impl ChannelManager { update_add_htlcs: Vec::new(), update_fulfill_htlcs: Vec::new(), update_fail_htlcs: vec![msg], + update_fail_malformed_htlcs: Vec::new(), commitment_signed: commitment_msg, }, }); @@ -1324,6 +1336,7 @@ impl ChannelManager { update_add_htlcs: Vec::new(), update_fulfill_htlcs: vec![msg], update_fail_htlcs: Vec::new(), + update_fail_malformed_htlcs: Vec::new(), commitment_signed: commitment_msg, } }); @@ -1722,11 +1735,11 @@ impl ChannelMessageHandler for ChannelManager { } if !acceptable_cycle { log_info!(self, "Failed to accept incoming HTLC: Payment looped through us twice"); - pending_forward_info = PendingHTLCStatus::Fail(msgs::UpdateFailHTLC { + pending_forward_info = PendingHTLCStatus::Fail(HTLCFailureMsg::Relay(msgs::UpdateFailHTLC { channel_id: msg.channel_id, htlc_id: msg.htlc_id, - reason: ChannelManager::build_first_hop_failure_packet(&shared_secret, 0x4000 | 0x2000 | 2, &[0;0]), - }); + reason: ChannelManager::build_first_hop_failure_packet(&shared_secret.unwrap(), 0x4000 | 0x2000 | 2, &[0;0]), + })); } else { will_forward = true; } @@ -1764,7 +1777,7 @@ impl ChannelMessageHandler for ChannelManager { }; *outbound_route = PendingOutboundHTLC::CycledRoute { source_short_channel_id, - incoming_packet_shared_secret: shared_secret, + incoming_packet_shared_secret: shared_secret.unwrap(), route, session_priv, }; @@ -1772,7 +1785,7 @@ impl ChannelMessageHandler for ChannelManager { hash_map::Entry::Vacant(e) => { e.insert(PendingOutboundHTLC::IntermediaryHopData { source_short_channel_id, - incoming_packet_shared_secret: shared_secret, + incoming_packet_shared_secret: shared_secret.unwrap(), }); } } @@ -2487,9 +2500,10 @@ mod tests { impl SendEvent { fn from_event(event: Event) -> SendEvent { match event { - Event::UpdateHTLCs { node_id, updates: msgs::CommitmentUpdate { update_add_htlcs, update_fulfill_htlcs, update_fail_htlcs, commitment_signed } } => { + Event::UpdateHTLCs { node_id, updates: msgs::CommitmentUpdate { update_add_htlcs, update_fulfill_htlcs, update_fail_htlcs, update_fail_malformed_htlcs, commitment_signed } } => { assert!(update_fulfill_htlcs.is_empty()); assert!(update_fail_htlcs.is_empty()); + assert!(update_fail_malformed_htlcs.is_empty()); SendEvent { node_id: node_id, msgs: update_add_htlcs, commitment_msg: commitment_signed } }, _ => panic!("Unexpected event type!"), @@ -2646,10 +2660,11 @@ mod tests { let events = node.node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); match events[0] { - Event::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref commitment_signed } } => { + Event::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref update_fail_malformed_htlcs, ref commitment_signed } } => { assert!(update_add_htlcs.is_empty()); assert_eq!(update_fulfill_htlcs.len(), 1); assert!(update_fail_htlcs.is_empty()); + assert!(update_fail_malformed_htlcs.is_empty()); expected_next_node = node_id.clone(); next_msgs = Some((update_fulfill_htlcs[0].clone(), commitment_signed.clone())); }, @@ -2770,10 +2785,11 @@ mod tests { let events = node.node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); match events[0] { - Event::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref commitment_signed } } => { + Event::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref update_fail_malformed_htlcs, ref commitment_signed } } => { assert!(update_add_htlcs.is_empty()); assert!(update_fulfill_htlcs.is_empty()); assert_eq!(update_fail_htlcs.len(), 1); + assert!(update_fail_malformed_htlcs.is_empty()); expected_next_node = node_id.clone(); next_msgs = Some((update_fail_htlcs[0].clone(), commitment_signed.clone())); }, diff --git a/src/ln/msgs.rs b/src/ln/msgs.rs index 44d71bd3329..4067e84b98c 100644 --- a/src/ln/msgs.rs +++ b/src/ln/msgs.rs @@ -1,5 +1,6 @@ use secp256k1::key::PublicKey; use secp256k1::{Secp256k1, Signature}; +use secp256k1; use bitcoin::util::hash::Sha256dHash; use bitcoin::network::serialize::{deserialize,serialize}; use bitcoin::blockdata::script::Script; @@ -399,6 +400,7 @@ pub struct CommitmentUpdate { pub update_add_htlcs: Vec, pub update_fulfill_htlcs: Vec, pub update_fail_htlcs: Vec, + pub update_fail_malformed_htlcs: Vec, pub commitment_signed: CommitmentSigned, } @@ -475,7 +477,10 @@ unsafe impl internal_traits::NoDealloc for OnionHopData{} #[derive(Clone)] pub struct OnionPacket { pub version: u8, - pub public_key: PublicKey, + /// In order to ensure we always return an error on Onion decode in compliance with BOLT 4, we + /// have to deserialize OnionPackets contained in UpdateAddHTLCs even if the ephemeral public + /// key (here) is bogus, so we hold a Result instead of a PublicKey as we'd like. + pub public_key: Result, pub hop_data: [u8; 20*65], pub hmac: [u8; 32], } @@ -1537,7 +1542,7 @@ impl MsgDecodable for OnionPacket { let secp_ctx = Secp256k1::without_caps(); Ok(Self { version: v[0], - public_key: secp_pubkey!(&secp_ctx, &v[1..34]), + public_key: PublicKey::from_slice(&secp_ctx, &v[1..34]), hop_data, hmac, }) @@ -1547,7 +1552,10 @@ impl MsgEncodable for OnionPacket { fn encode(&self) -> Vec { let mut res = Vec::with_capacity(1 + 33 + 20*65 + 32); res.push(self.version); - res.extend_from_slice(&self.public_key.serialize()); + match self.public_key { + Ok(pubkey) => res.extend_from_slice(&pubkey.serialize()), + Err(_) => res.extend_from_slice(&[0; 33]), + } res.extend_from_slice(&self.hop_data); res.extend_from_slice(&self.hmac); res diff --git a/src/ln/peer_handler.rs b/src/ln/peer_handler.rs index f941f47dc10..9315f56902d 100644 --- a/src/ln/peer_handler.rs +++ b/src/ln/peer_handler.rs @@ -697,7 +697,7 @@ impl PeerManager { Self::do_attempt_write_data(&mut descriptor, peer); continue; }, - Event::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref commitment_signed } } => { + Event::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref update_fail_malformed_htlcs, ref commitment_signed } } => { log_trace!(self, "Handling UpdateHTLCs event in peer_handler for node {} with {} adds, {} fulfills, {} fails for channel {}", log_pubkey!(node_id), update_add_htlcs.len(), @@ -716,6 +716,9 @@ impl PeerManager { for msg in update_fail_htlcs { peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(msg, 131))); } + for msg in update_fail_malformed_htlcs { + peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(msg, 135))); + } peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(commitment_signed, 132))); Self::do_attempt_write_data(&mut descriptor, peer); continue;