Skip to content
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

fix: don't ban a peer for sending a banned peer #5843

Conversation

stringhandler
Copy link
Collaborator

Description

Removes a check that would ban a peer if they sent a peer that we have banned.

Motivation and Context

The check would ban the source peer during peer sync if they sent a peer to us that we have banned. This is IMO exploitable, since a peer may not have connected to the banned peer in question and thus not know if it should be banned or not.

You could also get a peer banned in this manner:

  1. As an evil peer, I connect to a target node A.
  2. I then connect to a number of other nodes and get myself banned.
  3. The nodes I connect to then will ban node A when they send my node address during sync

How Has This Been Tested?

CI

What process can a PR reviewer use to test or verify this change?

Not sure it's quite a mission to replicate this.

Breaking Changes

  • None
  • Requires data directory on base node to be deleted
  • Requires hard fork
  • Other - Please specify

@ghpbot-tari-project ghpbot-tari-project added the P-acks_required Process - Requires more ACKs or utACKs label Oct 18, 2023
@github-actions
Copy link

Test Results (CI)

1 222 tests   1 222 ✔️  9m 51s ⏱️
     39 suites         0 💤
       1 files           0

Results for commit 619949a.

Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

Nice - so this was handled correctly discovery response but not in peer sync, think makes sense that the peer validator doesn't care that the peer is banned and that even a banned peer can have its peer record updated (as long as it stays banned)

@github-actions
Copy link

Test Results (Integration tests)

33 tests   33 ✔️  13m 46s ⏱️
11 suites    0 💤
  2 files      0

Results for commit 619949a.

@stringhandler stringhandler merged commit 12f8a75 into tari-project:development Oct 18, 2023
14 checks passed
@stringhandler stringhandler deleted the st-dont-ban-during-peer-sync branch October 18, 2023 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants