Harden RLP limits on allocations

[LukaszRozmej]

Changes

• Harden RLP limits on variable length fields
• Introduce RlpLimit struct with decent error messages
• Refactors on some stuff to reduce code.

Types of changes

What types of changes does your code introduce?

• Refactoring
• Other: validation hardening

Testing

Requires testing

• Yes

If yes, did you write tests?

• Yes

Documentation

Requires documentation update

• No

Requires explanation in Release Notes

• No

Remarks

Before if someone said he sent us XXX amount in some collection we could try to allocate that big array in RLP decoding. Now we have more sensible limits.

Where I couldn't easily pass a limit we are now defaulting to max 256K size - maybe still needs a bit more hardening? Should be enough, the longest field I can think of is Transaction.Data and there are Pyspec tests that require 400KB limit there: Collection count of 404000 is over limit 262144, so I went with 1M limit for it.

[benaadams]

What's this

[LukaszRozmej]

Dictionary for custom english words
For Rider spellchecker

[Copilot]

Pull Request Overview

This PR implements RLP (Recursive Length Prefix) allocation limits to harden against malicious peers sending oversized data structures. The changes introduce a new RlpLimit struct that provides type-safe allocation limits with descriptive error messages, replacing the previous unchecked RLP decoding that could allocate arbitrary amounts of memory.

Key changes:

• Added RlpLimit and RlpLimitException classes for limit enforcement
• Enhanced all RLP decode methods to accept optional limit parameters
• Applied specific limits throughout networking and serialization layers
• Improved error handling for RLP limit violations with automatic disconnection

Reviewed Changes

Copilot reviewed 120 out of 120 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/Nethermind/Nethermind.Serialization.Rlp/RlpLimit.cs	New limit struct with type-safe limit definitions and error message generation
src/Nethermind/Nethermind.Serialization.Rlp/RlpLimitException.cs	New exception type for limit violations
src/Nethermind/Nethermind.Serialization.Rlp/RlpStream.cs	Enhanced decode methods with limit parameters and guard checks
src/Nethermind/Nethermind.Network/P2P/ProtocolHandlers/ProtocolHandlerBase.cs	Added RLP limit exception handling with automatic session disconnection
src/Nethermind/Nethermind.Network/P2P/Utils/BackgroundTaskSchedulerWrapper.cs	Enhanced exception handling for different RLP exception types

[emlautarom1]

Not sure if this approach covers use cases like recursive data structures (ex. Tree = Leaf | Node(i64, Tree) where the encoding could look like [1, [2, [...]]]), though this might not be actually needed.

Also, why do we check the limits at the element level rather than at the beggining of decoding? We could set a limit for the maximum amount of bytes to decode (AFAIK we don't do async RLP decoding so we know ahead of time the number of bytes we're working with).

I would add tests that trigger this kind of limits to check that it's behaving as expected.

Lastly, there are a couple of files with unused imports that could be cleaned up.

[LukaszRozmej]

Not sure if this approach covers use cases like recursive data structures (ex. Tree = Leaf | Node(i64, Tree) where the encoding could look like [1, [2, [...]]]), though this might not be actually needed.

Also, why do we check the limits at the element level rather than at the beggining of decoding? We could set a limit for the maximum amount of bytes to decode (AFAIK we don't do async RLP decoding so we know ahead of time the number of bytes we're working with).

I would add tests that trigger this kind of limits to check that it's behaving as expected.

Lastly, there are a couple of files with unused imports that could be cleaned up.

This is a patch for current solution and I want to keep it as small as possible. I would love to move to comprehensive redesign like yours in #7975.

There are some tests already (for receipts for example).

Recursives would be handled well and are not a problem here.

We could decode element by element and potentially grow the buffers, but it would be slower.

[emlautarom1]

Recursives would be handled well and are not a problem here.

Good, that was my major concern.

There are some tests already (for receipts for example).

I see, I've probably missed it (Will_not_serve_receipts_requests_above_512()). If that's enough regarding testing then LGTM.

[LukaszRozmej]

Recursives would be handled well and are not a problem here.

Good, that was my major concern.

There are some tests already (for receipts for example).

I see, I've probably missed it (Will_not_serve_receipts_requests_above_512()). If that's enough regarding testing then LGTM.

I could add tests for all/most of the limits... but not sure if worth it. They are very actively tested in real world scenarios in hive tests for example.

[Marchhill]

Looks good, just a bit worried some of these limits won't be high enough