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

Merged
merged 5 commits into from
Jun 13, 2024
Merged

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

merged 5 commits into from
Jun 13, 2024

Conversation

jsdanielh
Copy link
Member

@jsdanielh jsdanielh commented May 9, 2024 •
edited
Loading

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 requested review from hrxi and viquezclaudio May 9, 2024 17:50
@jsdanielh jsdanielh force-pushed the jsdanielh/network2 branch from c122841 to 1cfe168 Compare May 9, 2024 19:19
@jsdanielh jsdanielh force-pushed the jsdanielh/network2 branch from 1cfe168 to fd8f7aa Compare May 10, 2024 14:45
@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 Show resolved Hide resolved
@jsdanielh jsdanielh force-pushed the jsdanielh/network2 branch 4 times, most recently from 74ab225 to fc08ae5 Compare May 30, 2024 22:08
@jsdanielh jsdanielh force-pushed the jsdanielh/network2 branch from fc08ae5 to 555eead Compare June 11, 2024 14:30
nibhar
nibhar approved these changes Jun 11, 2024
View reviewed changes
Copy link
Member

@nibhar nibhar left a comment

Choose a reason for hiding this comment

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

LGTM

network-libp2p/src/discovery/behaviour.rs
}
HandlerOutEvent::ObservedAddress { observed_address } => {
self.events
.push_back(ToSwarm::NewExternalAddrCandidate(observed_address));
}
HandlerOutEvent::Update => self.events.push_back(ToSwarm::GenerateEvent(Event::Update)),
HandlerOutEvent::Error(_) => self.events.push_back(ToSwarm::CloseConnection {
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly this is where the StateTransitionTimeout would end up. I may have missed it, but I think some logging would be nice here.

@jsdanielh jsdanielh force-pushed the jsdanielh/network2 branch from 555eead to 2fad9e3 Compare June 13, 2024 06:40
@jsdanielh
network-libp2p: Add limits to the peer contact addresses
9f38752 
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 added 3 commits June 13, 2024 11:40
@jsdanielh
network-libp2p: Reduce the reported observed addresses to 1
e249957 
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
85aafd6 
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
network-libp2p: Add discovery state transition timeout
4d7ec07 
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 jsdanielh force-pushed the jsdanielh/network2 branch 2 times, most recently from e011a71 to 87eeee1 Compare June 13, 2024 09:37
@jsdanielh
network-libp2p: Verify also peer contacts received
68ae7db 
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.
@jsdanielh jsdanielh force-pushed the jsdanielh/network2 branch from 87eeee1 to 68ae7db Compare June 13, 2024 09:53
@jsdanielh jsdanielh merged commit 68ae7db into albatross Jun 13, 2024
6 checks passed
@jsdanielh jsdanielh deleted the jsdanielh/network2 branch June 13, 2024 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hrxi hrxi hrxi left review comments

@Eligioo Eligioo Eligioo left review comments

@nibhar nibhar nibhar approved these changes

@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