-
Notifications
You must be signed in to change notification settings - Fork 385
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for custom HTLC TLVs #2308
Changes from all commits
039b1c8
d2e9cb4
f560320
0a2dbdf
e84fb06
8ff1604
dec3fb3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -110,13 +110,17 @@ pub(super) enum PendingHTLCRouting { | |
payment_metadata: Option<Vec<u8>>, | ||
incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed | ||
phantom_shared_secret: Option<[u8; 32]>, | ||
/// See [`RecipientOnionFields::custom_tlvs`] for more info. | ||
custom_tlvs: Vec<(u64, Vec<u8>)>, | ||
}, | ||
ReceiveKeysend { | ||
/// This was added in 0.0.116 and will break deserialization on downgrades. | ||
payment_data: Option<msgs::FinalOnionHopData>, | ||
payment_preimage: PaymentPreimage, | ||
payment_metadata: Option<Vec<u8>>, | ||
incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed | ||
/// See [`RecipientOnionFields::custom_tlvs`] for more info. | ||
custom_tlvs: Vec<(u64, Vec<u8>)>, | ||
}, | ||
} | ||
|
||
|
@@ -355,15 +359,32 @@ struct InboundOnionErr { | |
pub enum FailureCode { | ||
/// We had a temporary error processing the payment. Useful if no other error codes fit | ||
/// and you want to indicate that the payer may want to retry. | ||
TemporaryNodeFailure = 0x2000 | 2, | ||
TemporaryNodeFailure, | ||
/// We have a required feature which was not in this onion. For example, you may require | ||
/// some additional metadata that was not provided with this payment. | ||
RequiredNodeFeatureMissing = 0x4000 | 0x2000 | 3, | ||
RequiredNodeFeatureMissing, | ||
/// You may wish to use this when a `payment_preimage` is unknown, or the CLTV expiry of | ||
/// the HTLC is too close to the current block height for safe handling. | ||
/// Using this failure code in [`ChannelManager::fail_htlc_backwards_with_reason`] is | ||
/// equivalent to calling [`ChannelManager::fail_htlc_backwards`]. | ||
IncorrectOrUnknownPaymentDetails = 0x4000 | 15, | ||
IncorrectOrUnknownPaymentDetails, | ||
/// We failed to process the payload after the onion was decrypted. You may wish to | ||
/// use this when receiving custom HTLC TLVs with even type numbers that you don't recognize. | ||
/// | ||
/// If available, the tuple data may include the type number and byte offset in the | ||
/// decrypted byte stream where the failure occurred. | ||
InvalidOnionPayload(Option<(u64, u16)>), | ||
wpaulino marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
impl Into<u16> for FailureCode { | ||
fn into(self) -> u16 { | ||
match self { | ||
FailureCode::TemporaryNodeFailure => 0x2000 | 2, | ||
FailureCode::RequiredNodeFeatureMissing => 0x4000 | 0x2000 | 3, | ||
FailureCode::IncorrectOrUnknownPaymentDetails => 0x4000 | 15, | ||
FailureCode::InvalidOnionPayload(_) => 0x4000 | 22, | ||
} | ||
} | ||
} | ||
|
||
/// Error type returned across the peer_state mutex boundary. When an Err is generated for a | ||
|
@@ -2674,11 +2695,11 @@ where | |
amt_msat: u64, cltv_expiry: u32, phantom_shared_secret: Option<[u8; 32]>, allow_underpay: bool, | ||
counterparty_skimmed_fee_msat: Option<u64>, | ||
) -> Result<PendingHTLCInfo, InboundOnionErr> { | ||
let (payment_data, keysend_preimage, onion_amt_msat, outgoing_cltv_value, payment_metadata) = match hop_data { | ||
let (payment_data, keysend_preimage, custom_tlvs, onion_amt_msat, outgoing_cltv_value, payment_metadata) = match hop_data { | ||
msgs::InboundOnionPayload::Receive { | ||
payment_data, keysend_preimage, amt_msat, outgoing_cltv_value, payment_metadata, .. | ||
payment_data, keysend_preimage, custom_tlvs, amt_msat, outgoing_cltv_value, payment_metadata, .. | ||
} => | ||
(payment_data, keysend_preimage, amt_msat, outgoing_cltv_value, payment_metadata), | ||
(payment_data, keysend_preimage, custom_tlvs, amt_msat, outgoing_cltv_value, payment_metadata), | ||
_ => | ||
return Err(InboundOnionErr { | ||
err_code: 0x4000|22, | ||
|
@@ -2748,13 +2769,15 @@ where | |
payment_preimage, | ||
payment_metadata, | ||
incoming_cltv_expiry: outgoing_cltv_value, | ||
custom_tlvs, | ||
} | ||
} else if let Some(data) = payment_data { | ||
PendingHTLCRouting::Receive { | ||
payment_data: data, | ||
payment_metadata, | ||
incoming_cltv_expiry: outgoing_cltv_value, | ||
phantom_shared_secret, | ||
custom_tlvs, | ||
} | ||
} else { | ||
return Err(InboundOnionErr { | ||
|
@@ -3941,17 +3964,18 @@ where | |
} | ||
}) => { | ||
let (cltv_expiry, onion_payload, payment_data, phantom_shared_secret, mut onion_fields) = match routing { | ||
PendingHTLCRouting::Receive { payment_data, payment_metadata, incoming_cltv_expiry, phantom_shared_secret } => { | ||
PendingHTLCRouting::Receive { payment_data, payment_metadata, incoming_cltv_expiry, phantom_shared_secret, custom_tlvs } => { | ||
let _legacy_hop_data = Some(payment_data.clone()); | ||
let onion_fields = | ||
RecipientOnionFields { payment_secret: Some(payment_data.payment_secret), payment_metadata }; | ||
let onion_fields = RecipientOnionFields { payment_secret: Some(payment_data.payment_secret), | ||
payment_metadata, custom_tlvs }; | ||
(incoming_cltv_expiry, OnionPayload::Invoice { _legacy_hop_data }, | ||
Some(payment_data), phantom_shared_secret, onion_fields) | ||
}, | ||
PendingHTLCRouting::ReceiveKeysend { payment_data, payment_preimage, payment_metadata, incoming_cltv_expiry } => { | ||
PendingHTLCRouting::ReceiveKeysend { payment_data, payment_preimage, payment_metadata, incoming_cltv_expiry, custom_tlvs } => { | ||
let onion_fields = RecipientOnionFields { | ||
payment_secret: payment_data.as_ref().map(|data| data.payment_secret), | ||
payment_metadata | ||
payment_metadata, | ||
custom_tlvs, | ||
}; | ||
(incoming_cltv_expiry, OnionPayload::Spontaneous(payment_preimage), | ||
payment_data, None, onion_fields) | ||
|
@@ -4576,12 +4600,19 @@ where | |
/// Gets error data to form an [`HTLCFailReason`] given a [`FailureCode`] and [`ClaimableHTLC`]. | ||
fn get_htlc_fail_reason_from_failure_code(&self, failure_code: FailureCode, htlc: &ClaimableHTLC) -> HTLCFailReason { | ||
match failure_code { | ||
FailureCode::TemporaryNodeFailure => HTLCFailReason::from_failure_code(failure_code as u16), | ||
FailureCode::RequiredNodeFeatureMissing => HTLCFailReason::from_failure_code(failure_code as u16), | ||
FailureCode::TemporaryNodeFailure => HTLCFailReason::from_failure_code(failure_code.into()), | ||
FailureCode::RequiredNodeFeatureMissing => HTLCFailReason::from_failure_code(failure_code.into()), | ||
FailureCode::IncorrectOrUnknownPaymentDetails => { | ||
let mut htlc_msat_height_data = htlc.value.to_be_bytes().to_vec(); | ||
htlc_msat_height_data.extend_from_slice(&self.best_block.read().unwrap().height().to_be_bytes()); | ||
HTLCFailReason::reason(failure_code as u16, htlc_msat_height_data) | ||
HTLCFailReason::reason(failure_code.into(), htlc_msat_height_data) | ||
}, | ||
FailureCode::InvalidOnionPayload(data) => { | ||
let fail_data = match data { | ||
Some((typ, offset)) => [BigSize(typ).encode(), offset.encode()].concat(), | ||
None => Vec::new(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I think the |
||
}; | ||
HTLCFailReason::reason(failure_code.into(), fail_data) | ||
} | ||
} | ||
} | ||
|
@@ -4728,13 +4759,35 @@ where | |
/// event matches your expectation. If you fail to do so and call this method, you may provide | ||
/// the sender "proof-of-payment" when they did not fulfill the full expected payment. | ||
/// | ||
/// This function will fail the payment if it has custom TLVs with even type numbers, as we | ||
/// will assume they are unknown. If you intend to accept even custom TLVs, you should use | ||
/// [`claim_funds_with_known_custom_tlvs`]. | ||
/// | ||
/// [`Event::PaymentClaimable`]: crate::events::Event::PaymentClaimable | ||
/// [`Event::PaymentClaimable::claim_deadline`]: crate::events::Event::PaymentClaimable::claim_deadline | ||
/// [`Event::PaymentClaimed`]: crate::events::Event::PaymentClaimed | ||
/// [`process_pending_events`]: EventsProvider::process_pending_events | ||
/// [`create_inbound_payment`]: Self::create_inbound_payment | ||
/// [`create_inbound_payment_for_hash`]: Self::create_inbound_payment_for_hash | ||
/// [`claim_funds_with_known_custom_tlvs`]: Self::claim_funds_with_known_custom_tlvs | ||
pub fn claim_funds(&self, payment_preimage: PaymentPreimage) { | ||
self.claim_payment_internal(payment_preimage, false); | ||
} | ||
|
||
/// This is a variant of [`claim_funds`] that allows accepting a payment with custom TLVs with | ||
alecchendev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// even type numbers. | ||
/// | ||
/// # Note | ||
/// | ||
/// You MUST check you've understood all even TLVs before using this to | ||
/// claim, otherwise you may unintentionally agree to some protocol you do not understand. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don’t know if it should be documented here, in the pending changelog about this new custom HTLC TLVs support or at the spec-level, though as a receiver it should be mandatory to have a receiver-issued secret as part of the custom TLV before to even accept to process an experimental See the concern above deanonymization attack, and how such requirement would serve as a deterrent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see why this matters, can you explain what the concrete attack you're worried about is? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let’s take the “Cancellable payments” protocol as described here: https://github.com/BlockstreamResearch/scriptless-scripts/blob/master/md/multi-hop-locks.md#cancellable-payments Let’s assume there is a Lack of authentication by the mean of a receiver-issued secret allow anyone with the details of the cancellable payments to send a payment shard (e.g a routing hop), and therefore to probe the chain height as seen by the recipient Lightning node (a fingerprint vector that could be used to cross-link with the on-chain full-node). The landscape could be different with PTLCs, though here it’s too early to say if/how mutli-shard payment could be carried through. Yes, PTLCs could come with an inherent access structure, though this is not certain as of today. Overall, it’s a reasonable internet protocol design rule to authenticate your network input as early as you can. Recommending our custom HTLC TLVs to authenticate its custom protocol sounds a good rule of thumb. For recent failure between Lightning API interface and application processing, see https://lists.linuxfoundation.org/pipermail/lightning-dev/2023-June/003983.html There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that we only ever deal with an HTLC after we've validated the payment_secret, so I don't see why any specific handling has to be done in the custom TLVs, can you describe an attack where payment_secret isn't sufficient and something has to be added to the custom TLVs? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes where you have multiple payers and one payee, and each have to pay their shares of the payment e.g a crowdfunding between named participants. So current Somehow that type of multi-payer single-payee for crowdfunding or common bill splitting is what custom HTLC TLVs would be very nice to experiment with. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I'm not sure doing authentication with custom TLVs within a multi-payer single-payee MPP payment is possible in the current implementation of custom TLVs as we drop/fail non-matching TLVs, so if different payment parts had different secrets, they'd just get dropped? Although, either way, I think for now this recommendation may be a little out of scope for the docs here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well you would need to have custom TLVs for The hard thing is it’s hard to see if authentication or privacy is screwed or not before you have both a specification of the protocol and implementation. I think adding the sentence "You MAY authenticate the even TLVs additionally of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry was late to respond to this, is it helpful to add this even though a user couldn't actually take the suggestion? Ultimately any different custom TLV values between different payment parts will be dropped/failed through There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Fair point, multi-payer secret would need change in our MPP validation, well kinda restraint what you can do in term of custom HTLC TLVs, though can be extended in the future. |
||
/// | ||
/// [`claim_funds`]: Self::claim_funds | ||
pub fn claim_funds_with_known_custom_tlvs(&self, payment_preimage: PaymentPreimage) { | ||
self.claim_payment_internal(payment_preimage, true); | ||
} | ||
|
||
fn claim_payment_internal(&self, payment_preimage: PaymentPreimage, custom_tlvs_known: bool) { | ||
let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner()); | ||
|
||
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self); | ||
|
@@ -4761,6 +4814,23 @@ where | |
log_error!(self.logger, "Got a duplicate pending claimable event on payment hash {}! Please report this bug", | ||
log_bytes!(payment_hash.0)); | ||
} | ||
|
||
if let Some(RecipientOnionFields { ref custom_tlvs, .. }) = payment.onion_fields { | ||
if !custom_tlvs_known && custom_tlvs.iter().any(|(typ, _)| typ % 2 == 0) { | ||
log_info!(self.logger, "Rejecting payment with payment hash {} as we cannot accept payment with unknown even TLVs: {}", | ||
log_bytes!(payment_hash.0), log_iter!(custom_tlvs.iter().map(|(typ, _)| typ).filter(|typ| *typ % 2 == 0))); | ||
claimable_payments.pending_claiming_payments.remove(&payment_hash); | ||
mem::drop(claimable_payments); | ||
for htlc in payment.htlcs { | ||
let reason = self.get_htlc_fail_reason_from_failure_code(FailureCode::InvalidOnionPayload(None), &htlc); | ||
let source = HTLCSource::PreviousHopData(htlc.prev_hop); | ||
let receiver = HTLCDestination::FailedPayment { payment_hash }; | ||
self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver); | ||
} | ||
return; | ||
} | ||
} | ||
|
||
payment.htlcs | ||
} else { return; } | ||
}; | ||
|
@@ -7638,12 +7708,14 @@ impl_writeable_tlv_based_enum!(PendingHTLCRouting, | |
(1, phantom_shared_secret, option), | ||
(2, incoming_cltv_expiry, required), | ||
(3, payment_metadata, option), | ||
(5, custom_tlvs, optional_vec), | ||
}, | ||
(2, ReceiveKeysend) => { | ||
(0, payment_preimage, required), | ||
(2, incoming_cltv_expiry, required), | ||
(3, payment_metadata, option), | ||
(4, payment_data, option), // Added in 0.0.116 | ||
(5, custom_tlvs, optional_vec), | ||
}, | ||
;); | ||
|
||
|
@@ -8750,6 +8822,7 @@ where | |
payment_secret: None, // only used for retries, and we'll never retry on startup | ||
payment_metadata: None, // only used for retries, and we'll never retry on startup | ||
keysend_preimage: None, // only used for retries, and we'll never retry on startup | ||
custom_tlvs: Vec::new(), // only used for retries, and we'll never retry on startup | ||
pending_amt_msat: path_amt, | ||
pending_fee_msat: Some(path_fee), | ||
total_msat: path_amt, | ||
|
@@ -10092,6 +10165,7 @@ mod tests { | |
payment_data: Some(msgs::FinalOnionHopData { | ||
payment_secret: PaymentSecret([0; 32]), total_msat: sender_intended_amt_msat, | ||
}), | ||
custom_tlvs: Vec::new(), | ||
}; | ||
// Check that if the amount we received + the penultimate hop extra fee is less than the sender | ||
// intended amount, we fail the payment. | ||
|
@@ -10111,6 +10185,7 @@ mod tests { | |
payment_data: Some(msgs::FinalOnionHopData { | ||
payment_secret: PaymentSecret([0; 32]), total_msat: sender_intended_amt_msat, | ||
}), | ||
custom_tlvs: Vec::new(), | ||
}; | ||
assert!(node[0].node.construct_recv_pending_htlc_info(hop_data, [0; 32], PaymentHash([0; 32]), | ||
sender_intended_amt_msat - extra_fee_msat, 42, None, true, Some(extra_fee_msat)).is_ok()); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we return the custom HTLC TLVs as it could be used as a node deanonymization fingerprint where one would leak the LDK version number in function of recognize TLV type, or even the presence of a custom
tlv_record
processing application ?Returning a silent
invalid_onion_payload
might be better.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(a) I don't think node fingerprinting is in our security model - there's a million ways you can do it and its just not practical to prevent, (b) I don't see how returning a unknown TLV vs just
invalid_onion_payload
makes fingerprinting easier.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(a) I think deanonymization attack based on onion failure message is in LN security model, see the now-deprecated
incorrect_payment_amount
and `final_expiry_too_soon and b) You introduce a probing vector between versions of LDK, the one giving you back an unknown TLV record and the one giving you a valid result for the same custom TLV record (independently of this being a success or error).In a world with blinded paths, a payer might never be aware of the node_pubkey of the payee, and such malicious onion payload is a valid deanonymization vector. I agree with you in a world pre-blinding paths, we would care less about fingerprinting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a different attack - that's revealing if an HTLC is yours vs if its not. The attack you describe here is only discovering that you're running LDK and potentially which version. Again, I don't think its possible to defend against the second attack you describe here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the other point if you introduce an overlay protocol like cancellable payment, what content you return as an onion message might leak internal state machine, and therefore if it’s revealing if a HTLC is yours.
On the second attack, software user agent and version are considered in browser deanomymization literature (cf. https://leakuidatorplusteam.github.io/preprint.pdf) and it’s a strong technique to break the one whole anonymous set in segment. As a reminder as a successful de-anonymization attack is just multiple cumulating multiple leaks and correlating them until you can probe the result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we return no error for blinded paths (which may be up to further discussion upstream), and pre-blinded paths fingerprinting is outside our security model, is there any action here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking on the code, it sounds we always return a silent
invalid_onion_payload
as suggested i.eInvalidOnionPayload(None)
here: https://github.com/lightningdevkit/rust-lightning/pull/2308/files#diff-645d12c7269d09caab57d2e0c1fd34f672ee259994c86ad4bad7f4da9183671cR4812So I think it’s good in case of future errors return even for blinded paths in the future we’re good, no need to change the code.
Looking on the documentation of
FailureCode::InvalidOnionPayload
, “if available, the tuple data should include the type number and byte offset in the decrypted byte stream where the failure occurred”. This could be change to say we never return the custom TLV to avoid leaking user agent / version in case of future onion changes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just changed to say "may" instead of "should". I'm going to hold off on mentioning leaking user agent/version unless another reviewer chimes in as it still seems to be outside our security model from what I understand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well “may” is reasonable and PR should be numbered in release notes, so user can always go back to this conversation X years from now, assuming GH as a platform is still available.