Makes ChannelManager::force_close_channel fail for unknown chan_ids #777
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sr-gi
force-pushed
the
cm-force-close-checks
branch
from
75d9ee0 to
e6e0cf6
Compare
Codecov Report
@@ Coverage Diff @@
## main #777 +/- ##
==========================================
- Coverage 91.24% 91.24% -0.01%
==========================================
Files 37 37
Lines 22866 22847 -19
==========================================
- Hits 20865 20846 -19
Misses 2001 2001
Continue to review full report at Codecov.
|
TheBlueMatt
reviewed
Jan 14, 2021
lightning/src/ln/channelmanager.rs
Outdated
| } | ||
|
|
||
| /// Force close all channels, immediately broadcasting the latest local commitment transaction | ||
| /// for each to the chain and rejecting new HTLCs on each. | ||
| pub fn force_close_all_channels(&self) { | ||
| for chan in self.list_channels() { | ||
| self.force_close_channel(&chan.channel_id); | ||
| self.force_close_channel(&chan.channel_id).unwrap(); |
There was a problem hiding this comment.
This is race-y - if we close a channel in another thread while this is running we'll panic needlessly.
| @@ -918,7 +918,7 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> | |||
|
|
|||
| /// Force closes a channel, immediately broadcasting the latest local commitment transaction to | |||
| /// the chain and rejecting new HTLCs on the given channel. | |||
| pub fn force_close_channel(&self, channel_id: &[u8; 32]) { | |||
| pub fn force_close_channel(&self, channel_id: &[u8; 32]) -> Result<(), APIError>{ | |||
There was a problem hiding this comment.
It would be nice to update the docs here to state that we only fail in case the channel is not found.
sr-gi
force-pushed
the
cm-force-close-checks
branch
from
e6e0cf6 to
0e83723
Compare
TheBlueMatt
reviewed
Jan 15, 2021
lightning/src/ln/channelmanager.rs
Outdated
| } | ||
| } | ||
| } else { | ||
| self.force_close_channel(&msg.channel_id); | ||
| self.force_close_channel(&msg.channel_id).unwrap(); |
There was a problem hiding this comment.
This handles an untrusted message from a peer - if they send us a garbage channel_id we shouldn't crash.
There was a problem hiding this comment.
Ups, didn't realize that came from a peer.
Rebased in 03a72f1
lightning/src/ln/channelmanager.rs
Outdated
| @@ -3471,11 +3473,11 @@ impl<ChanSigner: ChannelKeys, M: Deref + Sync + Send, T: Deref + Sync + Send, K: | |||
| if msg.channel_id == [0; 32] { | |||
| for chan in self.list_channels() { | |||
| if chan.remote_network_id == *counterparty_node_id { | |||
| self.force_close_channel(&chan.channel_id); | |||
| self.force_close_channel(&chan.channel_id).unwrap(); | |||
There was a problem hiding this comment.
This is also race-y, I believe.
sr-gi
force-pushed
the
cm-force-close-checks
branch
from
0e83723 to
03a72f1
Compare
ariard
reviewed
Jan 18, 2021
| @@ -3471,11 +3473,11 @@ impl<ChanSigner: ChannelKeys, M: Deref + Sync + Send, T: Deref + Sync + Send, K: | |||
| if msg.channel_id == [0; 32] { | |||
| for chan in self.list_channels() { | |||
| if chan.remote_network_id == *counterparty_node_id { | |||
| self.force_close_channel(&chan.channel_id); | |||
| let _ = self.force_close_channel(&msg.channel_id); | |||
There was a problem hiding this comment.
Can you add a code comment here and below : "Untrusted messages from peer, we throw away the error if id points to a non-existent channel". A good warning to avoid vulns in in case of new work around this code path.
ChannelManager::force_close_channel does not fail if a non-existing channel id is being passed, making it hard to catch from an API point of view. Makes force_close_channel return in the same way close_channel does so the user calling the method with an unknown id can be warned.
sr-gi
force-pushed
the
cm-force-close-checks
branch
from
03a72f1 to
821f6cd
Compare
TheBlueMatt
added a commit
to TheBlueMatt/rust-lightning
that referenced
this pull request
Feb 3, 2021
When we receive an error message from a peer, it can indicate a channel which we should close. However, we previously did not check that the counterparty who sends us such a message is the counterparty with whom we have the channel, allowing any connected peer to make us force-close any channel we have as long as they know the channel id. This commit simply changes the force-close logic to check that the sender matches the channel's counterparty node_id, though as noted in lightningdevkit#105, we eventually need to change the indexing anyway to allow absurdly terrible peers to open channels with us. Found during review of lightningdevkit#777.
TheBlueMatt
added a commit
to TheBlueMatt/rust-lightning
that referenced
this pull request
Feb 7, 2021
When we receive an error message from a peer, it can indicate a channel which we should close. However, we previously did not check that the counterparty who sends us such a message is the counterparty with whom we have the channel, allowing any connected peer to make us force-close any channel we have as long as they know the channel id. This commit simply changes the force-close logic to check that the sender matches the channel's counterparty node_id, though as noted in lightningdevkit#105, we eventually need to change the indexing anyway to allow absurdly terrible peers to open channels with us. Found during review of lightningdevkit#777.
TheBlueMatt
added a commit
to TheBlueMatt/rust-lightning
that referenced
this pull request
Feb 10, 2021
When we receive an error message from a peer, it can indicate a channel which we should close. However, we previously did not check that the counterparty who sends us such a message is the counterparty with whom we have the channel, allowing any connected peer to make us force-close any channel we have as long as they know the channel id. This commit simply changes the force-close logic to check that the sender matches the channel's counterparty node_id, though as noted in lightningdevkit#105, we eventually need to change the indexing anyway to allow absurdly terrible peers to open channels with us. Found during review of lightningdevkit#777.
valentinewallace
pushed a commit
to valentinewallace/rust-lightning
that referenced
this pull request
Feb 18, 2021
When we receive an error message from a peer, it can indicate a channel which we should close. However, we previously did not check that the counterparty who sends us such a message is the counterparty with whom we have the channel, allowing any connected peer to make us force-close any channel we have as long as they know the channel id. This commit simply changes the force-close logic to check that the sender matches the channel's counterparty node_id, though as noted in lightningdevkit#105, we eventually need to change the indexing anyway to allow absurdly terrible peers to open channels with us. Found during review of lightningdevkit#777.
valentinewallace
pushed a commit
to valentinewallace/rust-lightning
that referenced
this pull request
Feb 19, 2021
When we receive an error message from a peer, it can indicate a channel which we should close. However, we previously did not check that the counterparty who sends us such a message is the counterparty with whom we have the channel, allowing any connected peer to make us force-close any channel we have as long as they know the channel id. This commit simply changes the force-close logic to check that the sender matches the channel's counterparty node_id, though as noted in lightningdevkit#105, we eventually need to change the indexing anyway to allow absurdly terrible peers to open channels with us. Found during review of lightningdevkit#777.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ChannelManager::force_close_channel does not fail if a non-existing channel id is being passed, making it hard to catch from an API point of view.
Makes force_close_channel return in the same way close_channel does so the user calling the method with an unknown id can be warned.
There's a couple of things that may need fixing:
close #773