Skip to content

Commit 26fe879

Browse files
valentinewallacevalentinewallace
committed
Correctly wrap phantom onion errors
In any place where fail_htlc_backwards_internal was called for a phantom payment failure, we weren't encoding the onion failure as if the phantom were the one failing. Instead, we were encoding the failure as if it were coming from the second-to-last hop. This caused our failures to not be parsed properly on the payer's side. Places we were encoding failures incorrectly include: * on failure of a call to inbound_payment::verify * on a user call to fail_htlc_backwards Also drop some unnecessary panics when reading OnionHopData objects. This also enables one of the phantom failure tests because we can construct OnionHopDatas with invalid amounts. Lastly, remove a bogus comment
1 parent 3faea33 commit 26fe879

File tree

3 files changed

+283
-14
lines changed

3 files changed

+283
-14
lines changed

‎lightning/src/ln/channelmanager.rs

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3075,9 +3075,6 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
30753075
// the channel is now on chain and our counterparty is
30763076
// trying to broadcast the HTLC-Timeout, but that's their
30773077
// problem, not ours.
3078-
//
3079-
// `fail_htlc_backwards_internal` is never called for
3080-
// phantom payments, so this is unreachable for them.
30813078
}
30823079
}
30833080
}
@@ -3792,12 +3789,18 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
37923789
pending_events.push(path_failure);
37933790
if let Some(ev) = full_failure_ev { pending_events.push(ev); }
37943791
},
3795-
HTLCSource::PreviousHopData(HTLCPreviousHopData { short_channel_id, htlc_id, incoming_packet_shared_secret, .. }) => {
3792+
HTLCSource::PreviousHopData(HTLCPreviousHopData { short_channel_id, htlc_id, incoming_packet_shared_secret, phantom_shared_secret, .. }) => {
37963793
let err_packet = match onion_error {
37973794
HTLCFailReason::Reason { failure_code, data } => {
37983795
log_trace!(self.logger, "Failing HTLC with payment_hash {} backwards from us with code {}", log_bytes!(payment_hash.0), failure_code);
3799-
let packet = onion_utils::build_failure_packet(&incoming_packet_shared_secret, failure_code, &data[..]).encode();
3800-
onion_utils::encrypt_failure_packet(&incoming_packet_shared_secret, &packet)
3796+
if let Some(phantom_ss) = phantom_shared_secret {
3797+
let phantom_packet = onion_utils::build_failure_packet(&phantom_ss, failure_code, &data[..]).encode();
3798+
let encrypted_phantom_packet = onion_utils::encrypt_failure_packet(&phantom_ss, &phantom_packet);
3799+
onion_utils::encrypt_failure_packet(&incoming_packet_shared_secret, &encrypted_phantom_packet.data[..])
3800+
} else {
3801+
let packet = onion_utils::build_failure_packet(&incoming_packet_shared_secret, failure_code, &data[..]).encode();
3802+
onion_utils::encrypt_failure_packet(&incoming_packet_shared_secret, &packet)
3803+
}
38013804
},
38023805
HTLCFailReason::LightningError { err } => {
38033806
log_trace!(self.logger, "Failing HTLC with payment_hash {} backwards with pre-built LightningError", log_bytes!(payment_hash.0));

‎lightning/src/ln/msgs.rs

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1291,10 +1291,6 @@ impl Readable for FinalOnionHopData {
12911291

12921292
impl Writeable for OnionHopData {
12931293
fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
1294-
// Note that this should never be reachable if Rust-Lightning generated the message, as we
1295-
// check values are sane long before we get here, though its possible in the future
1296-
// user-generated messages may hit this.
1297-
if self.amt_to_forward > MAX_VALUE_MSAT { panic!("We should never be sending infinite/overflow onion payments"); }
12981294
match self.format {
12991295
OnionHopDataFormat::Legacy { short_channel_id } => {
13001296
0u8.write(w)?;
@@ -1311,9 +1307,6 @@ impl Writeable for OnionHopData {
13111307
});
13121308
},
13131309
OnionHopDataFormat::FinalNode { ref payment_data, ref keysend_preimage } => {
1314-
if let Some(final_data) = payment_data {
1315-
if final_data.total_msat > MAX_VALUE_MSAT { panic!("We should never be sending infinite/overflow onion payments"); }
1316-
}
13171310
encode_varint_length_prefixed_tlv!(w, {
13181311
(2, HighZeroBytesDroppedVarInt(self.amt_to_forward), required),
13191312
(4, HighZeroBytesDroppedVarInt(self.outgoing_cltv_value), required),

‎lightning/src/ln/onion_route_tests.rs

Lines changed: 274 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ use ln::msgs;
2323
use ln::msgs::{ChannelMessageHandler, ChannelUpdate, OptionalField};
2424
use util::events::{Event, MessageSendEvent, MessageSendEventsProvider};
2525
use util::ser::{Writeable, Writer};
26-
use util::test_utils;
26+
use util::{byte_utils, test_utils};
2727
use util::config::UserConfig;
2828

2929
use bitcoin::hash_types::BlockHash;
@@ -677,3 +677,276 @@ fn test_phantom_onion_hmac_failure() {
677677
expect_payment_failed_conditions!(nodes[0], payment_hash, false, fail_conditions);
678678
}
679679

680+
#[test]
681+
fn test_phantom_invalid_onion_payload() {
682+
let chanmon_cfgs = create_chanmon_cfgs(2);
683+
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
684+
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
685+
let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
686+
687+
let channel = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
688+
689+
// Get the route.
690+
let recv_value_msat = 10_000;
691+
let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[1], Some(recv_value_msat));
692+
let (route, phantom_scid) = get_phantom_route!(nodes, recv_value_msat, channel);
693+
694+
// We'll use the session priv later when constructing an invalid onion packet.
695+
let session_priv = [3; 32];
696+
*nodes[0].keys_manager.override_session_priv.lock().unwrap() = Some(session_priv);
697+
nodes[0].node.send_payment(&route, payment_hash.clone(), &Some(payment_secret)).unwrap();
698+
check_added_monitors!(nodes[0], 1);
699+
let update_0 = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
700+
let mut update_add = update_0.update_add_htlcs[0].clone();
701+
702+
nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add);
703+
commitment_signed_dance!(nodes[1], nodes[0], &update_0.commitment_signed, false, true);
704+
705+
// Modify the onion packet to have an invalid payment amount.
706+
for (_, pending_forwards) in nodes[1].node.channel_state.lock().unwrap().forward_htlcs.iter_mut() {
707+
for f in pending_forwards.iter_mut() {
708+
match f {
709+
&mut HTLCForwardInfo::AddHTLC {
710+
forward_info: PendingHTLCInfo {
711+
routing: PendingHTLCRouting::Forward { ref mut onion_packet, .. },
712+
..
713+
}, ..
714+
} => {
715+
// Construct the onion payloads for the entire route and an invalid amount.
716+
let height = nodes[0].best_block_info().1;
717+
let session_priv = SecretKey::from_slice(&session_priv).unwrap();
718+
let mut onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route.paths[0], &session_priv).unwrap();
719+
let (mut onion_payloads, _, _) = onion_utils::build_onion_payloads(&route.paths[0], msgs::MAX_VALUE_MSAT + 1, &Some(payment_secret), height + 1, &None).unwrap();
720+
// We only want to construct the onion packet for the last hop, not the entire route, so
721+
// remove the first hop's payload and its keys.
722+
onion_keys.remove(0);
723+
onion_payloads.remove(0);
724+
725+
let new_onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &payment_hash);
726+
onion_packet.hop_data = new_onion_packet.hop_data;
727+
onion_packet.hmac = new_onion_packet.hmac;
728+
},
729+
_ => panic!("Unexpected forward"),
730+
}
731+
}
732+
}
733+
expect_pending_htlcs_forwardable_ignore!(nodes[1]);
734+
nodes[1].node.process_pending_htlc_forwards();
735+
expect_pending_htlcs_forwardable_ignore!(nodes[1]);
736+
nodes[1].node.process_pending_htlc_forwards();
737+
let update_1 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
738+
check_added_monitors!(&nodes[1], 1);
739+
assert!(update_1.update_fail_htlcs.len() == 1);
740+
let fail_msg = update_1.update_fail_htlcs[0].clone();
741+
nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg);
742+
commitment_signed_dance!(nodes[0], nodes[1], update_1.commitment_signed, false);
743+
744+
// Ensure the payment fails with the expected error.
745+
let error_data = Vec::new();
746+
let mut fail_conditions = PaymentFailedConditions::new()
747+
.blamed_scid(phantom_scid)
748+
.blamed_chan_closed(true)
749+
.expected_htlc_error_data(0x4000 | 22, &error_data);
750+
expect_payment_failed_conditions!(nodes[0], payment_hash, true, fail_conditions);
751+
}
752+
753+
#[test]
754+
fn test_phantom_final_incorrect_cltv_expiry() {
755+
let chanmon_cfgs = create_chanmon_cfgs(2);
756+
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
757+
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
758+
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
759+
760+
let channel = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
761+
762+
// Get the route.
763+
let recv_value_msat = 10_000;
764+
let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[1], Some(recv_value_msat));
765+
let (route, phantom_scid) = get_phantom_route!(nodes, recv_value_msat, channel);
766+
767+
// Route the HTLC through to the destination.
768+
nodes[0].node.send_payment(&route, payment_hash.clone(), &Some(payment_secret)).unwrap();
769+
check_added_monitors!(nodes[0], 1);
770+
let update_0 = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
771+
let mut update_add = update_0.update_add_htlcs[0].clone();
772+
773+
nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add);
774+
commitment_signed_dance!(nodes[1], nodes[0], &update_0.commitment_signed, false, true);
775+
776+
// Modify the payload so the phantom hop's HMAC is bogus.
777+
for (_, pending_forwards) in nodes[1].node.channel_state.lock().unwrap().forward_htlcs.iter_mut() {
778+
for f in pending_forwards.iter_mut() {
779+
match f {
780+
&mut HTLCForwardInfo::AddHTLC {
781+
forward_info: PendingHTLCInfo { ref mut outgoing_cltv_value, .. }, ..
782+
} => {
783+
*outgoing_cltv_value += 1;
784+
},
785+
_ => panic!("Unexpected forward"),
786+
}
787+
}
788+
}
789+
expect_pending_htlcs_forwardable_ignore!(nodes[1]);
790+
nodes[1].node.process_pending_htlc_forwards();
791+
expect_pending_htlcs_forwardable_ignore!(nodes[1]);
792+
nodes[1].node.process_pending_htlc_forwards();
793+
let update_1 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
794+
check_added_monitors!(&nodes[1], 1);
795+
assert!(update_1.update_fail_htlcs.len() == 1);
796+
let fail_msg = update_1.update_fail_htlcs[0].clone();
797+
nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg);
798+
commitment_signed_dance!(nodes[0], nodes[1], update_1.commitment_signed, false);
799+
800+
// Ensure the payment fails with the expected error.
801+
let expected_cltv = 82;
802+
let error_data = byte_utils::be32_to_array(expected_cltv).to_vec();
803+
let mut fail_conditions = PaymentFailedConditions::new()
804+
.blamed_scid(phantom_scid)
805+
.expected_htlc_error_data(18, &error_data);
806+
expect_payment_failed_conditions!(nodes[0], payment_hash, false, fail_conditions);
807+
}
808+
809+
#[test]
810+
fn test_phantom_failure_too_low_cltv() {
811+
let chanmon_cfgs = create_chanmon_cfgs(2);
812+
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
813+
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
814+
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
815+
816+
let channel = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
817+
818+
// Get the route.
819+
let recv_value_msat = 10_000;
820+
let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[1], Some(recv_value_msat));
821+
let (mut route, phantom_scid) = get_phantom_route!(nodes, recv_value_msat, channel);
822+
823+
// Modify the route to have a too-low cltv.
824+
route.paths[0][1].cltv_expiry_delta = 5;
825+
826+
// Route the HTLC through to the destination.
827+
nodes[0].node.send_payment(&route, payment_hash.clone(), &Some(payment_secret)).unwrap();
828+
check_added_monitors!(nodes[0], 1);
829+
let update_0 = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
830+
let mut update_add = update_0.update_add_htlcs[0].clone();
831+
832+
nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add);
833+
commitment_signed_dance!(nodes[1], nodes[0], &update_0.commitment_signed, false, true);
834+
835+
expect_pending_htlcs_forwardable_ignore!(nodes[1]);
836+
nodes[1].node.process_pending_htlc_forwards();
837+
expect_pending_htlcs_forwardable_ignore!(nodes[1]);
838+
nodes[1].node.process_pending_htlc_forwards();
839+
let update_1 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
840+
check_added_monitors!(&nodes[1], 1);
841+
assert!(update_1.update_fail_htlcs.len() == 1);
842+
let fail_msg = update_1.update_fail_htlcs[0].clone();
843+
nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg);
844+
commitment_signed_dance!(nodes[0], nodes[1], update_1.commitment_signed, false);
845+
846+
// Ensure the payment fails with the expected error.
847+
let error_data = Vec::new();
848+
let mut fail_conditions = PaymentFailedConditions::new()
849+
.blamed_scid(phantom_scid)
850+
.expected_htlc_error_data(17, &error_data);
851+
expect_payment_failed_conditions!(nodes[0], payment_hash, false, fail_conditions);
852+
}
853+
854+
#[test]
855+
fn test_phantom_failure_too_low_recv_amt() {
856+
let chanmon_cfgs = create_chanmon_cfgs(2);
857+
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
858+
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
859+
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
860+
861+
let channel = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
862+
863+
// Get the route with a too-low amount.
864+
let recv_amt_msat = 10_000;
865+
let bad_recv_amt_msat = recv_amt_msat - 10;
866+
let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[1], Some(recv_amt_msat));
867+
let (mut route, phantom_scid) = get_phantom_route!(nodes, bad_recv_amt_msat, channel);
868+
869+
// Route the HTLC through to the destination.
870+
nodes[0].node.send_payment(&route, payment_hash.clone(), &Some(payment_secret)).unwrap();
871+
check_added_monitors!(nodes[0], 1);
872+
let update_0 = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
873+
let mut update_add = update_0.update_add_htlcs[0].clone();
874+
875+
nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add);
876+
commitment_signed_dance!(nodes[1], nodes[0], &update_0.commitment_signed, false, true);
877+
878+
expect_pending_htlcs_forwardable_ignore!(nodes[1]);
879+
nodes[1].node.process_pending_htlc_forwards();
880+
expect_pending_htlcs_forwardable_ignore!(nodes[1]);
881+
nodes[1].node.process_pending_htlc_forwards();
882+
expect_pending_htlcs_forwardable_ignore!(nodes[1]);
883+
nodes[1].node.process_pending_htlc_forwards();
884+
let update_1 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
885+
check_added_monitors!(&nodes[1], 1);
886+
assert!(update_1.update_fail_htlcs.len() == 1);
887+
let fail_msg = update_1.update_fail_htlcs[0].clone();
888+
nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg);
889+
commitment_signed_dance!(nodes[0], nodes[1], update_1.commitment_signed, false);
890+
891+
// Ensure the payment fails with the expected error.
892+
let mut error_data = byte_utils::be64_to_array(bad_recv_amt_msat).to_vec();
893+
error_data.extend_from_slice(
894+
&byte_utils::be32_to_array(nodes[1].node.best_block.read().unwrap().height()),
895+
);
896+
let mut fail_conditions = PaymentFailedConditions::new()
897+
.blamed_scid(phantom_scid)
898+
.expected_htlc_error_data(0x4000 | 15, &error_data);
899+
expect_payment_failed_conditions!(nodes[0], payment_hash, true, fail_conditions);
900+
}
901+
902+
#[test]
903+
fn test_phantom_failure_reject_payment() {
904+
// Test that the user can successfully fail back a phantom node payment.
905+
let chanmon_cfgs = create_chanmon_cfgs(2);
906+
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
907+
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
908+
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
909+
910+
let channel = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
911+
912+
// Get the route with a too-low amount.
913+
let recv_amt_msat = 10_000;
914+
let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[1], Some(recv_amt_msat));
915+
let (mut route, phantom_scid) = get_phantom_route!(nodes, recv_amt_msat, channel);
916+
917+
// Route the HTLC through to the destination.
918+
nodes[0].node.send_payment(&route, payment_hash.clone(), &Some(payment_secret)).unwrap();
919+
check_added_monitors!(nodes[0], 1);
920+
let update_0 = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
921+
let mut update_add = update_0.update_add_htlcs[0].clone();
922+
923+
nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add);
924+
commitment_signed_dance!(nodes[1], nodes[0], &update_0.commitment_signed, false, true);
925+
926+
expect_pending_htlcs_forwardable_ignore!(nodes[1]);
927+
nodes[1].node.process_pending_htlc_forwards();
928+
expect_pending_htlcs_forwardable_ignore!(nodes[1]);
929+
nodes[1].node.process_pending_htlc_forwards();
930+
expect_payment_received!(nodes[1], payment_hash, payment_secret, recv_amt_msat);
931+
assert!(nodes[1].node.fail_htlc_backwards(&payment_hash));
932+
expect_pending_htlcs_forwardable_ignore!(nodes[1]);
933+
nodes[1].node.process_pending_htlc_forwards();
934+
935+
let update_1 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
936+
check_added_monitors!(&nodes[1], 1);
937+
assert!(update_1.update_fail_htlcs.len() == 1);
938+
let fail_msg = update_1.update_fail_htlcs[0].clone();
939+
nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg);
940+
commitment_signed_dance!(nodes[0], nodes[1], update_1.commitment_signed, false);
941+
942+
// Ensure the payment fails with the expected error.
943+
let mut error_data = byte_utils::be64_to_array(recv_amt_msat).to_vec();
944+
error_data.extend_from_slice(
945+
&byte_utils::be32_to_array(nodes[1].node.best_block.read().unwrap().height()),
946+
);
947+
let mut fail_conditions = PaymentFailedConditions::new()
948+
.blamed_scid(phantom_scid)
949+
.expected_htlc_error_data(0x4000 | 15, &error_data);
950+
expect_payment_failed_conditions!(nodes[0], payment_hash, true, fail_conditions);
951+
}
952+

0 commit comments

Comments
 (0)