From c94e53d9ddd65816b29af07ac075f542f0f5b37f Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 27 Dec 2019 17:44:46 -0500 Subject: [PATCH] Add support for variable-length onion payload reads using TLV --- fuzz/src/msg_targets/gen_target.sh | 2 +- fuzz/src/msg_targets/mod.rs | 2 +- fuzz/src/msg_targets/msg_onion_hop_data.rs | 2 +- lightning/src/ln/channelmanager.rs | 24 +++- lightning/src/ln/features.rs | 12 +- lightning/src/ln/functional_tests.rs | 4 +- lightning/src/ln/msgs.rs | 155 +++++++++++++++++---- lightning/src/ln/onion_utils.rs | 30 ++-- 8 files changed, 182 insertions(+), 49 deletions(-) diff --git a/fuzz/src/msg_targets/gen_target.sh b/fuzz/src/msg_targets/gen_target.sh index 2f6826ac..0121e4eb 100755 --- a/fuzz/src/msg_targets/gen_target.sh +++ b/fuzz/src/msg_targets/gen_target.sh @@ -34,8 +34,8 @@ GEN_TEST NodeAnnouncement test_msg_exact "" GEN_TEST UpdateAddHTLC test_msg_hole ", 85, 33" GEN_TEST ErrorMessage test_msg_hole ", 32, 2" -GEN_TEST OnionHopData test_msg_hole ", 1+8+8+4, 12" GEN_TEST Init test_msg_simple "" +GEN_TEST OnionHopData test_msg_simple "" GEN_TEST Ping test_msg_simple "" GEN_TEST Pong test_msg_simple "" diff --git a/fuzz/src/msg_targets/mod.rs b/fuzz/src/msg_targets/mod.rs index ef3c489f..69fc4e74 100644 --- a/fuzz/src/msg_targets/mod.rs +++ b/fuzz/src/msg_targets/mod.rs @@ -20,7 +20,7 @@ pub mod msg_channel_update; pub mod msg_node_announcement; pub mod msg_update_add_htlc; pub mod msg_error_message; -pub mod msg_onion_hop_data; pub mod msg_init; +pub mod msg_onion_hop_data; pub mod msg_ping; pub mod msg_pong; diff --git a/fuzz/src/msg_targets/msg_onion_hop_data.rs b/fuzz/src/msg_targets/msg_onion_hop_data.rs index e446a063..9d7ad602 100644 --- a/fuzz/src/msg_targets/msg_onion_hop_data.rs +++ b/fuzz/src/msg_targets/msg_onion_hop_data.rs @@ -7,7 +7,7 @@ use msg_targets::utils::VecWriter; #[inline] pub fn do_test(data: &[u8]) { - test_msg_hole!(msgs::OnionHopData, data, 1+8+8+4, 12); + test_msg_simple!(msgs::OnionHopData, data); } #[no_mangle] diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index fc521b34..36a20ee4 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -910,6 +910,9 @@ impl ChannelManager where M::T Err(err) => { let error_code = match err { msgs::DecodeError::UnknownVersion => 0x4000 | 1, // unknown realm byte + msgs::DecodeError::UnknownRequiredFeature| + msgs::DecodeError::InvalidValue| + msgs::DecodeError::ShortRead => 0x4000 | 22, // invalid_onion_payload _ => 0x2000 | 2, // Should never happen }; return_err!("Unable to decode our hop data", error_code, &[0;0]); @@ -917,7 +920,7 @@ impl ChannelManager where M::T Ok(msg) => { let mut hmac = [0; 32]; if let Err(_) = chacha_stream.read_exact(&mut hmac[..]) { - return_err!("Unable to decode hop data", 0x4000 | 1, &[0;0]); + return_err!("Unable to decode hop data", 0x4000 | 22, &[0;0]); } (msg, hmac) }, @@ -971,6 +974,15 @@ impl ChannelManager where M::T } else { let mut new_packet_data = [0; 20*65]; let read_pos = chacha_stream.read(&mut new_packet_data).unwrap(); + #[cfg(debug_assertions)] + { + // Check two things: + // a) that the behavior of our stream here will return Ok(0) even if the TLV + // read above emptied out our buffer and the unwrap() wont needlessly panic + // b) that we didn't somehow magically end up with extra data. + let mut t = [0; 1]; + debug_assert!(chacha_stream.read(&mut t).unwrap() == 0); + } // Once we've emptied the set of bytes our peer gave us, encrypt 0 bytes until we // fill the onion hop data we'll forward to our next-hop peer. chacha_stream.chacha.process_in_place(&mut new_packet_data[read_pos..]); @@ -995,10 +1007,18 @@ impl ChannelManager where M::T hmac: next_hop_hmac.clone(), }; + let short_channel_id = match next_hop_data.format { + msgs::OnionHopDataFormat::Legacy { short_channel_id } => short_channel_id, + msgs::OnionHopDataFormat::NonFinalNode { short_channel_id } => short_channel_id, + msgs::OnionHopDataFormat::FinalNode => { + return_err!("Final Node OnionHopData provided for us as an intermediary node", 0x4000 | 22, &[0;0]); + }, + }; + PendingHTLCStatus::Forward(PendingForwardHTLCInfo { onion_packet: Some(outgoing_packet), payment_hash: msg.payment_hash.clone(), - short_channel_id: next_hop_data.short_channel_id, + short_channel_id: short_channel_id, incoming_shared_secret: shared_secret, amt_to_forward: next_hop_data.amt_to_forward, outgoing_cltv_value: next_hop_data.outgoing_cltv_value, diff --git a/lightning/src/ln/features.rs b/lightning/src/ln/features.rs index eee534f7..50d8e381 100644 --- a/lightning/src/ln/features.rs +++ b/lightning/src/ln/features.rs @@ -182,13 +182,21 @@ impl Features { pub(crate) fn requires_unknown_bits(&self) -> bool { self.flags.iter().enumerate().any(|(idx, &byte)| { - ( idx != 0 && (byte & 0x55) != 0 ) || ( idx == 0 && (byte & 0x14) != 0 ) + (match idx { + 0 => (byte & 0b00010100), + 1 => (byte & 0b01010100), + _ => (byte & 0b01010101), + }) != 0 }) } pub(crate) fn supports_unknown_bits(&self) -> bool { self.flags.iter().enumerate().any(|(idx, &byte)| { - ( idx != 0 && byte != 0 ) || ( idx == 0 && (byte & 0xc4) != 0 ) + (match idx { + 0 => (byte & 0b11000100), + 1 => (byte & 0b11111100), + _ => byte, + }) != 0 }) } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 936e785f..5c16e727 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -5058,7 +5058,7 @@ fn test_onion_failure() { // describing a length-1 TLV payload, which is obviously bogus. new_payloads[0].data[0] = 1; msg.onion_routing_packet = onion_utils::construct_onion_packet_bogus_hopdata(new_payloads, onion_keys, [0; 32], &payment_hash); - }, ||{}, true, Some(PERM|1), Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: channels[1].0.contents.short_channel_id, is_permanent: true}));//XXX incremented channels idx here + }, ||{}, true, Some(PERM|22), Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: channels[1].0.contents.short_channel_id, is_permanent: true}));//XXX incremented channels idx here // final node failure run_onion_failure_test("invalid_realm", 3, &nodes, &route, &payment_hash, |msg| { @@ -5074,7 +5074,7 @@ fn test_onion_failure() { // length-1 TLV payload, which is obviously bogus. new_payloads[1].data[0] = 1; msg.onion_routing_packet = onion_utils::construct_onion_packet_bogus_hopdata(new_payloads, onion_keys, [0; 32], &payment_hash); - }, ||{}, false, Some(PERM|1), Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: channels[1].0.contents.short_channel_id, is_permanent: true})); + }, ||{}, false, Some(PERM|22), Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: channels[1].0.contents.short_channel_id, is_permanent: true})); // the following three with run_onion_failure_test_with_fail_intercept() test only the origin node // receiving simulated fail messages diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index 09636cef..b50e35b9 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -29,7 +29,7 @@ use std::io::Read; use std::result::Result; use util::events; -use util::ser::{Readable, Writeable, Writer}; +use util::ser::{Readable, Writeable, Writer, FixedLengthReader, HighZeroBytesDroppedVarInt}; use ln::channelmanager::{PaymentPreimage, PaymentHash}; @@ -39,10 +39,11 @@ pub enum DecodeError { /// A version byte specified something we don't know how to handle. /// Includes unknown realm byte in an OnionHopData packet UnknownVersion, - /// Unknown feature mandating we fail to parse message + /// Unknown feature mandating we fail to parse message (eg TLV with an even, unknown type) UnknownRequiredFeature, /// Value was invalid, eg a byte which was supposed to be a bool was something other than a 0 - /// or 1, a public key/private key/signature was invalid, text wasn't UTF-8, etc + /// or 1, a public key/private key/signature was invalid, text wasn't UTF-8, TLV was + /// syntactically incorrect, etc InvalidValue, /// Buffer too short ShortRead, @@ -613,15 +614,20 @@ mod fuzzy_internal_msgs { // them from untrusted input): pub(crate) enum OnionHopDataFormat { - Legacy, // aka Realm-0 + Legacy { // aka Realm-0 + short_channel_id: u64, + }, + NonFinalNode { + short_channel_id: u64, + }, + FinalNode, } pub struct OnionHopData { pub(crate) format: OnionHopDataFormat, - pub(crate) short_channel_id: u64, pub(crate) amt_to_forward: u64, pub(crate) outgoing_cltv_value: u32, - // 12 bytes of 0-padding + // 12 bytes of 0-padding for Legacy format } pub struct DecodedOnionErrorPacket { @@ -962,33 +968,74 @@ impl Writeable for OnionHopData { fn write(&self, w: &mut W) -> Result<(), ::std::io::Error> { w.size_hint(33); match self.format { - OnionHopDataFormat::Legacy => 0u8.write(w)?, + OnionHopDataFormat::Legacy { short_channel_id } => { + 0u8.write(w)?; + short_channel_id.write(w)?; + self.amt_to_forward.write(w)?; + self.outgoing_cltv_value.write(w)?; + w.write_all(&[0;12])?; + }, + OnionHopDataFormat::NonFinalNode { short_channel_id } => { + encode_varint_length_prefixed_tlv!(w, { + (2, HighZeroBytesDroppedVarInt(self.amt_to_forward)), + (4, HighZeroBytesDroppedVarInt(self.outgoing_cltv_value)), + (6, short_channel_id) + }); + }, + OnionHopDataFormat::FinalNode => { + encode_varint_length_prefixed_tlv!(w, { + (2, HighZeroBytesDroppedVarInt(self.amt_to_forward)), + (4, HighZeroBytesDroppedVarInt(self.outgoing_cltv_value)) + }); + }, } - self.short_channel_id.write(w)?; - self.amt_to_forward.write(w)?; - self.outgoing_cltv_value.write(w)?; - w.write_all(&[0;12])?; Ok(()) } } impl Readable for OnionHopData { - fn read(r: &mut R) -> Result { - Ok(OnionHopData { - format: { - let r: u8 = Readable::read(r)?; - if r != 0 { - return Err(DecodeError::UnknownVersion); + fn read(mut r: &mut R) -> Result { + use bitcoin::consensus::encode::{Decodable, Error, VarInt}; + let v: VarInt = Decodable::consensus_decode(&mut r) + .map_err(|e| match e { + Error::Io(ioe) => DecodeError::from(ioe), + _ => DecodeError::InvalidValue + })?; + const LEGACY_ONION_HOP_FLAG: u64 = 0; + let (format, amt, cltv_value) = if v.0 != LEGACY_ONION_HOP_FLAG { + let mut rd = FixedLengthReader::new(r, v.0); + let mut amt = HighZeroBytesDroppedVarInt(0u64); + let mut cltv_value = HighZeroBytesDroppedVarInt(0u32); + let mut short_id: Option = None; + decode_tlv!(&mut rd, { + (2, amt), + (4, cltv_value) + }, { + (6, short_id) + }); + rd.eat_remaining().map_err(|_| DecodeError::ShortRead)?; + let format = if let Some(short_channel_id) = short_id { + OnionHopDataFormat::NonFinalNode { + short_channel_id, } - OnionHopDataFormat::Legacy - }, - short_channel_id: Readable::read(r)?, - amt_to_forward: Readable::read(r)?, - outgoing_cltv_value: { - let v: u32 = Readable::read(r)?; - r.read_exact(&mut [0; 12])?; - v - }, + } else { + OnionHopDataFormat::FinalNode + }; + (format, amt.0, cltv_value.0) + } else { + let format = OnionHopDataFormat::Legacy { + short_channel_id: Readable::read(r)?, + }; + let amt: u64 = Readable::read(r)?; + let cltv_value: u32 = Readable::read(r)?; + r.read_exact(&mut [0; 12])?; + (format, amt, cltv_value) + }; + + Ok(OnionHopData { + format, + amt_to_forward: amt, + outgoing_cltv_value: cltv_value, }) } } @@ -1274,9 +1321,9 @@ impl_writeable_len_match!(NodeAnnouncement, { mod tests { use hex; use ln::msgs; - use ln::msgs::{ChannelFeatures, InitFeatures, NodeFeatures, OptionalField, OnionErrorPacket}; + use ln::msgs::{ChannelFeatures, InitFeatures, NodeFeatures, OptionalField, OnionErrorPacket, OnionHopDataFormat}; use ln::channelmanager::{PaymentPreimage, PaymentHash}; - use util::ser::Writeable; + use util::ser::{Writeable, Readable}; use bitcoin_hashes::sha256d::Hash as Sha256dHash; use bitcoin_hashes::hex::FromHex; @@ -1288,6 +1335,8 @@ mod tests { use secp256k1::key::{PublicKey,SecretKey}; use secp256k1::{Secp256k1, Message}; + use std::io::Cursor; + #[test] fn encoding_channel_reestablish_no_secret() { let cr = msgs::ChannelReestablish { @@ -1927,4 +1976,54 @@ mod tests { let target_value = hex::decode("004000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000").unwrap(); assert_eq!(encoded_value, target_value); } + + #[test] + fn encoding_legacy_onion_hop_data() { + let msg = msgs::OnionHopData { + format: OnionHopDataFormat::Legacy { + short_channel_id: 0xdeadbeef1bad1dea, + }, + amt_to_forward: 0x0badf00d01020304, + outgoing_cltv_value: 0xffffffff, + }; + let encoded_value = msg.encode(); + let target_value = hex::decode("00deadbeef1bad1dea0badf00d01020304ffffffff000000000000000000000000").unwrap(); + assert_eq!(encoded_value, target_value); + } + + #[test] + fn encoding_nonfinal_onion_hop_data() { + let mut msg = msgs::OnionHopData { + format: OnionHopDataFormat::NonFinalNode { + short_channel_id: 0xdeadbeef1bad1dea, + }, + amt_to_forward: 0x0badf00d01020304, + outgoing_cltv_value: 0xffffffff, + }; + let encoded_value = msg.encode(); + let target_value = hex::decode("1a02080badf00d010203040404ffffffff0608deadbeef1bad1dea").unwrap(); + assert_eq!(encoded_value, target_value); + msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap(); + if let OnionHopDataFormat::NonFinalNode { short_channel_id } = msg.format { + assert_eq!(short_channel_id, 0xdeadbeef1bad1dea); + } else { panic!(); } + assert_eq!(msg.amt_to_forward, 0x0badf00d01020304); + assert_eq!(msg.outgoing_cltv_value, 0xffffffff); + } + + #[test] + fn encoding_final_onion_hop_data() { + let mut msg = msgs::OnionHopData { + format: OnionHopDataFormat::FinalNode, + amt_to_forward: 0x0badf00d01020304, + outgoing_cltv_value: 0xffffffff, + }; + let encoded_value = msg.encode(); + 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!(); } + assert_eq!(msg.amt_to_forward, 0x0badf00d01020304); + assert_eq!(msg.outgoing_cltv_value, 0xffffffff); + } } diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index 84e8d979..d1f51738 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -121,8 +121,9 @@ pub(super) fn build_onion_payloads(route: &Route, starting_htlc_offset: u32) -> let value_msat = if cur_value_msat == 0 { hop.fee_msat } else { cur_value_msat }; let cltv = if cur_cltv == starting_htlc_offset { hop.cltv_expiry_delta + starting_htlc_offset } else { cur_cltv }; res.insert(0, msgs::OnionHopData { - format: msgs::OnionHopDataFormat::Legacy, - short_channel_id: last_short_channel_id, + format: msgs::OnionHopDataFormat::Legacy { + short_channel_id: last_short_channel_id, + }, amt_to_forward: value_msat, outgoing_cltv_value: cltv, }); @@ -525,32 +526,37 @@ mod tests { // Test vectors below are flat-out wrong: they claim to set outgoing_cltv_value to non-0 :/ let payloads = vec!( msgs::OnionHopData { - format: msgs::OnionHopDataFormat::Legacy, - short_channel_id: 0, + format: msgs::OnionHopDataFormat::Legacy { + short_channel_id: 0, + }, amt_to_forward: 0, outgoing_cltv_value: 0, }, msgs::OnionHopData { - format: msgs::OnionHopDataFormat::Legacy, - short_channel_id: 0x0101010101010101, + format: msgs::OnionHopDataFormat::Legacy { + short_channel_id: 0x0101010101010101, + }, amt_to_forward: 0x0100000001, outgoing_cltv_value: 0, }, msgs::OnionHopData { - format: msgs::OnionHopDataFormat::Legacy, - short_channel_id: 0x0202020202020202, + format: msgs::OnionHopDataFormat::Legacy { + short_channel_id: 0x0202020202020202, + }, amt_to_forward: 0x0200000002, outgoing_cltv_value: 0, }, msgs::OnionHopData { - format: msgs::OnionHopDataFormat::Legacy, - short_channel_id: 0x0303030303030303, + format: msgs::OnionHopDataFormat::Legacy { + short_channel_id: 0x0303030303030303, + }, amt_to_forward: 0x0300000003, outgoing_cltv_value: 0, }, msgs::OnionHopData { - format: msgs::OnionHopDataFormat::Legacy, - short_channel_id: 0x0404040404040404, + format: msgs::OnionHopDataFormat::Legacy { + short_channel_id: 0x0404040404040404, + }, amt_to_forward: 0x0400000004, outgoing_cltv_value: 0, }, -- 2.30.2