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

Missing flood protection check for number of message IDs when handling Ihave messages #560

Merged
merged 2 commits into from
Jun 27, 2024

Conversation

cortze
Copy link
Contributor

@cortze cortze commented Jun 27, 2024

Description

After looking at the gossipsub Iwant/Ihave handlers, I realised that there is a missing spam prevention check when opening the Ihave messages from a received ControlMessage.

To give some context, the spec defines that each Ihave message should have an upper limit of aggregated message IDs, which in this go implementation is defined by maxMessageLength = 5_000 message IDs per Ihave and topic, as Ihave messages can be aggregated in a single RPC call.

This spam check is already done at the moment of emitting the Gossips. However, I can't see a similar check at the moment of handling each of the message IDs within the Ihave messages.

We do checked that the remote peer stayed within the number of Ihave messages, and that we didn't exceed the message ID count for asked messages, but it doesn't check anything related to the size of the array of message IDs per each topic I have.

This PR adds that missing spam protection step, ensuring that we only read the first maxMessageLength messages of each Ihave as @MarcoPolo suggested.

Let me know if there is anything else that should be added or modified.

gossipsub.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

fair enough.

@vyzo vyzo merged commit 8e498e9 into libp2p:master Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants