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

security(net): Stop sending peer addresses from handshakes directly to the address book #7977

Merged
merged 4 commits into from
Nov 24, 2023

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Nov 22, 2023

Motivation

We want to stop sending connection addresses directly to the address book, because that's insecure.

Close #7951

PR Author Checklist

Check before marking the PR as ready for review:

  • Will the PR name make sense to users?
  • Does the PR have a priority label?
  • Have you added or updated tests?
  • Is the documentation up to date?
For significant changes:
  • Is there a summary in the CHANGELOG?
  • Can these changes be split into multiple PRs?

If a checkbox isn't relevant to the PR, mark it as done.

Complex Code or Requirements

This is actually simpler than the previous code, because it doesn't have concurrency issues with other updates for the same address.

Using gossiped addresses changes the outbound connection priority slightly.

Solution

  • Send handshake peer addresses to the per-connection address cache
  • Update documentation

Testing

@arya2 what tests should we write here?
Is inspection enough, or should we have a "not sent to the address book" or "sent to the cache" test?

Review

This is a routine fix.

Reviewer Checklist

Check before approving the PR:

  • Does the PR scope match the ticket?
  • Are there enough tests to make sure it works? Do the tests cover the PR motivation?
  • Are all the PR blockers dealt with?
    PR blockers can be dealt with in new tickets or PRs.

And check the PR Author checklist is complete.

Follow Up Work

There are some cleanups in #7824 that depend on this.

@teor2345 teor2345 added P-Medium ⚡ C-security Category: Security issues I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data A-network Area: Network protocol updates or fixes I-remote-trigger Remote nodes can make Zebra do something bad labels Nov 22, 2023
@teor2345 teor2345 self-assigned this Nov 22, 2023
@teor2345 teor2345 requested a review from a team as a code owner November 22, 2023 01:22
@teor2345 teor2345 requested review from upbqdn and removed request for a team November 22, 2023 01:22
upbqdn
upbqdn previously approved these changes Nov 23, 2023
Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

All changes in this PR look good to me, so I'm approving. However, I don't know the implications of sending handshake peer addresses to the per-connection address cache.

zebra-network/src/peer/connection.rs Outdated Show resolved Hide resolved
@teor2345
Copy link
Contributor Author

However, I don't know the implications of sending handshake peer addresses to the per-connection address cache.

This PR prevents two attacks:

  • peers modifying each others' addresses using version messages
  • peers filling the address book by repeatedly connecting and sending different addresses in each version message

Here are some potential drawbacks:

  • We might lose these addresses when the peer sends an addr message (but addr messages should always contain that peer's address, so in some cases we might lose the remote address of inbound connections). This is fixed in security: Limit how many addresses we use from each peer address message #7952 by adding all new addresses to the cache, taking any that are needed, then truncating the rest if needed.
  • These addresses are treated like gossiped addresses, so they have slightly higher priority. This is unlikely to have a significant impact, because their times will quickly become outdated by new addresses from this peer or other peers.

Co-authored-by: teor <teor@riseup.net>
@teor2345
Copy link
Contributor Author

The macOS job seems to have failed due to a GitHub runner error, this is an issue with their infrastructure. There is no output in the failed steps on the GUI or in the raw logs, there's just this:

2023-11-23T13:58:23.3683400Z Requested labels: macos-latest
2023-11-23T13:58:23.3683747Z Job defined at: ZcashFoundation/zebra/.github/workflows/ci-unit-tests-os.yml@refs/pull/7977/merge
2023-11-23T13:58:23.3683911Z Waiting for a runner to pick up this job...
2023-11-23T13:58:25.8364996Z Job is waiting for a hosted runner to come online.
2023-11-23T13:58:29.5183703Z Job is about to start running on the hosted runner: GitHub Actions 22 (hosted)

https://github.com/ZcashFoundation/zebra/actions/runs/6970673248/job/18969099332?pr=7977

Screenshot 2023-11-24 at 09 01 07

mergify bot added a commit that referenced this pull request Nov 24, 2023
@mergify mergify bot merged commit f08cc2d into main Nov 24, 2023
97 checks passed
@mergify mergify bot deleted the vers-to-cache branch November 24, 2023 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-network Area: Network protocol updates or fixes C-security Category: Security issues I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data I-remote-trigger Remote nodes can make Zebra do something bad
Projects
None yet
2 participants