diff --git a/eth/protocols/snap/handler.go b/eth/protocols/snap/handler.go index 26545f2960..34b2a27b7d 100644 --- a/eth/protocols/snap/handler.go +++ b/eth/protocols/snap/handler.go @@ -52,6 +52,16 @@ 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). 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 b0522c20bb..89ea15253a 100644 --- a/eth/protocols/snap/handler_test.go +++ b/eth/protocols/snap/handler_test.go @@ -220,6 +220,73 @@ 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; 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() + + 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..3d6bc96e1e 100644 --- a/eth/protocols/snap/handlers.go +++ b/eth/protocols/snap/handlers.go @@ -469,13 +469,22 @@ func ServiceGetTrieNodesQuery(chain *core.BlockChain, req *GetTrieNodesPacket) ( if accKey == nil { return nodes, fmt.Errorf("%w: invalid account node request", errBadRequest) } - blob, resolved, err := accTrie.GetNode(accKey) - loads += resolved // always account database reads, even for failures - if err != nil { - break + // 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) + } 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)) } - nodes = append(nodes, blob) - bytes += uint64(len(blob)) default: // Storage slots requested, open the storage trie and retrieve from there @@ -513,13 +522,21 @@ func ServiceGetTrieNodesQuery(chain *core.BlockChain, req *GetTrieNodesPacket) ( if err != nil { return nil, fmt.Errorf("%w: invalid storage key: %v", errBadRequest, err) } - blob, resolved, err := stTrie.GetNode(path) - loads += resolved // always account database reads, even for failures - if err != nil { - break + // 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) + } else { + 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)) } - 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 {