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(net): Avoid some self-connection nonce removal attacks #6410

Merged
merged 7 commits into from
Apr 25, 2023

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Mar 27, 2023

Motivation

We want Zebra to continue to reject self-connections, even if a malicious peer replays one of our nonces.

Fixes #6339

Complex Code or Requirements

This code changes concurrent futures with futures-based mutex locking.
The amount of work done in the critical sections is small.

Solution

  • Check for duplicate nonces from our internal random number generator
  • Stop removing remote nonces after the first attempt that uses them: all attempts should be rejected
  • Add a test that checks that self-connections fail

Manual Testing

I ran:

cd zebra-network
RUST_LOG=info cargo test -- --nocapture self_connections_should_fail

And got these logs:

...
2023-04-19T02:36:56.524554Z  INFO crawl_and_dial{new_peer_interval=1s}: starting the peer address crawler crawl_new_peer_interval=1s outbound_connections=0
2023-04-19T02:36:56.525989Z  INFO accept_inbound_connections{min_inbound_peer_connection_interval=1s listener_addr=Ok(127.0.0.1:39689)}:listen_accept{peer=In("127.0.0.1:39760")}: rejecting self-connection attempt connected_addr=In("127.0.0.1:39760")
2023-04-19T02:36:56.526043Z  INFO accept_inbound_connections{min_inbound_peer_connection_interval=1s listener_addr=Ok(127.0.0.1:39689)}:listen_accept{peer=In("127.0.0.1:39760")}: rejecting self-connection attempt connected_addr=Out("127.0.0.1:39689")
...

This shows that both sides of a self-connection reject that connection with a nonce reuse error.

Review

This is a routine audit fix.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

Follow Up

Add more unit tests:

  • self-connections fail specifically because of a nonce rejection error
  • multiple self-connections with the same nonce fail

These tests are complicated, so I don't think they are worth the effort to do now.

@teor2345 teor2345 added C-bug Category: This is a bug P-Low ❄️ C-security Category: Security issues I-unbounded-growth Zebra keeps using resources, without any limit I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data A-network Area: Network protocol updates or fixes A-concurrency Area: Async code, needs extra work to make it work properly. C-audit Category: Issues arising from audit findings labels Mar 27, 2023
@teor2345 teor2345 self-assigned this Mar 27, 2023
@codecov
Copy link

codecov bot commented Mar 27, 2023

Codecov Report

Merging #6410 (2325311) into main (7681da3) will decrease coverage by 0.01%.
The diff coverage is 45.45%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6410      +/-   ##
==========================================
- Coverage   77.72%   77.71%   -0.01%     
==========================================
  Files         307      307              
  Lines       40282    40283       +1     
==========================================
- Hits        31310    31307       -3     
- Misses       8972     8976       +4     

@mpguerra mpguerra mentioned this pull request Mar 27, 2023
36 tasks
@teor2345 teor2345 added P-Medium ⚡ and removed P-Low ❄️ I-unbounded-growth Zebra keeps using resources, without any limit C-security Category: Security issues labels Apr 18, 2023
@teor2345 teor2345 force-pushed the net-nonce-cache branch 2 times, most recently from ba4e144 to 7da974d Compare April 19, 2023 01:11
@teor2345 teor2345 marked this pull request as ready for review April 19, 2023 02:33
@teor2345 teor2345 requested a review from a team as a code owner April 19, 2023 02:33
@teor2345 teor2345 requested review from oxarbitrage and removed request for a team April 19, 2023 02:33
@teor2345 teor2345 changed the title fix(net): Avoid some self-connection nonce removal attacks, strengthen inbound peer connection rate-limit fix(net): Avoid some self-connection nonce removal attacks Apr 19, 2023
oxarbitrage
oxarbitrage previously approved these changes Apr 22, 2023
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.

Looks good to me, i made a few optional documentation suggestions.

zebra-network/src/peer/handshake.rs Outdated Show resolved Hide resolved
zebra-network/src/peer_set/initialize/tests/vectors.rs Outdated Show resolved Hide resolved
zebra-network/src/peer_set/initialize/tests/vectors.rs Outdated Show resolved Hide resolved
zebra-network/src/peer_set/initialize/tests/vectors.rs Outdated Show resolved Hide resolved
zebra-network/src/peer_set/initialize/tests/vectors.rs Outdated Show resolved Hide resolved
zebra-network/src/peer_set/initialize/tests/vectors.rs Outdated Show resolved Hide resolved
mergify bot added a commit that referenced this pull request Apr 25, 2023
@mergify mergify bot merged commit 1f8aa3c into main Apr 25, 2023
@mergify mergify bot deleted the net-nonce-cache branch April 25, 2023 12:50
@oxarbitrage oxarbitrage mentioned this pull request May 9, 2023
38 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-concurrency Area: Async code, needs extra work to make it work properly. A-network Area: Network protocol updates or fixes C-audit Category: Issues arising from audit findings C-bug Category: This is a bug I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NCC-E005955-MMC] zebra-network: Uncaught Nonce Reuse and Fragile Nonce Cache Eviction
2 participants