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

network-libp2p: Add fixes and improvements to discovery protocol #2461

Open
wants to merge 5 commits into
base: albatross
Choose a base branch
from
Open

network-libp2p: Add fixes and improvements to discovery protocol #2461

wants to merge 5 commits into from
+161 −99

Conversation

jsdanielh
Copy link
Contributor

@jsdanielh jsdanielh commented May 9, 2024 •
edited

What's in this pull request?

  • Add verification of peer contacts received from the different messages that can transmit a list of peer contacts in the discovery protocol. The verification includes checks for appropriate lengths, signatures and number of addresses.
  • Add timeout for the state transition in the discovery handler such that a peer don't hold us in a state indefinitely if it doesn't send us back a message we expect.
    This fixes DoS in Discovery Handler  #2458.
  • Do not use the observed addresses sent through the discovery protocol since these addresses are not authenticated and it's been observed that they are not adding any useful address to a peer and instead were adding addresses with incorrect TCP/UDP ports.
  • Reduce the reported observed addresses to a single address within the discovery protocol. This avoids DoS attacks from a peer sending several observed addresses for a specific connection. In terms of functionality, it stays the same since we were only reporting a single observed address (the address that got us a connection). Also reduced the interaction between the handler and the behaviour by setting handler attributes on the constructor instead of using events to the handler.
  • Add limits to the peer contact addresses and add the ability to verify that these limits are followed when a peer contact is received from the network (discovery protocol).

Pull request checklist

  • All tests pass. The project builds and runs.
  • I have resolved any merge conflicts.
  • I have resolved all clippy and rustfmt warnings.

@jsdanielh jsdanielh added this to the Nimiq PoS Mainnet milestone May 9, 2024
@jsdanielh jsdanielh added the testnet-restart Change breaks the protocol and requires a testnet restart label May 14, 2024
@jsdanielh jsdanielh force-pushed the jsdanielh/network2 branch 5 times, most recently from 2b9c6d0 to 1bfe8cd Compare May 20, 2024 17:40
@jsdanielh jsdanielh requested a review from nibhar May 20, 2024 17:40
@jsdanielh jsdanielh force-pushed the jsdanielh/network2 branch 7 times, most recently from 5777122 to 2377895 Compare May 27, 2024 12:52
hrxi
hrxi reviewed May 29, 2024
View reviewed changes
Copy link
Contributor

@hrxi hrxi 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, left some comments.

network-libp2p/src/discovery/handler.rs Outdated Show resolved Hide resolved
network-libp2p/src/discovery/handler.rs Outdated Show resolved Hide resolved
network-libp2p/src/discovery/peer_contacts.rs Outdated Show resolved Hide resolved
network-libp2p/tests/network.rs Outdated Show resolved Hide resolved
@jsdanielh
network-libp2p: Add limits to the peer contact addresses
7c830a8 
Add limits to the peer contact addresses and add the ability to
verify that these limits are followed when a peer contact is received
from the network (discovery protocol).
@jsdanielh
network-libp2p: Reduce the reported observed addresses to 1
398394d 
Reduce the reported observed addresses to a single address within the
discovery protocol. This avoids DoS attacks from a peer sending
several observed addresses for a specific connection. In terms of
functionality, it stays the same since we were only reporting a
single observed address (the address that got us a connection).
Also reduced the interaction between the handler and the behaviour
by setting handler attributes on the constructor instead of using
events to the handler.
@jsdanielh
network-libp2p: Do not use the observed addresses
a7e0292 
Do not use the observed addresses sent through the discovery protocol
since these addresses are not authenticated and it's been observed
that they are not adding any useful address to a peer and instead
were adding addresses with incorrect TCP/UDP ports.
@jsdanielh jsdanielh force-pushed the jsdanielh/network2 branch 3 times, most recently from 91b3003 to 74ab225 Compare May 30, 2024 21:38
@jsdanielh
network-libp2p: Add discovery state transition timeout
70cb187 
Add timeout for the state transition in the discovery handler such
that a peer don't hold us in a state indefinitely if it doesn't send
us back a message we expect.

This fixes #2458.
@jsdanielh
network-libp2p: Verify also peer contacts received
fc08ae5 
Add verification of peer contacts received from the different
messages that can transmit a list of peer contacts in the discovery
protocol. The verification includes checks for appropriate lengths,
signatures and number of addresses.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Eligioo Eligioo Eligioo left review comments

@hrxi hrxi hrxi left review comments

@nibhar nibhar nibhar left review comments

@viquezclaudio viquezclaudio Awaiting requested review from viquezclaudio

Assignees
No one assigned
Labels
testnet-restart Change breaks the protocol and requires a testnet restart
Projects
None yet
Milestone
Nimiq PoS Mainnet
Development

Successfully merging this pull request may close these issues.

DoS in Discovery Handler
4 participants
@jsdanielh @Eligioo @hrxi @nibhar