Correctly handle BADONION onion errors #1703
Merged
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
Codecov Report
@@ Coverage Diff @@
## main #1703 +/- ##
==========================================
+ Coverage 90.92% 91.67% +0.75%
==========================================
Files 86 86
Lines 46417 52678 +6261
Branches 46417 52678 +6261
==========================================
+ Hits 42204 48294 +6090
- Misses 4213 4384 +171
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
dunxen
reviewed
Sep 8, 2022
Should have something up today. Will open against your branch. |
TheBlueMatt
force-pushed
the
2022-09-badonion-first-check
branch
from
baff002 to
32df0cc
Compare
|
Thanks @dunxen! Added your test. |
TheBlueMatt
force-pushed
the
2022-09-badonion-first-check
branch
from
32df0cc to
befbfe9
Compare
dunxen
previously approved these changes
Sep 12, 2022
Currently we entirely ignore the BADONION bit when deciding how to handle HTLC failures. This opens us up to an attack where a malicious node always fails HTLCs backwards via `update_fail_malformed_htlc` with an error code of `BADONION|NODE|PERM|X`. In this case, we may decide to interpret this as a permanent node failure for the node encrypting the onion, i.e. the counterparty of the node who sent the `update_fail_malformed_htlc` message and ultimately failed the HTLC. Thus, any node we route through could cause us to fully remove its counterparty from our network graph. Luckily we do not do any persistent tracking of removed nodes, and thus will re-add the removed node once it is re-announced or on restart, however we are likely to add such persistent tracking (at least in-memory) in the future.
TheBlueMatt
force-pushed
the
2022-09-badonion-first-check
branch
from
befbfe9 to
3b1d0e5
Compare
Some tweaks by Matt Corallo <[email protected]>
TheBlueMatt
force-pushed
the
2022-09-badonion-first-check
branch
from
3b1d0e5 to
e6a3c23
Compare
|
Squashed without further changes. |
dunxen
approved these changes
Sep 13, 2022
wpaulino
approved these changes
Sep 13, 2022
valentinewallace
approved these changes
Sep 13, 2022
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.
Currently we entirely ignore the BADONION bit when deciding how to handle HTLC failures. This opens us up to an attack where a malicious node always fails HTLCs backwards via
update_fail_malformed_htlcwith an error code ofBADONION|NODE|PERM|X. In this case, we may decide to interpret this as a permanent node failure for the node encrypting the onion, i.e. the counterparty of the node who sent theupdate_fail_malformed_htlcmessage and ultimately failed the HTLC.Thus, any node we route through could cause us to fully remove its counterparty from our network graph. Luckily we do not do any persistent tracking of removed nodes, and thus will re-add the removed node once it is re-announced or on restart, however we are likely to add such persistent tracking (at least in-memory) in the future.
This probably needs test updates.