Merge pull request #1324 from valentinewallace/2022-02-phantom-followup

#1199 Followup

Author: TheBlueMatt

fuzz/src/full_stack.rs

@@ -390,6 +390,9 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
 		best_block: BestBlock::from_genesis(network),
 	};
 	let channelmanager = Arc::new(ChannelManager::new(fee_est.clone(), monitor.clone(), broadcast.clone(), Arc::clone(&logger), keys_manager.clone(), config, params));
+	// Adding new calls to `KeysInterface::get_secure_random_bytes` during startup can change all the
+	// keys subsequently generated in this test. Rather than regenerating all the messages manually,
+	// it's easier to just increment the counter here so the keys don't change.
 	keys_manager.counter.fetch_sub(1, Ordering::AcqRel);
 	let our_id = PublicKey::from_secret_key(&Secp256k1::signing_only(), &keys_manager.get_node_secret(Recipient::Node).unwrap());
 	let network_graph = Arc::new(NetworkGraph::new(genesis_block(network).block_hash()));

lightning/src/ln/channelmanager.rs

@@ -358,14 +358,15 @@ mod inbound_payment {
 // our payment, which we can use to decode errors or inform the user that the payment was sent.
 
 #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug
-enum PendingHTLCRouting {
+pub(super) enum PendingHTLCRouting {
 	Forward {
 		onion_packet: msgs::OnionPacket,
 		short_channel_id: u64, // This should be NonZero<u64> eventually when we bump MSRV
 	},
 	Receive {
 		payment_data: msgs::FinalOnionHopData,
 		incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed
+		phantom_shared_secret: Option<[u8; 32]>,
 	},
 	ReceiveKeysend {
 		payment_preimage: PaymentPreimage,
@@ -375,8 +376,8 @@ enum PendingHTLCRouting {
 
 #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug
 pub(super) struct PendingHTLCInfo {
-	routing: PendingHTLCRouting,
-	incoming_shared_secret: [u8; 32],
+	pub(super) routing: PendingHTLCRouting,
+	pub(super) incoming_shared_secret: [u8; 32],
 	payment_hash: PaymentHash,
 	pub(super) amt_to_forward: u64,
 	pub(super) outgoing_cltv_value: u32,
@@ -419,6 +420,7 @@ pub(crate) struct HTLCPreviousHopData {
 	short_channel_id: u64,
 	htlc_id: u64,
 	incoming_packet_shared_secret: [u8; 32],
+	phantom_shared_secret: Option<[u8; 32]>,
 
 	// This field is consumed by `claim_funds_from_hop()` when updating a force-closed backwards
 	// channel with a preimage provided by the forward channel.
@@ -2072,7 +2074,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	}
 
 	fn construct_recv_pending_htlc_info(&self, hop_data: msgs::OnionHopData, shared_secret: [u8; 32],
-		payment_hash: PaymentHash, amt_msat: u64, cltv_expiry: u32) -> Result<PendingHTLCInfo, ReceiveError>
+		payment_hash: PaymentHash, amt_msat: u64, cltv_expiry: u32, phantom_shared_secret: Option<[u8; 32]>) -> Result<PendingHTLCInfo, ReceiveError>
 	{
 		// final_incorrect_cltv_expiry
 		if hop_data.outgoing_cltv_value != cltv_expiry {
@@ -2129,6 +2131,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 					PendingHTLCRouting::Receive {
 						payment_data: data,
 						incoming_cltv_expiry: hop_data.outgoing_cltv_value,
+						phantom_shared_secret,
 					}
 				} else if let Some(payment_preimage) = keysend_preimage {
 					// We need to check that the sender knows the keysend preimage before processing this
@@ -2232,7 +2235,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		let pending_forward_info = match next_hop {
 			onion_utils::Hop::Receive(next_hop_data) => {
 				// OUR PAYMENT!
-				match self.construct_recv_pending_htlc_info(next_hop_data, shared_secret, msg.payment_hash, msg.amount_msat, msg.cltv_expiry) {
+				match self.construct_recv_pending_htlc_info(next_hop_data, shared_secret, msg.payment_hash, msg.amount_msat, msg.cltv_expiry, None) {
 					Ok(info) => {
 						// Note that we could obviously respond immediately with an update_fulfill_htlc
 						// message, however that would leak that we are the recipient of this payment, so
@@ -3012,17 +3015,18 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 										routing, incoming_shared_secret, payment_hash, amt_to_forward, outgoing_cltv_value },
 										prev_funding_outpoint } => {
 											macro_rules! fail_forward {
-												($msg: expr, $err_code: expr, $err_data: expr) => {
+												($msg: expr, $err_code: expr, $err_data: expr, $phantom_ss: expr) => {
 													{
 														log_info!(self.logger, "Failed to accept/forward incoming HTLC: {}", $msg);
 														let htlc_source = HTLCSource::PreviousHopData(HTLCPreviousHopData {
-															short_channel_id: short_chan_id,
+															short_channel_id: prev_short_channel_id,
 															outpoint: prev_funding_outpoint,
 															htlc_id: prev_htlc_id,
 															incoming_packet_shared_secret: incoming_shared_secret,
+															phantom_shared_secret: $phantom_ss,
 														});
 														failed_forwards.push((htlc_source, payment_hash,
-																HTLCFailReason::Reason { failure_code: $err_code, data: $err_data }
+															HTLCFailReason::Reason { failure_code: $err_code, data: $err_data }
 														));
 														continue;
 													}
@@ -3031,44 +3035,46 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 											if let PendingHTLCRouting::Forward { onion_packet, .. } = routing {
 												let phantom_secret_res = self.keys_manager.get_node_secret(Recipient::PhantomNode);
 												if phantom_secret_res.is_ok() && fake_scid::is_valid_phantom(&self.fake_scid_rand_bytes, short_chan_id) {
-													let shared_secret = {
+													let phantom_shared_secret = {
 														let mut arr = [0; 32];
 														arr.copy_from_slice(&SharedSecret::new(&onion_packet.public_key.unwrap(), &phantom_secret_res.unwrap())[..]);
 														arr
 													};
-													let next_hop = match onion_utils::decode_next_hop(shared_secret, &onion_packet.hop_data, onion_packet.hmac, payment_hash) {
+													let next_hop = match onion_utils::decode_next_hop(phantom_shared_secret, &onion_packet.hop_data, onion_packet.hmac, payment_hash) {
 														Ok(res) => res,
 														Err(onion_utils::OnionDecodeErr::Malformed { err_msg, err_code }) => {
-															fail_forward!(err_msg, err_code, Vec::new());
+															let sha256_of_onion = Sha256::hash(&onion_packet.hop_data).into_inner();
+															// In this scenario, the phantom would have sent us an
+															// `update_fail_malformed_htlc`, meaning here we encrypt the error as
+															// if it came from us (the second-to-last hop) but contains the sha256
+															// of the onion.
+															fail_forward!(err_msg, err_code, sha256_of_onion.to_vec(), None);
 														},
 														Err(onion_utils::OnionDecodeErr::Relay { err_msg, err_code }) => {
-															fail_forward!(err_msg, err_code, Vec::new());
+															fail_forward!(err_msg, err_code, Vec::new(), Some(phantom_shared_secret));
 														},
 													};
 													match next_hop {
 														onion_utils::Hop::Receive(hop_data) => {
-															match self.construct_recv_pending_htlc_info(hop_data, shared_secret, payment_hash, amt_to_forward, outgoing_cltv_value) {
+															match self.construct_recv_pending_htlc_info(hop_data, incoming_shared_secret, payment_hash, amt_to_forward, outgoing_cltv_value, Some(phantom_shared_secret)) {
 																Ok(info) => phantom_receives.push((prev_short_channel_id, prev_funding_outpoint, vec![(info, prev_htlc_id)])),
-																Err(ReceiveError { err_code, err_data, msg }) => fail_forward!(msg, err_code, err_data)
+																Err(ReceiveError { err_code, err_data, msg }) => fail_forward!(msg, err_code, err_data, Some(phantom_shared_secret))
 															}
 														},
 														_ => panic!(),
 													}
 												} else {
-													fail_forward!(format!("Unknown short channel id {} for forward HTLC", short_chan_id), 0x4000 | 10, Vec::new());
+													fail_forward!(format!("Unknown short channel id {} for forward HTLC", short_chan_id), 0x4000 | 10, Vec::new(), None);
 												}
 											} else {
-												fail_forward!(format!("Unknown short channel id {} for forward HTLC", short_chan_id), 0x4000 | 10, Vec::new());
+												fail_forward!(format!("Unknown short channel id {} for forward HTLC", short_chan_id), 0x4000 | 10, Vec::new(), None);
 											}
 										},
 									HTLCForwardInfo::FailHTLC { .. } => {
 										// Channel went away before we could fail it. This implies
 										// the channel is now on chain and our counterparty is
 										// trying to broadcast the HTLC-Timeout, but that's their
 										// problem, not ours.
-										//
-										// `fail_htlc_backwards_internal` is never called for
-										// phantom payments, so this is unreachable for them.
 									}
 								}
 							}
@@ -3091,6 +3097,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 										outpoint: prev_funding_outpoint,
 										htlc_id: prev_htlc_id,
 										incoming_packet_shared_secret: incoming_shared_secret,
+										// Phantom payments are only PendingHTLCRouting::Receive.
+										phantom_shared_secret: None,
 									});
 									match chan.get_mut().send_htlc(amt_to_forward, payment_hash, outgoing_cltv_value, htlc_source.clone(), onion_packet, &self.logger) {
 										Err(e) => {
@@ -3207,11 +3215,11 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 							HTLCForwardInfo::AddHTLC { prev_short_channel_id, prev_htlc_id, forward_info: PendingHTLCInfo {
 									routing, incoming_shared_secret, payment_hash, amt_to_forward, .. },
 									prev_funding_outpoint } => {
-								let (cltv_expiry, onion_payload) = match routing {
-									PendingHTLCRouting::Receive { payment_data, incoming_cltv_expiry } =>
-										(incoming_cltv_expiry, OnionPayload::Invoice(payment_data)),
+								let (cltv_expiry, onion_payload, phantom_shared_secret) = match routing {
+									PendingHTLCRouting::Receive { payment_data, incoming_cltv_expiry, phantom_shared_secret } =>
+										(incoming_cltv_expiry, OnionPayload::Invoice(payment_data), phantom_shared_secret),
 									PendingHTLCRouting::ReceiveKeysend { payment_preimage, incoming_cltv_expiry } =>
-										(incoming_cltv_expiry, OnionPayload::Spontaneous(payment_preimage)),
+										(incoming_cltv_expiry, OnionPayload::Spontaneous(payment_preimage), None),
 									_ => {
 										panic!("short_channel_id == 0 should imply any pending_forward entries are of type Receive");
 									}
@@ -3222,6 +3230,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 										outpoint: prev_funding_outpoint,
 										htlc_id: prev_htlc_id,
 										incoming_packet_shared_secret: incoming_shared_secret,
+										phantom_shared_secret,
 									},
 									value: amt_to_forward,
 									cltv_expiry,
@@ -3239,6 +3248,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 												outpoint: prev_funding_outpoint,
 												htlc_id: $htlc.prev_hop.htlc_id,
 												incoming_packet_shared_secret: $htlc.prev_hop.incoming_packet_shared_secret,
+												phantom_shared_secret,
 											}), payment_hash,
 											HTLCFailReason::Reason { failure_code: 0x4000 | 15, data: htlc_msat_height_data }
 										));
@@ -3778,12 +3788,18 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 				pending_events.push(path_failure);
 				if let Some(ev) = full_failure_ev { pending_events.push(ev); }
 			},
-			HTLCSource::PreviousHopData(HTLCPreviousHopData { short_channel_id, htlc_id, incoming_packet_shared_secret, .. }) => {
+			HTLCSource::PreviousHopData(HTLCPreviousHopData { short_channel_id, htlc_id, incoming_packet_shared_secret, phantom_shared_secret, .. }) => {
 				let err_packet = match onion_error {
 					HTLCFailReason::Reason { failure_code, data } => {
 						log_trace!(self.logger, "Failing HTLC with payment_hash {} backwards from us with code {}", log_bytes!(payment_hash.0), failure_code);
-						let packet = onion_utils::build_failure_packet(&incoming_packet_shared_secret, failure_code, &data[..]).encode();
-						onion_utils::encrypt_failure_packet(&incoming_packet_shared_secret, &packet)
+						if let Some(phantom_ss) = phantom_shared_secret {
+							let phantom_packet = onion_utils::build_failure_packet(&phantom_ss, failure_code, &data[..]).encode();
+							let encrypted_phantom_packet = onion_utils::encrypt_failure_packet(&phantom_ss, &phantom_packet);
+							onion_utils::encrypt_failure_packet(&incoming_packet_shared_secret, &encrypted_phantom_packet.data[..])
+						} else {
+							let packet = onion_utils::build_failure_packet(&incoming_packet_shared_secret, failure_code, &data[..]).encode();
+							onion_utils::encrypt_failure_packet(&incoming_packet_shared_secret, &packet)
+						}
 					},
 					HTLCFailReason::LightningError { err } => {
 						log_trace!(self.logger, "Failing HTLC with payment_hash {} backwards with pre-built LightningError", log_bytes!(payment_hash.0));
@@ -4487,7 +4503,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 									onion_utils::build_first_hop_failure_packet(incoming_shared_secret, error_code, &{
 										let mut res = Vec::with_capacity(8 + 128);
 										// TODO: underspecified, follow https://github.com/lightningnetwork/lightning-rfc/issues/791
-										res.extend_from_slice(&byte_utils::be16_to_array(0));
+										if error_code == 0x1000 | 20 {
+											res.extend_from_slice(&byte_utils::be16_to_array(0));
+										}
 										res.extend_from_slice(&upd.encode_with_len()[..]);
 										res
 									}[..])
@@ -5978,6 +5996,7 @@ impl_writeable_tlv_based_enum!(PendingHTLCRouting,
 	},
 	(1, Receive) => {
 		(0, payment_data, required),
+		(1, phantom_shared_secret, option),
 		(2, incoming_cltv_expiry, required),
 	},
 	(2, ReceiveKeysend) => {
@@ -6069,6 +6088,7 @@ impl_writeable_tlv_based_enum!(PendingHTLCStatus, ;
 
 impl_writeable_tlv_based!(HTLCPreviousHopData, {
 	(0, short_channel_id, required),
+	(1, phantom_shared_secret, option),
 	(2, outpoint, required),
 	(4, htlc_id, required),
 	(6, incoming_packet_shared_secret, required)

lightning/src/ln/msgs.rs

@@ -1299,10 +1299,6 @@ impl Readable for FinalOnionHopData {
 
 impl Writeable for OnionHopData {
 	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
-		// Note that this should never be reachable if Rust-Lightning generated the message, as we
-		// check values are sane long before we get here, though its possible in the future
-		// user-generated messages may hit this.
-		if self.amt_to_forward > MAX_VALUE_MSAT { panic!("We should never be sending infinite/overflow onion payments"); }
 		match self.format {
 			OnionHopDataFormat::Legacy { short_channel_id } => {
 				0u8.write(w)?;
@@ -1319,9 +1315,6 @@ impl Writeable for OnionHopData {
 				});
 			},
 			OnionHopDataFormat::FinalNode { ref payment_data, ref keysend_preimage } => {
-				if let Some(final_data) = payment_data {
-					if final_data.total_msat > MAX_VALUE_MSAT { panic!("We should never be sending infinite/overflow onion payments"); }
-				}
 				encode_varint_length_prefixed_tlv!(w, {
 					(2, HighZeroBytesDroppedVarInt(self.amt_to_forward), required),
 					(4, HighZeroBytesDroppedVarInt(self.outgoing_cltv_value), required),