From 3c5cca09064b76690f3c8dc77d0c31534cd3eb05 Mon Sep 17 00:00:00 2001 From: ozpool Date: Thu, 30 Apr 2026 15:44:48 +0530 Subject: [PATCH 1/3] eth/protocols/snap: validate trie node path length A peer-supplied path in a GetTrieNodes request is fed straight to accTrie.GetNode (or stTrie.GetNode), which calls compactToHex on the path before traversing the trie. compactToHex allocates 2*len(path)+1 nibbles regardless of whether the path can possibly match a node. Trie keys in any Ethereum state trie are Keccak256 hashes (32 bytes = 64 nibbles), so the longest valid compact-encoded path is 33 bytes (64 nibbles + 1 prefix byte). Anything longer is structurally invalid and cannot address a node. Currently the handler accepts arbitrary-length paths up to the 10MB message cap, performing a doomed trie traversal for each one. This gives a peer a cheap way to amplify CPU/memory usage on the server and silently makes geth a non-conforming reference for the devp2p snap test suite (clients that reject long paths at the protocol layer fail tests that pin geth's accept-and-empty-respond behavior). Add a guard at both the account-path and storage-path branches of ServiceGetTrieNodesQuery: if the path exceeds maxTrieNodePathLength (33 bytes), append a nil placeholder and skip the trie walk. The nil placeholder preserves positional alignment with the request and matches the existing wire behavior for paths that resolve to a non-existent node, so this is a pure CPU/allocation fix and not a protocol change. Adds unit tests in handler_test.go covering account paths just over the limit and far over the limit. The storage-path branch shares the same guard but exercising it requires a chain with existing accounts; that coverage is left as a follow-up. Refs #34853. --- eth/protocols/snap/handler.go | 8 ++++ eth/protocols/snap/handler_test.go | 65 ++++++++++++++++++++++++++++++ eth/protocols/snap/handlers.go | 20 +++++++++ 3 files changed, 93 insertions(+) diff --git a/eth/protocols/snap/handler.go b/eth/protocols/snap/handler.go index 26545f2960..30ebe9864d 100644 --- a/eth/protocols/snap/handler.go +++ b/eth/protocols/snap/handler.go @@ -52,6 +52,14 @@ const ( // If we spend too much time, then it's a fairly high chance of timing out // at the remote side, which means all the work is in vain. maxTrieNodeTimeSpent = 5 * time.Second + + // maxTrieNodePathLength is the longest a compact-encoded trie path can be + // while still addressing a node in any Ethereum state trie. Trie keys are + // Keccak256 hashes (32 bytes = 64 nibbles), and the compact encoding adds + // at most one prefix byte, giving 32+1 = 33 bytes. Anything longer is + // structurally invalid and cannot match a node, so the handler skips it + // without performing a trie traversal. + maxTrieNodePathLength = 33 ) // Handler is a callback to invoke from an outside runner after the boilerplate diff --git a/eth/protocols/snap/handler_test.go b/eth/protocols/snap/handler_test.go index b0522c20bb..f44dfd125a 100644 --- a/eth/protocols/snap/handler_test.go +++ b/eth/protocols/snap/handler_test.go @@ -220,6 +220,71 @@ func TestServiceGetAccessListsQueryByteLimit(t *testing.T) { } } +// TestServiceGetTrieNodesQueryPathLength verifies that ServiceGetTrieNodesQuery +// rejects structurally invalid (over-length) compact-encoded paths without +// allocating in compactToHex or walking the trie. Trie keys are Keccak256 +// hashes, so the longest valid compact-encoded path is 33 bytes; anything +// longer cannot match a node in any state trie. A peer sending such a path +// (up to the 10MB request size cap) would otherwise force the server to +// allocate 2*len(path)+1 nibbles per entry and start a doomed traversal. +func TestServiceGetTrieNodesQueryPathLength(t *testing.T) { + t.Parallel() + + bc, _, _ := getChainWithBALs(1, 100) + defer bc.Stop() + + root := bc.CurrentBlock().Root + + // One byte beyond the spec-aligned cap (33 bytes). + overlong := make([]byte, maxTrieNodePathLength+1) + for i := range overlong { + overlong[i] = byte(i) + } + // Far over the cap, exercising the worst-case allocation path. + gigantic := make([]byte, 1024) + for i := range gigantic { + gigantic[i] = byte(i) + } + + tests := []struct { + name string + paths []TrieNodePathSet + }{ + { + name: "account path one byte over limit", + paths: []TrieNodePathSet{{overlong}}, + }, + { + name: "account path far over limit", + paths: []TrieNodePathSet{{gigantic}}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + encPaths, err := rlp.EncodeToRawList(tt.paths) + if err != nil { + t.Fatalf("encode paths: %v", err) + } + req := &GetTrieNodesPacket{ + ID: 1, + Root: root, + Paths: encPaths, + Bytes: softResponseLimit, + } + nodes, err := ServiceGetTrieNodesQuery(bc, req) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(nodes) != 1 { + t.Fatalf("expected 1 placeholder node, got %d", len(nodes)) + } + if len(nodes[0]) != 0 { + t.Errorf("expected nil placeholder for oversized path, got %x", nodes[0]) + } + }) + } +} + // TestGetAccessListResponseDecoding verifies that an AccessListsPacket // round-trips through RLP encode/decode, preserving positional // correspondence and correctly representing absent BALs as empty strings. diff --git a/eth/protocols/snap/handlers.go b/eth/protocols/snap/handlers.go index 5a5733bdb4..90fc185027 100644 --- a/eth/protocols/snap/handlers.go +++ b/eth/protocols/snap/handlers.go @@ -469,6 +469,15 @@ func ServiceGetTrieNodesQuery(chain *core.BlockChain, req *GetTrieNodesPacket) ( if accKey == nil { return nodes, fmt.Errorf("%w: invalid account node request", errBadRequest) } + // Skip structurally invalid paths (longer than maxTrieNodePathLength) + // without traversing the trie. compactToHex would otherwise allocate + // 2*len(accKey)+1 bytes for a key that cannot possibly match any node. + // A nil placeholder is appended to preserve positional alignment with + // the request, matching the existing behavior for non-existent nodes. + if len(accKey) > maxTrieNodePathLength { + nodes = append(nodes, nil) + break + } blob, resolved, err := accTrie.GetNode(accKey) loads += resolved // always account database reads, even for failures if err != nil { @@ -513,6 +522,17 @@ func ServiceGetTrieNodesQuery(chain *core.BlockChain, req *GetTrieNodesPacket) ( if err != nil { return nil, fmt.Errorf("%w: invalid storage key: %v", errBadRequest, err) } + // Skip structurally invalid paths (longer than maxTrieNodePathLength) + // without traversing the storage trie. See the account-path branch + // above for the rationale; we still append a nil placeholder so + // the response stays positionally aligned with the request. + if len(path) > maxTrieNodePathLength { + nodes = append(nodes, nil) + if bytes > req.Bytes || loads > maxTrieNodeLookups || time.Since(start) > maxTrieNodeTimeSpent { + break + } + continue + } blob, resolved, err := stTrie.GetNode(path) loads += resolved // always account database reads, even for failures if err != nil { From d68ff2bc91879b5a32870fd7463f4d9929c7b630 Mon Sep 17 00:00:00 2001 From: ozpool Date: Thu, 30 Apr 2026 18:41:57 +0530 Subject: [PATCH 2/3] ci: retrigger after unrelated TestWalletNotifications flake From 5dbf26db90c84e77560bff62cab3e4507b650636 Mon Sep 17 00:00:00 2001 From: ozpool Date: Wed, 13 May 2026 12:08:14 +0530 Subject: [PATCH 3/3] =?UTF-8?q?eth/protocols/snap:=20address=20review=20?= =?UTF-8?q?=E2=80=94=20if-else=20guard,=20tighten=20path=20limit=20to=2032?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per review on #34854: - Refactor the path-length guard to if-else style (no append-nil-and-break); the trie traversal lives in the else branch. - Tighten maxTrieNodePathLength from 33 to 32 bytes. The theoretical compact-encoded upper bound is 33 bytes (64 nibbles + 1 prefix byte), but in an MPT the value node is always embedded in its parent and is never addressed as a standalone node, so the hexary path length is at most 63 nibbles and the compact encoding never exceeds 32 bytes. - Update test comment to reflect the 32-byte real-world cap. --- eth/protocols/snap/handler.go | 12 +++++++----- eth/protocols/snap/handler_test.go | 10 ++++++---- eth/protocols/snap/handlers.go | 31 ++++++++++++++---------------- 3 files changed, 27 insertions(+), 26 deletions(-) diff --git a/eth/protocols/snap/handler.go b/eth/protocols/snap/handler.go index 30ebe9864d..34b2a27b7d 100644 --- a/eth/protocols/snap/handler.go +++ b/eth/protocols/snap/handler.go @@ -55,11 +55,13 @@ const ( // maxTrieNodePathLength is the longest a compact-encoded trie path can be // while still addressing a node in any Ethereum state trie. Trie keys are - // Keccak256 hashes (32 bytes = 64 nibbles), and the compact encoding adds - // at most one prefix byte, giving 32+1 = 33 bytes. Anything longer is - // structurally invalid and cannot match a node, so the handler skips it - // without performing a trie traversal. - maxTrieNodePathLength = 33 + // Keccak256 hashes (32 bytes = 64 nibbles). The theoretical compact-encoded + // upper bound is 33 bytes (64 nibbles + 1 prefix byte), but in an MPT the + // value node is always embedded in its parent and is never addressed as a + // standalone node, so the hexary path length is at most 63 nibbles and the + // compact encoding never exceeds 32 bytes. Anything longer is structurally + // invalid and the handler skips it without performing a trie traversal. + maxTrieNodePathLength = 32 ) // Handler is a callback to invoke from an outside runner after the boilerplate diff --git a/eth/protocols/snap/handler_test.go b/eth/protocols/snap/handler_test.go index f44dfd125a..89ea15253a 100644 --- a/eth/protocols/snap/handler_test.go +++ b/eth/protocols/snap/handler_test.go @@ -223,10 +223,12 @@ func TestServiceGetAccessListsQueryByteLimit(t *testing.T) { // TestServiceGetTrieNodesQueryPathLength verifies that ServiceGetTrieNodesQuery // rejects structurally invalid (over-length) compact-encoded paths without // allocating in compactToHex or walking the trie. Trie keys are Keccak256 -// hashes, so the longest valid compact-encoded path is 33 bytes; anything -// longer cannot match a node in any state trie. A peer sending such a path -// (up to the 10MB request size cap) would otherwise force the server to -// allocate 2*len(path)+1 nibbles per entry and start a doomed traversal. +// hashes; value nodes are always embedded in their parent and are never +// addressed as standalone nodes, so the longest compact-encoded path that can +// match a node is 32 bytes. Anything longer cannot match a node in any state +// trie. A peer sending such a path (up to the 10MB request size cap) would +// otherwise force the server to allocate 2*len(path)+1 nibbles per entry and +// start a doomed traversal. func TestServiceGetTrieNodesQueryPathLength(t *testing.T) { t.Parallel() diff --git a/eth/protocols/snap/handlers.go b/eth/protocols/snap/handlers.go index 90fc185027..3d6bc96e1e 100644 --- a/eth/protocols/snap/handlers.go +++ b/eth/protocols/snap/handlers.go @@ -476,15 +476,15 @@ func ServiceGetTrieNodesQuery(chain *core.BlockChain, req *GetTrieNodesPacket) ( // the request, matching the existing behavior for non-existent nodes. if len(accKey) > maxTrieNodePathLength { nodes = append(nodes, nil) - break + } else { + blob, resolved, err := accTrie.GetNode(accKey) + loads += resolved // always account database reads, even for failures + if err != nil { + break + } + nodes = append(nodes, blob) + bytes += uint64(len(blob)) } - blob, resolved, err := accTrie.GetNode(accKey) - loads += resolved // always account database reads, even for failures - if err != nil { - break - } - nodes = append(nodes, blob) - bytes += uint64(len(blob)) default: // Storage slots requested, open the storage trie and retrieve from there @@ -528,18 +528,15 @@ func ServiceGetTrieNodesQuery(chain *core.BlockChain, req *GetTrieNodesPacket) ( // the response stays positionally aligned with the request. if len(path) > maxTrieNodePathLength { nodes = append(nodes, nil) - if bytes > req.Bytes || loads > maxTrieNodeLookups || time.Since(start) > maxTrieNodeTimeSpent { + } else { + blob, resolved, err := stTrie.GetNode(path) + loads += resolved // always account database reads, even for failures + if err != nil { break } - continue + nodes = append(nodes, blob) + bytes += uint64(len(blob)) } - blob, resolved, err := stTrie.GetNode(path) - loads += resolved // always account database reads, even for failures - if err != nil { - break - } - nodes = append(nodes, blob) - bytes += uint64(len(blob)) // Sanity check limits to avoid DoS on the store trie loads if bytes > req.Bytes || loads > maxTrieNodeLookups || time.Since(start) > maxTrieNodeTimeSpent {