From 42e908883f3a655270e0b442295a361d56e14b3b Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 26 Aug 2018 16:35:26 -0400 Subject: [PATCH] Return a malformed HTLC message when ephemeral pubkey is garbage This resolves a spec-compliance bug with BOLT 4 where we simply failed to deserialize the message and thus could never return an HTLC failure message. However, note that BOLT 4 incorrectly hints that a non-malformed message should be used ("...MUST report a route failure to the origin node") which we cannot do as we cannot derive a SharedSecret to encrypt a regular update_fail_htlc message --- fuzz/Cargo.toml | 8 +-- fuzz/fuzz_targets/channel_target.rs | 2 +- fuzz/fuzz_targets/msg_targets/gen_target.sh | 2 +- .../msg_update_add_htlc_target.rs | 11 ++--- src/ln/channelmanager.rs | 49 ++++++++++--------- src/ln/msgs.rs | 13 +++-- 6 files changed, 47 insertions(+), 38 deletions(-) rename fuzz/fuzz_targets/{msg_targets => }/msg_update_add_htlc_target.rs (74%) diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index 85ba3d11..48c9064c 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 04a3cacc..ed5fa13b 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_targets/gen_target.sh b/fuzz/fuzz_targets/msg_targets/gen_target.sh index 4a5dc2fc..249c0ce1 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 07a0f819..5616047c 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/channelmanager.rs b/src/ln/channelmanager.rs index 88eb4b12..6275713b 100644 --- a/src/ln/channelmanager.rs +++ b/src/ln/channelmanager.rs @@ -625,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, }) @@ -681,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 { () => { { @@ -697,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) => { @@ -709,7 +719,7 @@ impl ChannelManager { 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()); } } } @@ -776,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(); @@ -786,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(), }; @@ -852,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 @@ -1735,7 +1738,7 @@ impl ChannelMessageHandler for ChannelManager { 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; @@ -1774,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, }; @@ -1782,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(), }); } } diff --git a/src/ln/msgs.rs b/src/ln/msgs.rs index 6c7c8cfc..4067e84b 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; @@ -476,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], } @@ -1538,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, }) @@ -1548,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 -- 2.30.2