Skip to content

Sanititze and document incoming HTLC cltv_expiry handling #579

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 24, 2020
Merged

Sanititze and document incoming HTLC cltv_expiry handling #579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
795aff8
Document exactly our CLTV sanitization policy for incoming HTLCs
Apr 10, 2020
ad5f728
Document exactly our CLTV sanitization policy for final incoming HTLCs
Apr 10, 2020
886223a
Sanitize outgoing HTLC cltv_value
Apr 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 11 additions & 12 deletions fuzz/src/full_stack.rs
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 0 additions & 4 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1727,8 +1727,6 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
return Err(ChannelError::Close("Remote provided CLTV expiry in seconds instead of block height"));
}

//TODO: Check msg.cltv_expiry further? Do this in channel manager?

if self.channel_state & ChannelState::LocalShutdownSent as u32 != 0 {
if let PendingHTLCStatus::Forward(_) = pending_forward_state {
panic!("ChannelManager shouldn't be trying to add a forwardable HTLC after we've started closing");
Expand Down Expand Up @@ -3559,8 +3557,6 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
return Err(ChannelError::Ignore("Cannot send value that would put us over their reserve value"));
}

//TODO: Check cltv_expiry? Do this in channel manager?

// Now update local state:
if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == (ChannelState::AwaitingRemoteRevoke as u32) {
self.holding_cell_htlc_updates.push(HTLCUpdateAwaitingACK::AddHTLC {
Expand Down
19 changes: 15 additions & 4 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use secp256k1;
use chain::chaininterface::{BroadcasterInterface,ChainListener,FeeEstimator};
use chain::transaction::OutPoint;
use ln::channel::{Channel, ChannelError};
use ln::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateErr, ManyChannelMonitor, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY, HTLC_FAIL_BACK_BUFFER};
use ln::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateErr, ManyChannelMonitor, HTLC_FAIL_BACK_BUFFER, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY};
use ln::features::{InitFeatures, NodeFeatures};
use ln::router::{Route, RouteHop};
use ln::msgs;
Expand Down Expand Up @@ -1039,7 +1039,11 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref> ChannelMan

// OUR PAYMENT!
// final_expiry_too_soon
if (msg.cltv_expiry as u64) < self.latest_block_height.load(Ordering::Acquire) as u64 + (CLTV_CLAIM_BUFFER + LATENCY_GRACE_PERIOD_BLOCKS) as u64 {
// We have to have some headroom to broadcast on chain if we have the preimage, so make sure we have at least
// HTLC_FAIL_BACK_BUFFER blocks to go.
// Also, ensure that, in the case of an unknown payment hash, our payment logic has enough time to fail the HTLC backward
// before our onchain logic triggers a channel closure (see HTLC_FAIL_BACK_BUFFER rational).
if (msg.cltv_expiry as u64) <= self.latest_block_height.load(Ordering::Acquire) as u64 + HTLC_FAIL_BACK_BUFFER as u64 + 1 {
return_err!("The final CLTV expiry is too soon to handle", 17, &[0;0]);
}
// final_incorrect_htlc_amount
Expand Down Expand Up @@ -1163,13 +1167,20 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref> ChannelMan
break Some(("Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta", 0x1000 | 13, Some(self.get_channel_update(chan).unwrap())));
}
let cur_height = self.latest_block_height.load(Ordering::Acquire) as u32 + 1;
// We want to have at least LATENCY_GRACE_PERIOD_BLOCKS to fail prior to going on chain CLAIM_BUFFER blocks before expiration
if msg.cltv_expiry <= cur_height + CLTV_CLAIM_BUFFER + LATENCY_GRACE_PERIOD_BLOCKS as u32 { // expiry_too_soon
// Theoretically, channel counterparty shouldn't send us a HTLC expiring now, but we want to be robust wrt to counterparty
// packet sanitization (see HTLC_FAIL_BACK_BUFFER rational)
if msg.cltv_expiry <= cur_height + HTLC_FAIL_BACK_BUFFER as u32 { // expiry_too_soon
break Some(("CLTV expiry is too close", 0x1000 | 14, Some(self.get_channel_update(chan).unwrap())));
}
if msg.cltv_expiry > cur_height + CLTV_FAR_FAR_AWAY as u32 { // expiry_too_far
break Some(("CLTV expiry is too far in the future", 21, None));
}
// In theory, we would be safe against unitentional channel-closure, if we only required a margin of LATENCY_GRACE_PERIOD_BLOCKS.
// But, to be safe against policy reception, we use a longuer delay.
if (*outgoing_cltv_value) as u64 <= (cur_height + HTLC_FAIL_BACK_BUFFER) as u64 {
break Some(("Outgoing CLTV value is too soon", 0x1000 | 14, Some(self.get_channel_update(chan).unwrap())));
}

break None;
}
{
Expand Down
5 changes: 3 additions & 2 deletions lightning/src/util/events.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,9 @@ pub enum Event {
},
/// Indicates we've received money! Just gotta dig out that payment preimage and feed it to
/// ChannelManager::claim_funds to get it....
/// Note that if the preimage is not known or the amount paid is incorrect, you must call
/// ChannelManager::fail_htlc_backwards to free up resources for this HTLC.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I think we should still encourage the user to do this - waiting for the timeout is kinda bad for the network. Its possible waiting for the timeout is better from a privacy perspective, but hopefully such attacks aren't feasible, what do you think?

/// Note that if the preimage is not known or the amount paid is incorrect, you should call
/// ChannelManager::fail_htlc_backwards to free up resources for this HTLC and avoid
/// network congestion.
/// The amount paid should be considered 'incorrect' when it is less than or more than twice
/// the amount expected.
/// If you fail to call either ChannelManager::claim_funds or
Expand Down