From 3c5cca09064b76690f3c8dc77d0c31534cd3eb05 Mon Sep 17 00:00:00 2001 From: ozpool Date: Thu, 30 Apr 2026 15:44:48 +0530 Subject: [PATCH] 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 {