Add support for variable-length onion payload reads using TLV
authorMatt Corallo <git@bluematt.me>
Fri, 27 Dec 2019 22:44:46 +0000 (17:44 -0500)
committerMatt Corallo <git@bluematt.me>
Tue, 11 Feb 2020 21:27:38 +0000 (16:27 -0500)
fuzz/src/msg_targets/gen_target.sh
fuzz/src/msg_targets/mod.rs
fuzz/src/msg_targets/msg_onion_hop_data.rs
lightning/src/ln/channelmanager.rs
lightning/src/ln/features.rs
lightning/src/ln/functional_tests.rs
lightning/src/ln/msgs.rs
lightning/src/ln/onion_utils.rs

index 2f6826ac5381767416a1e47c0dcdb6426e2d92af..0121e4eb821990ef5c104d95f15fe1f13ae212a8 100755 (executable)
@@ -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 ""
index ef3c489f041768721328499c1f120ddc7db07f2d..69fc4e74219b8cf20e81e5cb49937918c596ccf5 100644 (file)
@@ -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;
index e446a063bd9df18bc52c268b57e15c138a6ac6f9..9d7ad602582fafa2a36d5cee16adefabd6d28195 100644 (file)
@@ -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]
index fc521b345a641736d0c72a8f700b75e63a11bfd1..36a20ee4b3e1850c06db2603a4aaeb9acf3872e7 100644 (file)
@@ -910,6 +910,9 @@ impl<ChanSigner: ChannelKeys, M: Deref> ChannelManager<ChanSigner, M> 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<ChanSigner: ChannelKeys, M: Deref> ChannelManager<ChanSigner, M> 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<ChanSigner: ChannelKeys, M: Deref> ChannelManager<ChanSigner, M> 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<ChanSigner: ChannelKeys, M: Deref> ChannelManager<ChanSigner, M> 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,
index eee534f71087215ff1f00e71be32e314ab44d623..50d8e38157871bf40f0ab416cdd28c92eaf1bfb7 100644 (file)
@@ -182,13 +182,21 @@ impl<T: sealed::Context> Features<T> {
 
        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
                })
        }
 
index 936e785f562f9c77815fa57aa4b54b231da3d7fb..5c16e72748198917d93ae7bd956106f94df641f3 100644 (file)
@@ -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
index 09636cef16019239c83061796fe6686abbed9b2b..b50e35b96dc14c62966aab1e8a3c85ca19097bd7 100644 (file)
@@ -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<W: Writer>(&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<R: Read> Readable<R> for OnionHopData {
-       fn read(r: &mut R) -> Result<Self, DecodeError> {
-               Ok(OnionHopData {
-                       format: {
-                               let r: u8 = Readable::read(r)?;
-                               if r != 0 {
-                                       return Err(DecodeError::UnknownVersion);
+       fn read(mut r: &mut R) -> Result<Self, DecodeError> {
+               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<u64> = 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);
+       }
 }
index 84e8d9794e0097eadc4d08c1bf292354a9867390..d1f51738f924fc4056520fa5d2a7cc3f478dbc65 100644 (file)
@@ -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,
                        },