Return a malformed HTLC message when ephemeral pubkey is garbage
authorMatt Corallo <git@bluematt.me>
Sun, 26 Aug 2018 20:35:26 +0000 (16:35 -0400)
committerMatt Corallo <git@bluematt.me>
Mon, 27 Aug 2018 15:47:11 +0000 (11:47 -0400)
This resolves a spec-compliance bug with BOLT 4 where we simply
failed to deserialize the message and thus could never return an
HTLC failure message. However, note that BOLT 4 incorrectly hints
that a non-malformed message should be used ("...MUST report a
route failure to the origin node") which we cannot do as we cannot
derive a SharedSecret to encrypt a regular update_fail_htlc message

fuzz/Cargo.toml
fuzz/fuzz_targets/channel_target.rs
fuzz/fuzz_targets/msg_targets/gen_target.sh
fuzz/fuzz_targets/msg_targets/msg_update_add_htlc_target.rs [deleted file]
fuzz/fuzz_targets/msg_update_add_htlc_target.rs [new file with mode: 0644]
src/ln/channelmanager.rs
src/ln/msgs.rs

index 85ba3d11f3b08c6103e45017b8e75ca23e87949e..48c9064c649aec2e14ad565f2a75870712eae7a6 100644 (file)
@@ -64,6 +64,10 @@ path = "fuzz_targets/msg_pong_target.rs"
 name = "msg_error_message_target"
 path = "fuzz_targets/msg_error_message_target.rs"
 
+[[bin]]
+name = "msg_update_add_htlc_target"
+path = "fuzz_targets/msg_update_add_htlc_target.rs"
+
 [[bin]]
 name = "msg_accept_channel_target"
 path = "fuzz_targets/msg_targets/msg_accept_channel_target.rs"
@@ -100,10 +104,6 @@ path = "fuzz_targets/msg_targets/msg_revoke_and_ack_target.rs"
 name = "msg_shutdown_target"
 path = "fuzz_targets/msg_targets/msg_shutdown_target.rs"
 
-[[bin]]
-name = "msg_update_add_htlc_target"
-path = "fuzz_targets/msg_targets/msg_update_add_htlc_target.rs"
-
 [[bin]]
 name = "msg_update_fail_malformed_htlc_target"
 path = "fuzz_targets/msg_targets/msg_update_fail_malformed_htlc_target.rs"
index 04a3caccadcb85d08389acede83112729c63d6d0..ed5fa13b09cef6055629b5824a80534a19a967ea 100644 (file)
@@ -269,7 +269,7 @@ pub fn do_test(data: &[u8]) {
                        0 => {
                                test_err!(channel.send_htlc(slice_to_be64(get_slice!(8)), [42; 32], slice_to_be32(get_slice!(4)), msgs::OnionPacket {
                                        version: get_slice!(1)[0],
-                                       public_key: get_pubkey!(),
+                                       public_key: PublicKey::from_slice(&secp_ctx, get_slice!(33)),
                                        hop_data: [0; 20*65],
                                        hmac: [0; 32],
                                }));
index 4a5dc2fc5565d9c25739c97b01faf512c5f74e72..249c0ce1f346fee4e21553369caff42b032c1b5c 100755 (executable)
@@ -1,4 +1,4 @@
-for target in CommitmentSigned FundingCreated FundingLocked FundingSigned OpenChannel RevokeAndACK Shutdown UpdateAddHTLC UpdateFailHTLC UpdateFailMalformedHTLC UpdateFee UpdateFulfillHTLC AcceptChannel ClosingSigned ChannelReestablish; do
+for target in CommitmentSigned FundingCreated FundingLocked FundingSigned OpenChannel RevokeAndACK Shutdown UpdateFailHTLC UpdateFailMalformedHTLC UpdateFee UpdateFulfillHTLC AcceptChannel ClosingSigned ChannelReestablish; do
        tn=$(echo $target | sed 's/\([a-z0-9]\)\([A-Z]\)/\1_\2/g')
        fn=msg_$(echo $tn | tr '[:upper:]' '[:lower:]')_target.rs
        cat msg_target_template.txt | sed s/MSG_TARGET/$target/ > $fn
diff --git a/fuzz/fuzz_targets/msg_targets/msg_update_add_htlc_target.rs b/fuzz/fuzz_targets/msg_targets/msg_update_add_htlc_target.rs
deleted file mode 100644 (file)
index 07a0f81..0000000
+++ /dev/null
@@ -1,46 +0,0 @@
-// This file is auto-generated by gen_target.sh based on msg_target_template.txt
-// To modify it, modify msg_target_template.txt and run gen_target.sh instead.
-
-extern crate lightning;
-
-use lightning::ln::msgs;
-use lightning::util::reset_rng_state;
-
-use lightning::ln::msgs::{MsgEncodable, MsgDecodable};
-
-mod utils;
-
-#[inline]
-pub fn do_test(data: &[u8]) {
-       reset_rng_state();
-       test_msg!(msgs::UpdateAddHTLC, data);
-}
-
-#[cfg(feature = "afl")]
-#[macro_use] extern crate afl;
-#[cfg(feature = "afl")]
-fn main() {
-       fuzz!(|data| {
-               do_test(data);
-       });
-}
-
-#[cfg(feature = "honggfuzz")]
-#[macro_use] extern crate honggfuzz;
-#[cfg(feature = "honggfuzz")]
-fn main() {
-       loop {
-               fuzz!(|data| {
-                       do_test(data);
-               });
-       }
-}
-
-extern crate hex;
-#[cfg(test)]
-mod tests {
-       #[test]
-       fn duplicate_crash() {
-               super::do_test(&::hex::decode("00").unwrap());
-       }
-}
diff --git a/fuzz/fuzz_targets/msg_update_add_htlc_target.rs b/fuzz/fuzz_targets/msg_update_add_htlc_target.rs
new file mode 100644 (file)
index 0000000..5616047
--- /dev/null
@@ -0,0 +1,45 @@
+extern crate lightning;
+
+use lightning::ln::msgs;
+use lightning::util::reset_rng_state;
+
+use lightning::ln::msgs::{MsgEncodable, MsgDecodable};
+
+#[inline]
+pub fn do_test(data: &[u8]) {
+       reset_rng_state();
+       if let Ok(msg) = msgs::UpdateAddHTLC::decode(data){
+               let enc = msg.encode();
+               assert_eq!(&data[0..85], &enc[0..85]);
+               assert_eq!(&data[85+33..enc.len()], &enc[85+33..]);
+       }
+}
+
+#[cfg(feature = "afl")]
+#[macro_use] extern crate afl;
+#[cfg(feature = "afl")]
+fn main() {
+       fuzz!(|data| {
+               do_test(data);
+       });
+}
+
+#[cfg(feature = "honggfuzz")]
+#[macro_use] extern crate honggfuzz;
+#[cfg(feature = "honggfuzz")]
+fn main() {
+       loop {
+               fuzz!(|data| {
+                       do_test(data);
+               });
+       }
+}
+
+extern crate hex;
+#[cfg(test)]
+mod tests {
+       #[test]
+       fn duplicate_crash() {
+               super::do_test(&::hex::decode("00").unwrap());
+       }
+}
index 88eb4b126aeeb7cadd5c574aac92b518643bb4ec..6275713b589043142626fd2cbd582b0a3e456cee 100644 (file)
@@ -625,7 +625,7 @@ impl ChannelManager {
 
                Ok(msgs::OnionPacket{
                        version: 0,
-                       public_key: onion_keys.first().unwrap().ephemeral_pubkey,
+                       public_key: Ok(onion_keys.first().unwrap().ephemeral_pubkey),
                        hop_data: packet_data,
                        hmac: hmac_res,
                })
@@ -681,10 +681,7 @@ impl ChannelManager {
                ChannelManager::encrypt_failure_packet(shared_secret, &failure_packet.encode()[..])
        }
 
-       fn decode_update_add_htlc_onion(&self, msg: &msgs::UpdateAddHTLC) -> (PendingHTLCStatus, SharedSecret, MutexGuard<ChannelHolder>) {
-               let shared_secret = SharedSecret::new(&self.secp_ctx, &msg.onion_routing_packet.public_key, &self.our_network_key);
-               let (rho, mu) = ChannelManager::gen_rho_mu_from_shared_secret(&shared_secret);
-
+       fn decode_update_add_htlc_onion(&self, msg: &msgs::UpdateAddHTLC) -> (PendingHTLCStatus, Option<SharedSecret>, MutexGuard<ChannelHolder>) {
                macro_rules! get_onion_hash {
                        () => {
                                {
@@ -697,6 +694,19 @@ impl ChannelManager {
                        }
                }
 
+               if let Err(_) = msg.onion_routing_packet.public_key {
+                       log_info!(self, "Failed to accept/forward incoming HTLC with invalid ephemeral pubkey");
+                       return (PendingHTLCStatus::Fail(HTLCFailureMsg::Malformed(msgs::UpdateFailMalformedHTLC {
+                               channel_id: msg.channel_id,
+                               htlc_id: msg.htlc_id,
+                               sha256_of_onion: get_onion_hash!(),
+                               failure_code: 0x8000 | 0x4000 | 6,
+                       })), None, self.channel_state.lock().unwrap());
+               }
+
+               let shared_secret = SharedSecret::new(&self.secp_ctx, &msg.onion_routing_packet.public_key.unwrap(), &self.our_network_key);
+               let (rho, mu) = ChannelManager::gen_rho_mu_from_shared_secret(&shared_secret);
+
                let mut channel_state = None;
                macro_rules! return_err {
                        ($msg: expr, $err_code: expr, $data: expr) => {
@@ -709,7 +719,7 @@ impl ChannelManager {
                                                channel_id: msg.channel_id,
                                                htlc_id: msg.htlc_id,
                                                reason: ChannelManager::build_first_hop_failure_packet(&shared_secret, $err_code, $data),
-                                       })), shared_secret, channel_state.unwrap());
+                                       })), Some(shared_secret), channel_state.unwrap());
                                }
                        }
                }
@@ -776,7 +786,7 @@ impl ChannelManager {
                                chacha.process(&msg.onion_routing_packet.hop_data[65..], &mut new_packet_data[0..19*65]);
                                chacha.process(&ChannelManager::ZERO[0..65], &mut new_packet_data[19*65..]);
 
-                               let mut new_pubkey = msg.onion_routing_packet.public_key.clone();
+                               let mut new_pubkey = msg.onion_routing_packet.public_key.unwrap();
 
                                let blinding_factor = {
                                        let mut sha = Sha256::new();
@@ -786,26 +796,19 @@ impl ChannelManager {
                                        sha.result(&mut res);
                                        match SecretKey::from_slice(&self.secp_ctx, &res) {
                                                Err(_) => {
-                                                       // Return temporary node failure as its technically our issue, not the
-                                                       // channel's issue.
-                                                       return_err!("Blinding factor is an invalid private key", 0x2000 | 2, &[0;0]);
+                                                       return_err!("Blinding factor is an invalid private key", 0x8000 | 0x4000 | 6, &get_onion_hash!());
                                                },
                                                Ok(key) => key
                                        }
                                };
 
-                               match new_pubkey.mul_assign(&self.secp_ctx, &blinding_factor) {
-                                       Err(_) => {
-                                               // Return temporary node failure as its technically our issue, not the
-                                               // channel's issue.
-                                               return_err!("New blinding factor is an invalid private key", 0x2000 | 2, &[0;0]);
-                                       },
-                                       Ok(_) => {}
-                               };
+                               if let Err(_) = new_pubkey.mul_assign(&self.secp_ctx, &blinding_factor) {
+                                       return_err!("New blinding factor is an invalid private key", 0x8000 | 0x4000 | 6, &get_onion_hash!());
+                               }
 
                                let outgoing_packet = msgs::OnionPacket {
                                        version: 0,
-                                       public_key: new_pubkey,
+                                       public_key: Ok(new_pubkey),
                                        hop_data: new_packet_data,
                                        hmac: next_hop_data.hmac.clone(),
                                };
@@ -852,7 +855,7 @@ impl ChannelManager {
                        }
                }
 
-               (pending_forward_info, shared_secret, channel_state.unwrap())
+               (pending_forward_info, Some(shared_secret), channel_state.unwrap())
        }
 
        /// only fails if the channel does not yet have an assigned short_id
@@ -1735,7 +1738,7 @@ impl ChannelMessageHandler for ChannelManager {
                                        pending_forward_info = PendingHTLCStatus::Fail(HTLCFailureMsg::Relay(msgs::UpdateFailHTLC {
                                                channel_id: msg.channel_id,
                                                htlc_id: msg.htlc_id,
-                                               reason: ChannelManager::build_first_hop_failure_packet(&shared_secret, 0x4000 | 0x2000 | 2, &[0;0]),
+                                               reason: ChannelManager::build_first_hop_failure_packet(&shared_secret.unwrap(), 0x4000 | 0x2000 | 2, &[0;0]),
                                        }));
                                } else {
                                        will_forward = true;
@@ -1774,7 +1777,7 @@ impl ChannelMessageHandler for ChannelManager {
                                        };
                                        *outbound_route = PendingOutboundHTLC::CycledRoute {
                                                source_short_channel_id,
-                                               incoming_packet_shared_secret: shared_secret,
+                                               incoming_packet_shared_secret: shared_secret.unwrap(),
                                                route,
                                                session_priv,
                                        };
@@ -1782,7 +1785,7 @@ impl ChannelMessageHandler for ChannelManager {
                                hash_map::Entry::Vacant(e) => {
                                        e.insert(PendingOutboundHTLC::IntermediaryHopData {
                                                source_short_channel_id,
-                                               incoming_packet_shared_secret: shared_secret,
+                                               incoming_packet_shared_secret: shared_secret.unwrap(),
                                        });
                                }
                        }
index 6c7c8cfc7281682af919360df1c892342277e820..4067e84b98c18bff1d186b885a5dffbdbbdc06f8 100644 (file)
@@ -1,5 +1,6 @@
 use secp256k1::key::PublicKey;
 use secp256k1::{Secp256k1, Signature};
+use secp256k1;
 use bitcoin::util::hash::Sha256dHash;
 use bitcoin::network::serialize::{deserialize,serialize};
 use bitcoin::blockdata::script::Script;
@@ -476,7 +477,10 @@ unsafe impl internal_traits::NoDealloc for OnionHopData{}
 #[derive(Clone)]
 pub struct OnionPacket {
        pub version: u8,
-       pub public_key: PublicKey,
+       /// In order to ensure we always return an error on Onion decode in compliance with BOLT 4, we
+       /// have to deserialize OnionPackets contained in UpdateAddHTLCs even if the ephemeral public
+       /// key (here) is bogus, so we hold a Result instead of a PublicKey as we'd like.
+       pub public_key: Result<PublicKey, secp256k1::Error>,
        pub hop_data: [u8; 20*65],
        pub hmac: [u8; 32],
 }
@@ -1538,7 +1542,7 @@ impl MsgDecodable for OnionPacket {
                let secp_ctx = Secp256k1::without_caps();
                Ok(Self {
                        version: v[0],
-                       public_key: secp_pubkey!(&secp_ctx, &v[1..34]),
+                       public_key: PublicKey::from_slice(&secp_ctx, &v[1..34]),
                        hop_data,
                        hmac,
                })
@@ -1548,7 +1552,10 @@ impl MsgEncodable for OnionPacket {
        fn encode(&self) -> Vec<u8> {
                let mut res = Vec::with_capacity(1 + 33 + 20*65 + 32);
                res.push(self.version);
-               res.extend_from_slice(&self.public_key.serialize());
+               match self.public_key {
+                       Ok(pubkey) => res.extend_from_slice(&pubkey.serialize()),
+                       Err(_) => res.extend_from_slice(&[0; 33]),
+               }
                res.extend_from_slice(&self.hop_data);
                res.extend_from_slice(&self.hmac);
                res