mirror of
https://github.com/ethereum/go-ethereum.git
synced 2026-05-24 08:49:29 +00:00
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.
This commit is contained in:
parent
01036bed83
commit
3c5cca0906
3 changed files with 93 additions and 0 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
Loading…
Reference in a new issue