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

[Merged by Bors] - Limit snappy input stream #1738

Closed
wants to merge 10 commits into from

Conversation

pawanjay176
Copy link
Member

@pawanjay176 pawanjay176 commented Oct 6, 2020

Issue Addressed

N/A

Proposed Changes

This PR limits the length of the stream received by the snappy decoder to be the maximum allowed size for the received rpc message type. Also adds further checks to ensure that the length specified in the rpc encoding-dependent header is within the bounds for the rpc message type being decoded.

@pawanjay176 pawanjay176 changed the title Mitigate snappy malicious messages Limit snappy input stream Oct 6, 2020
@pawanjay176 pawanjay176 marked this pull request as ready for review October 6, 2020 21:00
Copy link

@protolambda protolambda left a comment

Choose a reason for hiding this comment

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

Nice test and good changes, but think I found a minor bug in the same code. Metadata requests should be 0 bytes, but it looks like the length bytes are still read.

beacon_node/eth2_libp2p/src/rpc/codec/ssz_snappy.rs Outdated Show resolved Hide resolved
Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

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

This looks good. Nice work!

Although a little more effort, I think we might benefit from implementing an RPC Limit struct type with max and min fields rather than the tuple.

I think it will make the code a little more readable and hopefully prevent mixing up the tuple indexes when doing things like length > limit.1 || length < limit.0

What do you think?

@pawanjay176
Copy link
Member Author

Agree this is ugly. Will make the changes :)

@AgeManning
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Oct 11, 2020
## Issue Addressed

N/A

## Proposed Changes

This PR limits the length of the stream received by the snappy decoder to be the maximum allowed size for the received rpc message type. Also adds further checks to ensure that the length specified in the rpc [encoding-dependent header](https://github.com/ethereum/eth2.0-specs/blob/dev/specs/phase0/p2p-interface.md#encoding-strategies) is within the bounds for the rpc message type being decoded.
@bors bors bot changed the title Limit snappy input stream [Merged by Bors] - Limit snappy input stream Oct 11, 2020
@bors bors bot closed this Oct 11, 2020
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.

3 participants