Penalize peers that send an invalid rpc request #6986
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ackintosh
commented
Feb 12, 2025
Comment on lines
840
to
853
| ConnectionEvent::ListenUpgradeError(e) => { | ||
| if matches!(e.error, RPCError::InvalidData(_)) { | ||
| // Peer is not complying with the protocol. Notify the application and disconnect. | ||
| let inbound_substream_id = self.current_inbound_substream_id; | ||
| self.current_inbound_substream_id.0 += 1; | ||
|
|
||
| self.events_out.push(HandlerEvent::Err(HandlerErr::Inbound { | ||
| id: inbound_substream_id, | ||
| proto: Protocol::DataColumnsByRange, // FIXME: replace this hardcoded protocol | ||
| error: e.error, | ||
| })); | ||
| self.shutdown(None); | ||
| } | ||
| } |
There was a problem hiding this comment.
// FIXME: replace this hardcoded protocol
The RPCError::InvalidData error is emitted by the inbound codec here. The problem I'm facing is that the handler cannot determine which protocol the invalid data belongs to. I think changing the type of InboundUpgrade::Error from RPCError to (Protocol, RPCError) might solves this. What do you think?
ackintosh
changed the title
ackintosh
changed the title
ackintosh
added
ready-for-review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue Addressed
Since #6847, invalid
BlocksByRange/BlobsByRangerequests, which do not comply with the spec, are handled in the Handler. Any peer that sends an invalid request is penalized and disconnected.However, other kinds of invalid rpc request, which result in decoding errors, are just dropped. No penalty is applied and the connection with the peer remains.
Proposed Changes
I have added handling for the
ListenUpgradeErrorevent to notify the application of anRPCError:InvalidDataerror and disconnect to the peer that sent the invalid rpc request.I also added tests for handling invalid rpc requests.