Merge pull request #132 from TheBlueMatt/2018-08-bolt-4-spec-return-fail

Return a malformed HTLC message when ephemeral pubkey is garbage

Author: TheBlueMatt

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"

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],
 				}));

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;

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

fuzz/fuzz_targets/msg_targets/msg_update_add_htlc_target.rs renamed to fuzz/fuzz_targets/msg_update_add_htlc_target.rs

@@ -1,19 +1,18 @@
-// 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;
 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")]

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 {

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<ChannelHolder>) {
-		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<SharedSecret>, MutexGuard<ChannelHolder>) {
 		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,15 +1777,15 @@ 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,
 					};
 				},
 				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()));
 				},