-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Protect vectorized payloads from wasteful and unneeded deserializations #6782
Open
AndreaLanfranchi
wants to merge
18
commits into
zcash:master
Choose a base branch
from
AndreaLanfranchi:vectorized-payloads
base: master
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Protect vectorized payloads from wasteful and unneeded deserializations #6782
AndreaLanfranchi
wants to merge
18
commits into
zcash:master
from
AndreaLanfranchi:vectorized-payloads
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
Payload gets discarded regardless its content
Must be a multiple of underlying item
Avoid deserialization of huge payloads
Avoid deserialization of huge payloads
Also check (optionally) vector holds at least one item
Item size is variable on behalf of Inv type
Make it clear (beyond comment) we're deliberately ignoring a return value It also makes clear we're not calling a void
daira
added
I-dos
Problems and improvements with respect to Denial-of-Service.
safe-to-build
Used to send PR to prod CI environment
labels
Jan 29, 2024
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.
Abstract
Several message types carry vectorized payloads. There are limits to the number of items such payloads can hold (e.g. an
inv
message can't carry more than 50k items).Issue
The check for max items threshold exceeded happens after the deserialization has been performed and therefore the code discards a content which had an additional cost to be produced. See here for example.
As a result it can happen a malicious peer can send several
inv
messages with way more items than max allowed per message before being disconnected for excessive penalization which leads to wasteful usage of resources and possibly increases the surface of attack for DoS.Proposed solution
Early perform a lightweight check on behalf of raw datastream contents before starting the deserialization (and creation of vector) and return immediately in case of validation failure. This is a revised version of the partially implemented solution I pointed out on Horizen's Zend
The overall implementation adds negligible overhead to the processing of legit messages.
The checking function also verifies (optionally) whether the vector of items is actually not empty : this is to avoid a flood of messages (e.g.
inv
) with empty payloads that has the only goal to waste cycles for processing totally un-useful messages without any penalization.Thank you for your attention.
Addendum
Questions for the devs:
getblocks
andgetheaders
should be standardized. The latter discards the message if IBD is true whilegetblocks
doesn't. Which of the two is more appropriate ? Or is it correct as is ?getheaders
andgetblocks
the max number of block locator hashes must not exceed 2'000. Unless I miss something there's not such a check in the deserialization of the payload and as a result the code could end up deserializing up to 65.5K hashes in a single message. Is it worth checking early the size of the vector ?