Place access list deserialization guards#9466
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds security hardening to the AccessListForRpc deserialization to prevent memory exhaustion attacks when processing large or malicious requests to eth_sendTransaction. The implementation replaces automatic JSON deserialization with manual parsing that enforces strict limits on the number of access list items and storage keys.
Key Changes:
- Implemented custom JSON deserialization with memory usage guards
- Added configurable limits: max 1000 items per access list, max 1000 storage keys per item, max 10000 total storage keys
- Enhanced input validation with explicit checks for expected JSON token types
| else if (reader.TokenType == JsonTokenType.StartArray) | ||
| { | ||
| storageKeys = new List<UInt256>(); | ||
| int keyCount = 0; |
There was a problem hiding this comment.
[nitpick] The variable keyCount tracks storage keys per item while storageItemsCount tracks total storage keys across all items. Consider renaming keyCount to itemStorageKeyCount or keysInCurrentItem to clarify the distinction and improve code readability.
Shouldn't they be failing now if they covered? Since you are throwing new exceptions |
There are standard tests for this endpoint, they still work (and helped me to fix a bug). I generated base for this code with ChatGPT and didn't have to do much changes, so AI:
|
…ForRpc.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ForRpc.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Place access list deserialization guards * refactor * fix property casing * Update src/Nethermind/Nethermind.Facade/Eth/RpcTransaction/AccessListForRpc.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update src/Nethermind/Nethermind.Facade/Eth/RpcTransaction/AccessListForRpc.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * rename --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Changes
AccessListForRpcdeserializationeth_sendTransactionTypes of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Existing tests should cover it.
Documentation
Requires documentation update
Requires explanation in Release Notes