Check the PK of the source of an error before closing chans from it #787
Conversation
Codecov Report
@@ Coverage Diff @@
## main #787 +/- ##
==========================================
- Coverage 90.79% 90.75% -0.05%
==========================================
Files 38 44 +6
Lines 23168 24119 +951
==========================================
+ Hits 21036 21888 +852
- Misses 2132 2231 +99
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I'm sad to hear about this bug. Given that channel id are made of the funding_txid & funding_output_index, a peer could have reasonably guessed our channel ids by observing onchain LN transactions...
Thinking further, we should modify the spec to blind channel id with a secret only known by channel counterparties, maybe even scoping short_chan_id for private channels ?
| @@ -3479,7 +3489,7 @@ impl<ChanSigner: ChannelKeys, M: Deref + Sync + Send, T: Deref + Sync + Send, K: | |||
| } | |||
| } else { | |||
There was a problem hiding this comment.
About the zero-value channel id branch, wouldn't be better to sanitize early that case at open_channel reception ? Such value would be a hint of either our CSRNG being burst of the counterparty being buggy or sneaky. Either way we should discard that value from our internal channel map ?
There was a problem hiding this comment.
Hmm? Not sure exactly what you mean - zero-value channel id means "close all channels with me", its not a specific reference to a channel.
We should just implement #105 - it'll solve this bug and also we can do per-channel locking at the same time. |
TheBlueMatt
force-pushed
the
2021-02-check-close-source
branch
from
d0c8792 to
7c0b55d
Compare
|
Added a test, as requested by @ariard on IRC, though I didn't remove the duplicate-closure on |
TheBlueMatt
force-pushed
the
2021-02-check-close-source
branch
from
7c0b55d to
97d6d91
Compare
|
Code Review ACK 97d6d91 modulo commit squash. Thanks for adding test! |
|
Two typos (f, isntead) in the commit name |
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
force-pushed
the
2021-02-check-close-source
branch
from
97d6d91 to
8ca03f8
Compare
|
Fixed the typo. Note that the "f..." commits, generally are intended to be squashed into previous commits before merge, they exist just to aid reviewers in seeing changes between revisions. |
TheBlueMatt
force-pushed
the
2021-02-check-close-source
branch
from
8ca03f8 to
bd8382a
Compare
|
Went ahead and squashed, only code change was the doc typo. |
|
re-Code Review ACK bd8382a, changes are typo/squash |
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 #105, we eventually need to change the indexing anyway to allow
absurdly terrible peers to open channels with us.
Found during review of #777.
Plus threw in a fix for docs on peer handler which I found while writing a SocketDescriptor wrapper in Java NIO.