From aa92ad4f42d772c53ce4a2d9c76cf760b4450031 Mon Sep 17 00:00:00 2001
From: Kim De Mey <7857583+kdeme@users.noreply.github.com>
Date: Mon, 2 Dec 2024 15:38:00 +0700
Subject: [PATCH] discv4: Fix assert on invalid RLP list in Neighbours message
 (#763)

The assert would occur when the rlp size of the a node in the
nodes rlp list is incorrectly set too high and then the next
`listElem` call for the next node will start from the
incorrect data. When that data is not a list the assert in
`listElem` will be triggered.

Fixed by adding a `listLen` call which checks if it is a list.
Added also more strictness by:
- Checking if that list is of len 4, which it must be
- raising immediatly on invalid IP length
- raising immediatly on invalid public key / node id

+ test cases
---
 eth/p2p/discovery.nim        | 14 +++++++-------
 tests/p2p/test_discovery.nim | 20 ++++++++++++++++++++
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/eth/p2p/discovery.nim b/eth/p2p/discovery.nim
index aa0e722..7725f32 100644
--- a/eth/p2p/discovery.nim
+++ b/eth/p2p/discovery.nim
@@ -220,6 +220,9 @@ proc recvNeighbours(
   var neighbours = newSeqOfCap[Node](16)
   for i in 0 ..< sz:
     let n = neighboursList.listElem(i)
+    if n.listLen() != 4:
+      raise newException(RlpError, "Invalid nodes list")
+
     let ipBlob = n.listElem(0).toBytes
     var ip: IpAddress
     case ipBlob.len
@@ -228,17 +231,14 @@ proc recvNeighbours(
     of 16:
       ip = IpAddress(family: IpAddressFamily.IPv6, address_v6: toArray(16, ipBlob))
     else:
-      error "Wrong ip address length!"
-      continue
+      raise newException(RlpError, "Invalid RLP byte string length for IP address")
 
     let udpPort = n.listElem(1).toInt(uint16).Port
     let tcpPort = n.listElem(2).toInt(uint16).Port
-    let pk = PublicKey.fromRaw(n.listElem(3).toBytes)
-    if pk.isErr:
-      warn "Could not parse public key"
-      continue
+    let pk = PublicKey.fromRaw(n.listElem(3).toBytes).valueOr:
+      raise newException(RlpError, "Invalid RLP byte string for node id")
 
-    neighbours.add(newNode(pk[], Address(ip: ip, udpPort: udpPort, tcpPort: tcpPort)))
+    neighbours.add(newNode(pk, Address(ip: ip, udpPort: udpPort, tcpPort: tcpPort)))
   d.kademlia.recvNeighbours(node, neighbours)
 
 proc recvFindNode(
diff --git a/tests/p2p/test_discovery.nim b/tests/p2p/test_discovery.nim
index c9c5460..14b89ce 100644
--- a/tests/p2p/test_discovery.nim
+++ b/tests/p2p/test_discovery.nim
@@ -13,6 +13,7 @@ import
   std/sequtils,
   chronos, stew/byteutils, nimcrypto/keccak, testutils/unittests,
   ../../eth/common/keys, ../../eth/p2p/[discovery, kademlia, enode],
+  ../../eth/rlp,
   ../stubloglevel
 
 proc localAddress(port: int): Address =
@@ -108,6 +109,21 @@ procSuite "Discovery Tests":
       # We currently do not raise on this, so can't really test it
       # "0x03f847b841AA0000000000000000000000000000000000000000000000000000000000000000a99a96bd988e1839272f93257bd9dfb2e558390e1f9bff28cdc8f04c9b5d06b1846fd3aed7",
     ]
+
+    let invalidRlpData = [
+      # valid version: "0x04f8a5f89ef84d847f000001827661827669b840ebefe173cab8832f6d6623f2a4d415959c1b80071e6af7d5986417d4bd07df19318b30d3d4fde75b025314ce22a523a714bef6c61838094e5d3640dccc6b9e34f84d847f000001827662827662b840ebefe173cab8832f6d6623f2a4d415959c1b80071e6af7d5986417d4bd07df19318b30d3d4fde75b025314ce22a523a714bef6c61838094e5d3640dccc6b9e348479e7f252",
+      # Invalid rlp item length (for the individual node list), causing a next listElem to read wrong data.
+      "0x04f8a5f89ef856847f000001827661827669b840ebefe173cab8832f6d6623f2a4d415959c1b80071e6af7d5986417d4bd07df19318b30d3d4fde75b025314ce22a523a714bef6c61838094e5d3640dccc6b9e34f84d847f000001827662827662b8403114ae2fe09b7a71d6693451c0d043df8315fb811e43c7b97ef2ff87409a2601b4425f92ffff80008417d66489cb1f8414d9dd4393ba16ebb455aa47345a222b8479e7f252",
+      # Invalid IP length in nodes
+      "0x04f8a4f89df84c837f0001827661827669b840ebefe173cab8832f6d6623f2a4d415959c1b80071e6af7d5986417d4bd07df19318b30d3d4fde75b025314ce22a523a714bef6c61838094e5d3640dccc6b9e34f84d847f000001827662827662b840ebefe173cab8832f6d6623f2a4d415959c1b80071e6af7d5986417d4bd07df19318b30d3d4fde75b025314ce22a523a714bef6c61838094e5d3640dccc6b9e348479e7f252",
+      # Invalid Public key in nodes
+      "0x04f8a5f89ef84d847f000001827661827669b840fbefe173cab8832f6d6623f2a4d415959c1b80071e6af7d5986417d4bd07df19318b30d3d4fde75b025314ce22a523a714bef6c61838094e5d3640dccc6b9e34f84d847f000001827662827662b840ebefe173cab8832f6d6623f2a4d415959c1b80071e6af7d5986417d4bd07df19318b30d3d4fde75b025314ce22a523a714bef6c61838094e5d3640dccc6b9e348479e7f252",
+      # Invalid Public key in nodes - too short
+      "0x04f8a4f89df84c847f000001827661827669b83fefe173cab8832f6d6623f2a4d415959c1b80071e6af7d5986417d4bd07df19318b30d3d4fde75b025314ce22a523a714bef6c61838094e5d3640dccc6b9e34f84d847f000001827662827662b840ebefe173cab8832f6d6623f2a4d415959c1b80071e6af7d5986417d4bd07df19318b30d3d4fde75b025314ce22a523a714bef6c61838094e5d3640dccc6b9e348479e7f252",
+      # nodes list len of 3 (removed port)
+      "0x04f8a2f89bf84a847f000001827661b840ebefe173cab8832f6d6623f2a4d415959c1b80071e6af7d5986417d4bd07df19318b30d3d4fde75b025314ce22a523a714bef6c61838094e5d3640dccc6b9e34f84d847f000001827662827662b840ebefe173cab8832f6d6623f2a4d415959c1b80071e6af7d5986417d4bd07df19318b30d3d4fde75b025314ce22a523a714bef6c61838094e5d3640dccc6b9e348479e7f252"
+    ]
+
     let
       address = localAddress(20302)
       nodeKey = PrivateKey.fromHex(
@@ -117,6 +133,10 @@ procSuite "Discovery Tests":
       expect DiscProtocolError:
         bootNode.receive(address, packData(hexToSeqByte(data), nodeKey))
 
+    for data in invalidRlpData:
+      expect RlpError:
+        bootNode.receive(address, packData(hexToSeqByte(data), nodeKey))
+
     # empty msg id and payload, doesn't raise, just fails and prints wrong
     # msg mac
     bootNode.receive(address, packData(@[], nodeKey))