eth/protocols/eth: Fix overflows in GetBlockHeaders
#31522
Conversation
eth/protocols/eth/handlers.go
Outdated
| next := current + query.Skip + 1 | ||
| if next <= current { // check for overflow | ||
| infos, _ := json.MarshalIndent(peer.Peer.Info(), "", " ") | ||
| peer.Log().Warn("GetBlockHeaders skip overflow attack", "current", current, "skip", query.Skip, "next", next, "attacker", infos) |
There was a problem hiding this comment.
The same, our implementation is resistant with the invalid devp2p message
If overflow happens and the corresponding header is not found, the loop will be terminated very soon;
If overflow happens and the corresponding header exists, the loop continue but the maximum number of headers can be served is still capped.
There was a problem hiding this comment.
It makes no sense to keep emitting warning logs for the invalid devp2p message, as they are allowed by protocol.
The correct way is to ensure these invalid messages are sanitized and handled properly, without the ability to crash the node.
|
@rjl493456442 I removed the log statements. Can you review again? |
|
I don’t think applying this patch is necessary. As mentioned earlier, it’s up to the requester to determine which part of the chain segment they want to request. If the requested data is available, it will be aggregated into the response. Additionally, the response size and serving time are already protected by certain limits. It's not possible for attackers to bypass these constrains and result in the heavy resource abusing. EDIT: discussed with team and we decide we go with this patch. |
This PR handles overflow and underflow attack vectors for the
GetBlockHeadersrequest via specially craftedSkipvalue.The issue was first brought to attention in this issue: erigontech/erigon#13931