Skip to content

Commit 53a3c62

Browse files
TheBlueMattTheBlueMatt
committed
Add support for variable-length onion payload reads using TLV
1 parent c326061 commit 53a3c62

File tree

8 files changed

+180
-48
lines changed

8 files changed

+180
-48
lines changed

‎fuzz/src/msg_targets/gen_target.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,8 @@ GEN_TEST NodeAnnouncement test_msg_exact ""
3434

3535
GEN_TEST UpdateAddHTLC test_msg_hole ", 85, 33"
3636
GEN_TEST ErrorMessage test_msg_hole ", 32, 2"
37-
GEN_TEST OnionHopData test_msg_hole ", 1+8+8+4, 12"
3837

3938
GEN_TEST Init test_msg_simple ""
39+
GEN_TEST OnionHopData test_msg_simple ""
4040
GEN_TEST Ping test_msg_simple ""
4141
GEN_TEST Pong test_msg_simple ""

‎fuzz/src/msg_targets/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ pub mod msg_channel_update;
2020
pub mod msg_node_announcement;
2121
pub mod msg_update_add_htlc;
2222
pub mod msg_error_message;
23-
pub mod msg_onion_hop_data;
2423
pub mod msg_init;
24+
pub mod msg_onion_hop_data;
2525
pub mod msg_ping;
2626
pub mod msg_pong;

‎fuzz/src/msg_targets/msg_onion_hop_data.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ use msg_targets::utils::VecWriter;
77

88
#[inline]
99
pub fn do_test(data: &[u8]) {
10-
test_msg_hole!(msgs::OnionHopData, data, 1+8+8+4, 12);
10+
test_msg_simple!(msgs::OnionHopData, data);
1111
}
1212

1313
#[no_mangle]

‎lightning/src/ln/channelmanager.rs

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -910,14 +910,17 @@ impl<ChanSigner: ChannelKeys, M: Deref> ChannelManager<ChanSigner, M> where M::T
910910
Err(err) => {
911911
let error_code = match err {
912912
msgs::DecodeError::UnknownVersion => 0x4000 | 1, // unknown realm byte
913+
msgs::DecodeError::UnknownRequiredFeature|
914+
msgs::DecodeError::InvalidValue|
915+
msgs::DecodeError::ShortRead => 0x4000 | 22, // invalid_onion_payload
913916
_ => 0x2000 | 2, // Should never happen
914917
};
915918
return_err!("Unable to decode our hop data", error_code, &[0;0]);
916919
},
917920
Ok(msg) => {
918921
let mut hmac = [0; 32];
919922
if let Err(_) = chacha_stream.read_exact(&mut hmac[..]) {
920-
return_err!("Unable to decode hop data", 0x4000 | 1, &[0;0]);
923+
return_err!("Unable to decode hop data", 0x4000 | 22, &[0;0]);
921924
}
922925
(msg, hmac)
923926
},
@@ -971,6 +974,15 @@ impl<ChanSigner: ChannelKeys, M: Deref> ChannelManager<ChanSigner, M> where M::T
971974
} else {
972975
let mut new_packet_data = [0; 20*65];
973976
let read_pos = chacha_stream.read(&mut new_packet_data).unwrap();
977+
#[cfg(debug_assertions)]
978+
{
979+
// Check two things:
980+
// a) that the behavior of our stream here will return Ok(0) even if the TLV
981+
// read above emptied out our buffer and the unwrap() wont needlessly panic
982+
// b) that we didn't somehow magically end up with extra data.
983+
let mut t = [0; 1];
984+
debug_assert!(chacha_stream.read(&mut t).unwrap() == 0);
985+
}
974986
// Once we've emptied the set of bytes our peer gave us, encrypt 0 bytes until we
975987
// fill the onion hop data we'll forward to our next-hop peer.
976988
chacha_stream.chacha.process_in_place(&mut new_packet_data[read_pos..]);
@@ -995,10 +1007,18 @@ impl<ChanSigner: ChannelKeys, M: Deref> ChannelManager<ChanSigner, M> where M::T
9951007
hmac: next_hop_hmac.clone(),
9961008
};
9971009

1010+
let short_channel_id = match next_hop_data.format {
1011+
msgs::OnionHopDataFormat::Legacy { short_channel_id } => short_channel_id,
1012+
msgs::OnionHopDataFormat::NonFinalNode { short_channel_id } => short_channel_id,
1013+
msgs::OnionHopDataFormat::FinalNode => {
1014+
return_err!("Final Node OnionHopData provided for us as an intermediary node", 0x4000 | 22, &[0;0]);
1015+
},
1016+
};
1017+
9981018
PendingHTLCStatus::Forward(PendingForwardHTLCInfo {
9991019
onion_packet: Some(outgoing_packet),
10001020
payment_hash: msg.payment_hash.clone(),
1001-
short_channel_id: next_hop_data.short_channel_id,
1021+
short_channel_id: short_channel_id,
10021022
incoming_shared_secret: shared_secret,
10031023
amt_to_forward: next_hop_data.amt_to_forward,
10041024
outgoing_cltv_value: next_hop_data.outgoing_cltv_value,

‎lightning/src/ln/features.rs

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -182,13 +182,21 @@ impl<T: sealed::Context> Features<T> {
182182

183183
pub(crate) fn requires_unknown_bits(&self) -> bool {
184184
self.flags.iter().enumerate().any(|(idx, &byte)| {
185-
( idx != 0 && (byte & 0x55) != 0 ) || ( idx == 0 && (byte & 0x14) != 0 )
185+
(match idx {
186+
0 => (byte & 0b00010100),
187+
1 => (byte & 0b01010100),
188+
_ => (byte & 0b01010101),
189+
}) != 0
186190
})
187191
}
188192

189193
pub(crate) fn supports_unknown_bits(&self) -> bool {
190194
self.flags.iter().enumerate().any(|(idx, &byte)| {
191-
( idx != 0 && byte != 0 ) || ( idx == 0 && (byte & 0xc4) != 0 )
195+
(match idx {
196+
0 => (byte & 0b11000100),
197+
1 => (byte & 0b11111100),
198+
_ => byte,
199+
}) != 0
192200
})
193201
}
194202

‎lightning/src/ln/functional_tests.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5058,7 +5058,7 @@ fn test_onion_failure() {
50585058
// describing a length-1 TLV payload, which is obviously bogus.
50595059
new_payloads[0].data[0] = 1;
50605060
msg.onion_routing_packet = onion_utils::construct_onion_packet_bogus_hopdata(new_payloads, onion_keys, [0; 32], &payment_hash);
5061-
}, ||{}, 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
5061+
}, ||{}, 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
50625062

50635063
// final node failure
50645064
run_onion_failure_test("invalid_realm", 3, &nodes, &route, &payment_hash, |msg| {
@@ -5074,7 +5074,7 @@ fn test_onion_failure() {
50745074
// length-1 TLV payload, which is obviously bogus.
50755075
new_payloads[1].data[0] = 1;
50765076
msg.onion_routing_packet = onion_utils::construct_onion_packet_bogus_hopdata(new_payloads, onion_keys, [0; 32], &payment_hash);
5077-
}, ||{}, false, Some(PERM|1), Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: channels[1].0.contents.short_channel_id, is_permanent: true}));
5077+
}, ||{}, false, Some(PERM|22), Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: channels[1].0.contents.short_channel_id, is_permanent: true}));
50785078

50795079
// the following three with run_onion_failure_test_with_fail_intercept() test only the origin node
50805080
// receiving simulated fail messages

‎lightning/src/ln/msgs.rs

Lines changed: 125 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ use std::io::Read;
2929
use std::result::Result;
3030

3131
use util::events;
32-
use util::ser::{Readable, Writeable, Writer};
32+
use util::ser::{Readable, Writeable, Writer, FixedLengthReader, HighZeroBytesDroppedVarInt};
3333

3434
use ln::channelmanager::{PaymentPreimage, PaymentHash};
3535

@@ -39,7 +39,7 @@ pub enum DecodeError {
3939
/// A version byte specified something we don't know how to handle.
4040
/// Includes unknown realm byte in an OnionHopData packet
4141
UnknownVersion,
42-
/// Unknown feature mandating we fail to parse message
42+
/// Unknown feature mandating we fail to parse message (eg TLV with an even, unknown type)
4343
UnknownRequiredFeature,
4444
/// Value was invalid, eg a byte which was supposed to be a bool was something other than a 0
4545
/// or 1, a public key/private key/signature was invalid, text wasn't UTF-8, etc
@@ -613,15 +613,20 @@ mod fuzzy_internal_msgs {
613613
// them from untrusted input):
614614

615615
pub(crate) enum OnionHopDataFormat {
616-
Legacy, // aka Realm-0
616+
Legacy { // aka Realm-0
617+
short_channel_id: u64,
618+
},
619+
NonFinalNode {
620+
short_channel_id: u64,
621+
},
622+
FinalNode,
617623
}
618624

619625
pub struct OnionHopData {
620626
pub(crate) format: OnionHopDataFormat,
621-
pub(crate) short_channel_id: u64,
622627
pub(crate) amt_to_forward: u64,
623628
pub(crate) outgoing_cltv_value: u32,
624-
// 12 bytes of 0-padding
629+
// 12 bytes of 0-padding for Legacy format
625630
}
626631

627632
pub struct DecodedOnionErrorPacket {
@@ -962,33 +967,74 @@ impl Writeable for OnionHopData {
962967
fn write<W: Writer>(&self, w: &mut W) -> Result<(), ::std::io::Error> {
963968
w.size_hint(33);
964969
match self.format {
965-
OnionHopDataFormat::Legacy => 0u8.write(w)?,
970+
OnionHopDataFormat::Legacy { short_channel_id } => {
971+
0u8.write(w)?;
972+
short_channel_id.write(w)?;
973+
self.amt_to_forward.write(w)?;
974+
self.outgoing_cltv_value.write(w)?;
975+
w.write_all(&[0;12])?;
976+
},
977+
OnionHopDataFormat::NonFinalNode { short_channel_id } => {
978+
encode_varint_length_prefixed_tlv!(w, {
979+
(2, HighZeroBytesDroppedVarInt(self.amt_to_forward)),
980+
(4, HighZeroBytesDroppedVarInt(self.outgoing_cltv_value)),
981+
(6, short_channel_id)
982+
});
983+
},
984+
OnionHopDataFormat::FinalNode => {
985+
encode_varint_length_prefixed_tlv!(w, {
986+
(2, HighZeroBytesDroppedVarInt(self.amt_to_forward)),
987+
(4, HighZeroBytesDroppedVarInt(self.outgoing_cltv_value))
988+
});
989+
},
966990
}
967-
self.short_channel_id.write(w)?;
968-
self.amt_to_forward.write(w)?;
969-
self.outgoing_cltv_value.write(w)?;
970-
w.write_all(&[0;12])?;
971991
Ok(())
972992
}
973993
}
974994

975995
impl<R: Read> Readable<R> for OnionHopData {
976-
fn read(r: &mut R) -> Result<Self, DecodeError> {
977-
Ok(OnionHopData {
978-
format: {
979-
let r: u8 = Readable::read(r)?;
980-
if r != 0 {
981-
return Err(DecodeError::UnknownVersion);
996+
fn read(mut r: &mut R) -> Result<Self, DecodeError> {
997+
use bitcoin::consensus::encode::{Decodable, Error, VarInt};
998+
let v: VarInt = Decodable::consensus_decode(&mut r)
999+
.map_err(|e| match e {
1000+
Error::Io(ioe) => DecodeError::from(ioe),
1001+
_ => DecodeError::InvalidValue
1002+
})?;
1003+
const LEGACY_ONION_HOP_FLAG: u64 = 0;
1004+
let (format, amt, cltv_value) = if v.0 != LEGACY_ONION_HOP_FLAG {
1005+
let mut rd = FixedLengthReader::new(r, v.0);
1006+
let mut amt = HighZeroBytesDroppedVarInt(0u64);
1007+
let mut cltv_value = HighZeroBytesDroppedVarInt(0u32);
1008+
let mut short_id: Option<u64> = None;
1009+
decode_tlv!(&mut rd, {
1010+
(2, amt),
1011+
(4, cltv_value)
1012+
}, {
1013+
(6, short_id)
1014+
});
1015+
rd.eat_remaining().map_err(|_| DecodeError::ShortRead)?;
1016+
let format = if let Some(short_channel_id) = short_id {
1017+
OnionHopDataFormat::NonFinalNode {
1018+
short_channel_id,
9821019
}
983-
OnionHopDataFormat::Legacy
984-
},
985-
short_channel_id: Readable::read(r)?,
986-
amt_to_forward: Readable::read(r)?,
987-
outgoing_cltv_value: {
988-
let v: u32 = Readable::read(r)?;
989-
r.read_exact(&mut [0; 12])?;
990-
v
991-
},
1020+
} else {
1021+
OnionHopDataFormat::FinalNode
1022+
};
1023+
(format, amt.0, cltv_value.0)
1024+
} else {
1025+
let format = OnionHopDataFormat::Legacy {
1026+
short_channel_id: Readable::read(r)?,
1027+
};
1028+
let amt: u64 = Readable::read(r)?;
1029+
let cltv_value: u32 = Readable::read(r)?;
1030+
r.read_exact(&mut [0; 12])?;
1031+
(format, amt, cltv_value)
1032+
};
1033+
1034+
Ok(OnionHopData {
1035+
format,
1036+
amt_to_forward: amt,
1037+
outgoing_cltv_value: cltv_value,
9921038
})
9931039
}
9941040
}
@@ -1274,9 +1320,9 @@ impl_writeable_len_match!(NodeAnnouncement, {
12741320
mod tests {
12751321
use hex;
12761322
use ln::msgs;
1277-
use ln::msgs::{ChannelFeatures, InitFeatures, NodeFeatures, OptionalField, OnionErrorPacket};
1323+
use ln::msgs::{ChannelFeatures, InitFeatures, NodeFeatures, OptionalField, OnionErrorPacket, OnionHopDataFormat};
12781324
use ln::channelmanager::{PaymentPreimage, PaymentHash};
1279-
use util::ser::Writeable;
1325+
use util::ser::{Writeable, Readable};
12801326

12811327
use bitcoin_hashes::sha256d::Hash as Sha256dHash;
12821328
use bitcoin_hashes::hex::FromHex;
@@ -1288,6 +1334,8 @@ mod tests {
12881334
use secp256k1::key::{PublicKey,SecretKey};
12891335
use secp256k1::{Secp256k1, Message};
12901336

1337+
use std::io::Cursor;
1338+
12911339
#[test]
12921340
fn encoding_channel_reestablish_no_secret() {
12931341
let cr = msgs::ChannelReestablish {
@@ -1927,4 +1975,54 @@ mod tests {
19271975
let target_value = hex::decode("004000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000").unwrap();
19281976
assert_eq!(encoded_value, target_value);
19291977
}
1978+
1979+
#[test]
1980+
fn encoding_legacy_onion_hop_data() {
1981+
let msg = msgs::OnionHopData {
1982+
format: OnionHopDataFormat::Legacy {
1983+
short_channel_id: 0xdeadbeef1bad1dea,
1984+
},
1985+
amt_to_forward: 0x0badf00d01020304,
1986+
outgoing_cltv_value: 0xffffffff,
1987+
};
1988+
let encoded_value = msg.encode();
1989+
let target_value = hex::decode("00deadbeef1bad1dea0badf00d01020304ffffffff000000000000000000000000").unwrap();
1990+
assert_eq!(encoded_value, target_value);
1991+
}
1992+
1993+
#[test]
1994+
fn encoding_nonfinal_onion_hop_data() {
1995+
let mut msg = msgs::OnionHopData {
1996+
format: OnionHopDataFormat::NonFinalNode {
1997+
short_channel_id: 0xdeadbeef1bad1dea,
1998+
},
1999+
amt_to_forward: 0x0badf00d01020304,
2000+
outgoing_cltv_value: 0xffffffff,
2001+
};
2002+
let encoded_value = msg.encode();
2003+
let target_value = hex::decode("1a02080badf00d010203040404ffffffff0608deadbeef1bad1dea").unwrap();
2004+
assert_eq!(encoded_value, target_value);
2005+
msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap();
2006+
if let OnionHopDataFormat::NonFinalNode { short_channel_id } = msg.format {
2007+
assert_eq!(short_channel_id, 0xdeadbeef1bad1dea);
2008+
} else { panic!(); }
2009+
assert_eq!(msg.amt_to_forward, 0x0badf00d01020304);
2010+
assert_eq!(msg.outgoing_cltv_value, 0xffffffff);
2011+
}
2012+
2013+
#[test]
2014+
fn encoding_final_onion_hop_data() {
2015+
let mut msg = msgs::OnionHopData {
2016+
format: OnionHopDataFormat::FinalNode,
2017+
amt_to_forward: 0x0badf00d01020304,
2018+
outgoing_cltv_value: 0xffffffff,
2019+
};
2020+
let encoded_value = msg.encode();
2021+
let target_value = hex::decode("1002080badf00d010203040404ffffffff").unwrap();
2022+
assert_eq!(encoded_value, target_value);
2023+
msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap();
2024+
if let OnionHopDataFormat::FinalNode = msg.format { } else { panic!(); }
2025+
assert_eq!(msg.amt_to_forward, 0x0badf00d01020304);
2026+
assert_eq!(msg.outgoing_cltv_value, 0xffffffff);
2027+
}
19302028
}

‎lightning/src/ln/onion_utils.rs

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -121,8 +121,9 @@ pub(super) fn build_onion_payloads(route: &Route, starting_htlc_offset: u32) ->
121121
let value_msat = if cur_value_msat == 0 { hop.fee_msat } else { cur_value_msat };
122122
let cltv = if cur_cltv == starting_htlc_offset { hop.cltv_expiry_delta + starting_htlc_offset } else { cur_cltv };
123123
res.insert(0, msgs::OnionHopData {
124-
format: msgs::OnionHopDataFormat::Legacy,
125-
short_channel_id: last_short_channel_id,
124+
format: msgs::OnionHopDataFormat::Legacy {
125+
short_channel_id: last_short_channel_id,
126+
},
126127
amt_to_forward: value_msat,
127128
outgoing_cltv_value: cltv,
128129
});
@@ -525,32 +526,37 @@ mod tests {
525526
// Test vectors below are flat-out wrong: they claim to set outgoing_cltv_value to non-0 :/
526527
let payloads = vec!(
527528
msgs::OnionHopData {
528-
format: msgs::OnionHopDataFormat::Legacy,
529-
short_channel_id: 0,
529+
format: msgs::OnionHopDataFormat::Legacy {
530+
short_channel_id: 0,
531+
},
530532
amt_to_forward: 0,
531533
outgoing_cltv_value: 0,
532534
},
533535
msgs::OnionHopData {
534-
format: msgs::OnionHopDataFormat::Legacy,
535-
short_channel_id: 0x0101010101010101,
536+
format: msgs::OnionHopDataFormat::Legacy {
537+
short_channel_id: 0x0101010101010101,
538+
},
536539
amt_to_forward: 0x0100000001,
537540
outgoing_cltv_value: 0,
538541
},
539542
msgs::OnionHopData {
540-
format: msgs::OnionHopDataFormat::Legacy,
541-
short_channel_id: 0x0202020202020202,
543+
format: msgs::OnionHopDataFormat::Legacy {
544+
short_channel_id: 0x0202020202020202,
545+
},
542546
amt_to_forward: 0x0200000002,
543547
outgoing_cltv_value: 0,
544548
},
545549
msgs::OnionHopData {
546-
format: msgs::OnionHopDataFormat::Legacy,
547-
short_channel_id: 0x0303030303030303,
550+
format: msgs::OnionHopDataFormat::Legacy {
551+
short_channel_id: 0x0303030303030303,
552+
},
548553
amt_to_forward: 0x0300000003,
549554
outgoing_cltv_value: 0,
550555
},
551556
msgs::OnionHopData {
552-
format: msgs::OnionHopDataFormat::Legacy,
553-
short_channel_id: 0x0404040404040404,
557+
format: msgs::OnionHopDataFormat::Legacy {
558+
short_channel_id: 0x0404040404040404,
559+
},
554560
amt_to_forward: 0x0400000004,
555561
outgoing_cltv_value: 0,
556562
},

0 commit comments

Comments
 (0)