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.

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

Jump to bottom

Stop closing connections on unexpected messages, Credit: Equilibrium #3120

Merged
merged 6 commits into from
Nov 30, 2021
Merged

Stop closing connections on unexpected messages, Credit: Equilibrium #3120

merged 6 commits into from
Nov 30, 2021
+235 −89

Conversation

teor2345
Copy link
Contributor

Motivation

Zebra disconnects from peers when they send unexpected messages, and when they send NotFound messages.

We should ignore those messages instead, to stop denial of service and eclipse attacks.

Closes #2107 (minimal fix)

Specifications

Bitcoin uses a ban-score per connection or IP address, rather than disconnecting on the first bad message:
https://developer.bitcoin.org/devguide/p2p_network.html#misbehaving-nodes

Solution

  • Ignore known but unsupported Messages from peers
  • Ignore unknown message commands from peers
  • Stop ignoring some completed Responses (bug fix on an earlier PR which ignored some unexpected messages)
  • Stop returning NotFound messages as errors, finish the current response instead

Review

@oxarbitrage can review this PR. It is a lower priority than the NU5 work.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

We need to refactor the Connection code before we can add tests for it.

@teor2345 teor2345 added C-bug Category: This is a bug P-Medium C-security Category: Security issues I-hang A Zebra component stops responding to requests I-slow Problems with performance or responsiveness I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data A-network Area: Network protocol updates or fixes labels Nov 30, 2021
@teor2345 teor2345 requested a review from oxarbitrage November 30, 2021 03:48
@teor2345 teor2345 self-assigned this Nov 30, 2021
oxarbitrage
oxarbitrage approved these changes Nov 30, 2021
View reviewed changes
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

nice work, LGTM!

@oxarbitrage oxarbitrage enabled auto-merge (squash) November 30, 2021 18:03
@oxarbitrage oxarbitrage merged commit a358c41 into main Nov 30, 2021
@oxarbitrage oxarbitrage deleted the allow-unexpected-messages branch November 30, 2021 19:26
This was referenced Dec 1, 2021
Merged
Closed
Merged
teor2345 added a commit that referenced this pull request Dec 21, 2021
@teor2345
Revert "Stop ignoring some completed Responses"
b07d636 
This reverts commit 0383562 from PR #3120,
but keeps the metrics and logging changes since that commit.
This was referenced Dec 21, 2021
Merged
Closed
Merged
conradoplg pushed a commit that referenced this pull request Dec 21, 2021
@teor2345
Revert "Stop ignoring some completed Responses" (#3274)
8db0528 
* Revert "Stop ignoring some completed Responses"

This reverts commit 0383562 from PR #3120,
but keeps the metrics and logging changes since that commit.

* Document why the request handling needs to happen in this order
@conradoplg conradoplg mentioned this pull request Jan 14, 2022
Closed
@teor2345 teor2345 mentioned this pull request Jun 7, 2022
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oxarbitrage oxarbitrage oxarbitrage approved these changes

Assignees

@teor2345 teor2345

Labels
A-network Area: Network protocol updates or fixes C-bug Category: This is a bug C-security Category: Security issues I-hang A Zebra component stops responding to requests I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data I-slow Problems with performance or responsiveness
Projects
None yet
Milestone
No milestone
2 participants
@teor2345 @oxarbitrage