Support (de)serializing payment_data in onion TLVs and track them
authorMatt Corallo <git@bluematt.me>
Thu, 2 Jan 2020 01:20:42 +0000 (20:20 -0500)
committerMatt Corallo <git@bluematt.me>
Tue, 14 Apr 2020 23:54:17 +0000 (19:54 -0400)
This is the first step in Base AMP support, just tracking the
relevant data in internal datastructures.

lightning/src/ln/channelmanager.rs
lightning/src/ln/msgs.rs
lightning/src/ln/onion_utils.rs

index 84b595a2d3fcaef5b624b61e3fd27a0513766c75..1a207bf9104f1b4dcc45b9329c0cedf43af3986f 100644 (file)
@@ -74,7 +74,9 @@ enum PendingHTLCRouting {
                onion_packet: msgs::OnionPacket,
                short_channel_id: u64, // This should be NonZero<u64> eventually when we bump MSRV
        },
-       Receive {},
+       Receive {
+               payment_data: Option<msgs::FinalOnionHopData>,
+       },
 }
 
 #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug
@@ -119,6 +121,16 @@ pub(super) struct HTLCPreviousHopData {
        incoming_packet_shared_secret: [u8; 32],
 }
 
+struct ClaimableHTLC {
+       prev_hop: HTLCPreviousHopData,
+       value: u64,
+       /// Filled in when the HTLC was received with a payment_secret packet, which contains a
+       /// total_msat (which may differ from value if this is a Multi-Path Payment) and a
+       /// payment_secret which prevents path-probing attacks and can associate different HTLCs which
+       /// are part of the same payment.
+       payment_data: Option<msgs::FinalOnionHopData>,
+}
+
 /// Tracks the inbound corresponding to an outbound HTLC
 #[derive(Clone, PartialEq)]
 pub(super) enum HTLCSource {
@@ -276,12 +288,11 @@ pub(super) struct ChannelHolder<ChanSigner: ChannelKeys> {
        /// guarantees are made about the existence of a channel with the short id here, nor the short
        /// ids in the PendingHTLCInfo!
        pub(super) forward_htlcs: HashMap<u64, Vec<HTLCForwardInfo>>,
-       /// payment_hash -> Vec<(amount_received, htlc_source)> for tracking things that were to us and
-       /// can be failed/claimed by the user
+       /// Tracks HTLCs that were to us and can be failed/claimed by the user
        /// Note that while this is held in the same mutex as the channels themselves, no consistency
        /// guarantees are made about the channels given here actually existing anymore by the time you
        /// go to read them!
-       pub(super) claimable_htlcs: HashMap<PaymentHash, Vec<(u64, HTLCPreviousHopData)>>,
+       claimable_htlcs: HashMap<PaymentHash, Vec<ClaimableHTLC>>,
        /// Messages to send to peers - pushed to in the same lock that they are generated in (except
        /// for broadcast messages, where ordering isn't as strict).
        pub(super) pending_msg_events: Vec<events::MessageSendEvent>,
@@ -1007,13 +1018,19 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref> ChannelMan
                                        return_err!("Upstream node set CLTV to the wrong value", 18, &byte_utils::be32_to_array(msg.cltv_expiry));
                                }
 
+                               let payment_data = match next_hop_data.format {
+                                       msgs::OnionHopDataFormat::Legacy { .. } => None,
+                                       msgs::OnionHopDataFormat::NonFinalNode { .. } => return_err!("Got non final data with an HMAC of 0", 0x4000 | 22, &[0;0]),
+                                       msgs::OnionHopDataFormat::FinalNode { payment_data } => payment_data,
+                               };
+
                                // 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
                                // instead we stay symmetric with the forwarding case, only responding (after a
                                // delay) once they've send us a commitment_signed!
 
                                PendingHTLCStatus::Forward(PendingHTLCInfo {
-                                       routing: PendingHTLCRouting::Receive {},
+                                       routing: PendingHTLCRouting::Receive { payment_data },
                                        payment_hash: msg.payment_hash.clone(),
                                        incoming_shared_secret: shared_secret,
                                        amt_to_forward: next_hop_data.amt_to_forward,
@@ -1580,17 +1597,18 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref> ChannelMan
                                        for forward_info in pending_forwards.drain(..) {
                                                match forward_info {
                                                        HTLCForwardInfo::AddHTLC { prev_short_channel_id, prev_htlc_id, forward_info: PendingHTLCInfo {
-                                                                       routing: PendingHTLCRouting::Receive { },
+                                                                       routing: PendingHTLCRouting::Receive { payment_data },
                                                                        incoming_shared_secret, payment_hash, amt_to_forward, .. }, } => {
-                                                               let prev_hop_data = HTLCPreviousHopData {
+                                                               let prev_hop = HTLCPreviousHopData {
                                                                        short_channel_id: prev_short_channel_id,
                                                                        htlc_id: prev_htlc_id,
                                                                        incoming_packet_shared_secret: incoming_shared_secret,
                                                                };
-                                                               match channel_state.claimable_htlcs.entry(payment_hash) {
-                                                                       hash_map::Entry::Occupied(mut entry) => entry.get_mut().push((amt_to_forward, prev_hop_data)),
-                                                                       hash_map::Entry::Vacant(entry) => { entry.insert(vec![(amt_to_forward, prev_hop_data)]); },
-                                                               };
+                                                               channel_state.claimable_htlcs.entry(payment_hash).or_insert(Vec::new()).push(ClaimableHTLC {
+                                                                       prev_hop,
+                                                                       value: amt_to_forward,
+                                                                       payment_data,
+                                                               });
                                                                new_events.push(events::Event::PaymentReceived {
                                                                        payment_hash: payment_hash,
                                                                        amt: amt_to_forward,
@@ -1660,11 +1678,11 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref> ChannelMan
                let mut channel_state = Some(self.channel_state.lock().unwrap());
                let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(payment_hash);
                if let Some(mut sources) = removed_source {
-                       for (recvd_value, htlc_with_hash) in sources.drain(..) {
+                       for htlc in sources.drain(..) {
                                if channel_state.is_none() { channel_state = Some(self.channel_state.lock().unwrap()); }
                                self.fail_htlc_backwards_internal(channel_state.take().unwrap(),
-                                               HTLCSource::PreviousHopData(htlc_with_hash), payment_hash,
-                                               HTLCFailReason::Reason { failure_code: 0x4000 | 15, data: byte_utils::be64_to_array(recvd_value).to_vec() });
+                                               HTLCSource::PreviousHopData(htlc.prev_hop), payment_hash,
+                                               HTLCFailReason::Reason { failure_code: 0x4000 | 15, data: byte_utils::be64_to_array(htlc.value).to_vec() });
                        }
                        true
                } else { false }
@@ -1788,17 +1806,17 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref> ChannelMan
                let mut channel_state = Some(self.channel_state.lock().unwrap());
                let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(&payment_hash);
                if let Some(mut sources) = removed_source {
-                       for (received_amount, htlc_with_hash) in sources.drain(..) {
+                       for htlc in sources.drain(..) {
                                if channel_state.is_none() { channel_state = Some(self.channel_state.lock().unwrap()); }
-                               if received_amount < expected_amount || received_amount > expected_amount * 2 {
-                                       let mut htlc_msat_data = byte_utils::be64_to_array(received_amount).to_vec();
+                               if htlc.value < expected_amount || htlc.value > expected_amount * 2 {
+                                       let mut htlc_msat_data = byte_utils::be64_to_array(htlc.value).to_vec();
                                        let mut height_data = byte_utils::be32_to_array(self.latest_block_height.load(Ordering::Acquire) as u32).to_vec();
                                        htlc_msat_data.append(&mut height_data);
                                        self.fail_htlc_backwards_internal(channel_state.take().unwrap(),
-                                                                        HTLCSource::PreviousHopData(htlc_with_hash), &payment_hash,
+                                                                        HTLCSource::PreviousHopData(htlc.prev_hop), &payment_hash,
                                                                         HTLCFailReason::Reason { failure_code: 0x4000|15, data: htlc_msat_data });
                                } else {
-                                       self.claim_funds_internal(channel_state.take().unwrap(), HTLCSource::PreviousHopData(htlc_with_hash), payment_preimage);
+                                       self.claim_funds_internal(channel_state.take().unwrap(), HTLCSource::PreviousHopData(htlc.prev_hop), payment_preimage);
                                }
                        }
                        true
@@ -3098,8 +3116,9 @@ impl Writeable for PendingHTLCInfo {
                                onion_packet.write(writer)?;
                                short_channel_id.write(writer)?;
                        },
-                       &PendingHTLCRouting::Receive { } => {
+                       &PendingHTLCRouting::Receive { ref payment_data } => {
                                1u8.write(writer)?;
+                               payment_data.write(writer)?;
                        },
                }
                self.incoming_shared_secret.write(writer)?;
@@ -3118,7 +3137,9 @@ impl Readable for PendingHTLCInfo {
                                        onion_packet: Readable::read(reader)?,
                                        short_channel_id: Readable::read(reader)?,
                                },
-                               1u8 => PendingHTLCRouting::Receive { },
+                               1u8 => PendingHTLCRouting::Receive {
+                                       payment_data: Readable::read(reader)?,
+                               },
                                _ => return Err(DecodeError::InvalidValue),
                        },
                        incoming_shared_secret: Readable::read(reader)?,
@@ -3187,6 +3208,12 @@ impl_writeable!(HTLCPreviousHopData, 0, {
        incoming_packet_shared_secret
 });
 
+impl_writeable!(ClaimableHTLC, 0, {
+       prev_hop,
+       value,
+       payment_data
+});
+
 impl Writeable for HTLCSource {
        fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ::std::io::Error> {
                match self {
@@ -3328,9 +3355,8 @@ impl<ChanSigner: ChannelKeys + Writeable, M: Deref, T: Deref, K: Deref, F: Deref
                for (payment_hash, previous_hops) in channel_state.claimable_htlcs.iter() {
                        payment_hash.write(writer)?;
                        (previous_hops.len() as u64).write(writer)?;
-                       for &(recvd_amt, ref previous_hop) in previous_hops.iter() {
-                               recvd_amt.write(writer)?;
-                               previous_hop.write(writer)?;
+                       for htlc in previous_hops.iter() {
+                               htlc.write(writer)?;
                        }
                }
 
@@ -3507,7 +3533,7 @@ impl<'a, ChanSigner: ChannelKeys + Readable, M: Deref, T: Deref, K: Deref, F: De
                        let previous_hops_len: u64 = Readable::read(reader)?;
                        let mut previous_hops = Vec::with_capacity(cmp::min(previous_hops_len as usize, 2));
                        for _ in 0..previous_hops_len {
-                               previous_hops.push((Readable::read(reader)?, Readable::read(reader)?));
+                               previous_hops.push(Readable::read(reader)?);
                        }
                        claimable_htlcs.insert(payment_hash, previous_hops);
                }
index 85f3026eafa7930d705b9d7cc1b1b8fed3517b39..294dbb965e781aebdfa90bcb00ca4e2fff081755 100644 (file)
@@ -614,6 +614,11 @@ pub trait RoutingMessageHandler : Send + Sync {
 mod fuzzy_internal_msgs {
        // These types aren't intended to be pub, but are exposed for direct fuzzing (as we deserialize
        // them from untrusted input):
+       #[derive(Clone)]
+       pub(crate) struct FinalOnionHopData {
+               pub(crate) payment_secret: [u8; 32],
+               pub(crate) total_msat: u64,
+       }
 
        pub(crate) enum OnionHopDataFormat {
                Legacy { // aka Realm-0
@@ -622,7 +627,9 @@ mod fuzzy_internal_msgs {
                NonFinalNode {
                        short_channel_id: u64,
                },
-               FinalNode,
+               FinalNode {
+                       payment_data: Option<FinalOnionHopData>,
+               },
        }
 
        pub struct OnionHopData {
@@ -965,6 +972,22 @@ impl_writeable!(UpdateAddHTLC, 32+8+8+32+4+1366, {
        onion_routing_packet
 });
 
+impl Writeable for FinalOnionHopData {
+       fn write<W: Writer>(&self, w: &mut W) -> Result<(), ::std::io::Error> {
+               w.size_hint(32 + 8 - (self.total_msat.leading_zeros()/8) as usize);
+               self.payment_secret.write(w)?;
+               HighZeroBytesDroppedVarInt(self.total_msat).write(w)
+       }
+}
+
+impl Readable for FinalOnionHopData {
+       fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
+               let payment_secret = Readable::read(r)?;
+               let amt: HighZeroBytesDroppedVarInt<u64> = Readable::read(r)?;
+               Ok(Self { payment_secret, total_msat: amt.0 })
+       }
+}
+
 impl Writeable for OnionHopData {
        fn write<W: Writer>(&self, w: &mut W) -> Result<(), ::std::io::Error> {
                w.size_hint(33);
@@ -983,7 +1006,14 @@ impl Writeable for OnionHopData {
                                        (6, short_channel_id)
                                });
                        },
-                       OnionHopDataFormat::FinalNode => {
+                       OnionHopDataFormat::FinalNode { payment_data: Some(ref final_data) } => {
+                               encode_varint_length_prefixed_tlv!(w, {
+                                       (2, HighZeroBytesDroppedVarInt(self.amt_to_forward)),
+                                       (4, HighZeroBytesDroppedVarInt(self.outgoing_cltv_value)),
+                                       (8, final_data)
+                               });
+                       },
+                       OnionHopDataFormat::FinalNode { payment_data: None } => {
                                encode_varint_length_prefixed_tlv!(w, {
                                        (2, HighZeroBytesDroppedVarInt(self.amt_to_forward)),
                                        (4, HighZeroBytesDroppedVarInt(self.outgoing_cltv_value))
@@ -1008,19 +1038,24 @@ impl Readable for OnionHopData {
                        let mut amt = HighZeroBytesDroppedVarInt(0u64);
                        let mut cltv_value = HighZeroBytesDroppedVarInt(0u32);
                        let mut short_id: Option<u64> = None;
+                       let mut payment_data: Option<FinalOnionHopData> = None;
                        decode_tlv!(&mut rd, {
                                (2, amt),
                                (4, cltv_value)
                        }, {
-                               (6, short_id)
+                               (6, short_id),
+                               (8, payment_data)
                        });
                        rd.eat_remaining().map_err(|_| DecodeError::ShortRead)?;
                        let format = if let Some(short_channel_id) = short_id {
+                               if payment_data.is_some() { return Err(DecodeError::InvalidValue); }
                                OnionHopDataFormat::NonFinalNode {
                                        short_channel_id,
                                }
                        } else {
-                               OnionHopDataFormat::FinalNode
+                               OnionHopDataFormat::FinalNode {
+                                       payment_data
+                               }
                        };
                        (format, amt.0, cltv_value.0)
                } else {
@@ -1305,7 +1340,7 @@ impl_writeable_len_match!(NodeAnnouncement, {
 mod tests {
        use hex;
        use ln::msgs;
-       use ln::msgs::{ChannelFeatures, InitFeatures, NodeFeatures, OptionalField, OnionErrorPacket, OnionHopDataFormat};
+       use ln::msgs::{ChannelFeatures, FinalOnionHopData, InitFeatures, NodeFeatures, OptionalField, OnionErrorPacket, OnionHopDataFormat};
        use ln::channelmanager::{PaymentPreimage, PaymentHash};
        use util::ser::{Writeable, Readable};
 
@@ -1998,7 +2033,9 @@ mod tests {
        #[test]
        fn encoding_final_onion_hop_data() {
                let mut msg = msgs::OnionHopData {
-                       format: OnionHopDataFormat::FinalNode,
+                       format: OnionHopDataFormat::FinalNode {
+                               payment_data: None,
+                       },
                        amt_to_forward: 0x0badf00d01020304,
                        outgoing_cltv_value: 0xffffffff,
                };
@@ -2006,7 +2043,36 @@ mod tests {
                let target_value = hex::decode("1002080badf00d010203040404ffffffff").unwrap();
                assert_eq!(encoded_value, target_value);
                msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap();
-               if let OnionHopDataFormat::FinalNode = msg.format { } else { panic!(); }
+               if let OnionHopDataFormat::FinalNode { payment_data: None } = msg.format { } else { panic!(); }
+               assert_eq!(msg.amt_to_forward, 0x0badf00d01020304);
+               assert_eq!(msg.outgoing_cltv_value, 0xffffffff);
+       }
+
+       #[test]
+       fn encoding_final_onion_hop_data_with_secret() {
+               let expected_payment_secret = [0x42u8; 32];
+               let mut msg = msgs::OnionHopData {
+                       format: OnionHopDataFormat::FinalNode {
+                               payment_data: Some(FinalOnionHopData {
+                                       payment_secret: expected_payment_secret,
+                                       total_msat: 0x1badca1f
+                               }),
+                       },
+                       amt_to_forward: 0x0badf00d01020304,
+                       outgoing_cltv_value: 0xffffffff,
+               };
+               let encoded_value = msg.encode();
+               let target_value = hex::decode("3602080badf00d010203040404ffffffff082442424242424242424242424242424242424242424242424242424242424242421badca1f").unwrap();
+               assert_eq!(encoded_value, target_value);
+               msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap();
+               if let OnionHopDataFormat::FinalNode {
+                       payment_data: Some(FinalOnionHopData {
+                               payment_secret,
+                               total_msat: 0x1badca1f
+                       })
+               } = msg.format {
+                       assert_eq!(payment_secret, expected_payment_secret);
+               } else { panic!(); }
                assert_eq!(msg.amt_to_forward, 0x0badf00d01020304);
                assert_eq!(msg.outgoing_cltv_value, 0xffffffff);
        }
index c2835460e22b9646f9a659686280283c99f577bf..a97621045370366069bdb5e9b2a3d4ebb68dffff 100644 (file)
@@ -123,7 +123,9 @@ pub(super) fn build_onion_payloads(route: &Route, starting_htlc_offset: u32) ->
                res.insert(0, msgs::OnionHopData {
                        format: if hop.node_features.supports_variable_length_onion() {
                                if idx == 0 {
-                                       msgs::OnionHopDataFormat::FinalNode
+                                       msgs::OnionHopDataFormat::FinalNode {
+                                               payment_data: None,
+                                       }
                                } else {
                                        msgs::OnionHopDataFormat::NonFinalNode {
                                                short_channel_id: last_short_channel_id,