#37352 [BC-Insight] Missing Liveness Check in `collectTableNodes()`
Submitted on Dec 2nd 2024 at 17:42:35 UTC by @CertiK for Attackathon | Ethereum Protocol
Report ID: #37352
Report Type: Blockchain/DLT
Report severity: Insight
Target: https://github.com/ledgerwatch/erigon
Impacts:
(Specifications) A bug in specifications with no direct impact on client implementations
Description
Brief/Intro
An issue that the node misses liveness check will be added to the node table with collectTableNodes()
was identified in the Ethereum client Erigon ( https://github.com/erigontech/erigon ).
Vulnerability Details
Affected Codebase: https://github.com/erigontech/erigon/tree/v2.61.0-beta1
The function collectTableNodes()
is intended to collect all the nodes for the FindNode result given a specified distance:
https://github.com/erigontech/erigon/blob/v2.60.10/accounts/abi/type.go#L158
func (t *UDPv5) collectTableNodes(rip net.IP, distances []uint, limit int) []*enode.Node {
nodes := make([]*enode.Node, 0, len(distances))
var processed = make(map[uint]struct{})
for _, dist := range distances {
// Reject duplicate / invalid distances.
_, seen := processed[dist]
if seen || dist > 256 {
continue
}
// Get the nodes.
var bn []*enode.Node
if dist == 0 {
bn = []*enode.Node{t.Self()}
} else if dist <= 256 {
t.tab.mutex.Lock()
bn = unwrapNodes(t.tab.bucketAtDistance(int(dist)).entries)
t.tab.mutex.Unlock()
}
processed[dist] = struct{}{}
// Apply some pre-checks to avoid sending invalid nodes.
for _, n := range bn {
// TODO livenessChecks > 1
if netutil.CheckRelayIP(rip, n.IP()) != nil {
continue
}
nodes = append(nodes, n)
if len(nodes) >= limit {
return nodes
}
}
}
return nodes
}
However, it misses the liveness chen when collecting the nodes into the table, which is also mentioned as the TODO: (https://github.com/erigontech/erigon/blob/v2.61.0-beta1/p2p/discover/v5_udp.go#L839 ):
In this case, the node that has not been checked liveness will also be included in the table.
It is worth noted a similar issue has been fixed in go-ethereum: https://github.com/ethereum/go-ethereum/pull/28686
Impact Details
.Nodes with no liveness check will be included in the node table.
References
https://github.com/erigontech/erigon/tree/v2.61.0-beta1
https://github.com/ethereum/go-ethereum/pull/28686
Proof of Concept
Proof of Concept
Here we provide the following test case to show that nodes with no liveness check will be collected.
The default livenessChecks in function wrapNode()
is zero, so there is no liveness check, which is used to mimic the nodes without liveness checks.
package discover
import (
"bytes"
"context"
"crypto/ecdsa"
"encoding/binary"
"errors"
"fmt"
"net"
"reflect"
"runtime"
"testing"
"time"
"github.com/ledgerwatch/erigon/turbo/testlog"
"github.com/ledgerwatch/log/v3"
"github.com/ledgerwatch/erigon/p2p/discover/v5wire"
"github.com/ledgerwatch/erigon/p2p/enode"
"github.com/ledgerwatch/erigon/p2p/enr"
"github.com/ledgerwatch/erigon/rlp"
)
func wrapNode(n *enode.Node) *node {
return &node{Node: *n}
}
func wrapNodes(ns []*enode.Node) []*node {
result := make([]*node, len(ns))
for i, n := range ns {
result[i] = wrapNode(n)
}
return result
}
////////////unit test//////////
func TestCollectTableNodes(t *testing.T) {
logger := log.New()
test := newUDPV5Test(t, logger)
t.Cleanup(test.close)
nodes253 := nodesAtDistance(test.table.self().ID(), 253, 5)
fillTable(test.table, wrapNodes(nodes253))
rip := new(net.IP)
distances := []uint{253}
limit := 256
nodes := test.udp.collectTableNodes(*rip, distances, limit)
fmt.Printf("The collected nodes are: %v\n", nodes)
fmt.Printf("Number of the collected nodes is: %d\n", len(nodes))
}
As the test result shows, all nodes without liveness check are also collected into the table:
=== RUN TestCollectTableNodes
The collected nodes are: [enr:-DyAgIJpZIRudWxsgmlwhAEAAgGIbnVsbGFkZHKgN_Nh_UDMNfBEwoyLNAHxti9lG9dB0WZUoYuTrh3Fikg enr:-DyAgIJpZIRudWxsgmlwhAIAAgKIbnVsbGFkZHKgN8qNUV_UmPwblUu29QgouNp_lYSdQkXJ7WowYEHpX_0 enr:-DyAgIJpZIRudWxsgmlwhAMAAgOIbnVsbGFkZHKgN6JjlG7eFq-HMOlKgjkhSh0dzSAeuFeUfcTQSwF6qrQ enr:-DyAgIJpZIRudWxsgmlwhAQAAgSIbnVsbGFkZHKgN3xzWvF5XH-wIVucytScYrWLImpix0rdD5oFVO2Ytrc enr:-DyAgIJpZIRudWxsgmlwhAUAAgWIbnVsbGFkZHKgN5LIK4BmeON56kQ4ocADZHbe9pKkBfoOLctBpfgBpWM]
Number of the collected nodes is: 5
--- PASS: TestCollectTableNodes (0.04s)
PASS
Was this helpful?