Modify nodes verification (#398)

* Modify nodes verification

* Move nodes verification to separate module

By moving verification to separate module it can be re-used
in different contexts not only in discoveryv5.

eth/p2p/discoveryv5/nodes_verification.nim

@@ -0,0 +1,85 @@
+{.push raises: [Defect].}
+
+import
+  std/[sets, options],
+  stew/results, stew/shims/net, chronicles, chronos, 
+  "."/[node, enr, routing_table]
+
+logScope:
+  topics = "nodes-verification"
+
+proc validIp(sender, address: IpAddress): bool =
+  let
+    s = initTAddress(sender, Port(0))
+    a = initTAddress(address, Port(0))
+  if a.isAnyLocal():
+    return false
+  if a.isMulticast():
+    return false
+  if a.isLoopback() and not s.isLoopback():
+    return false
+  if a.isSiteLocal() and not s.isSiteLocal():
+    return false
+  # TODO: Also check for special reserved ip addresses:
+  # https://www.iana.org/assignments/iana-ipv4-special-registry/iana-ipv4-special-registry.xhtml
+  # https://www.iana.org/assignments/iana-ipv6-special-registry/iana-ipv6-special-registry.xhtml
+  return true
+
+proc verifyNodesRecords(enrs: openarray[Record], fromNode: Node, nodesLimit: int,
+    distances: Option[seq[uint16]]): seq[Node] =
+  ## Verify and convert ENRs to a sequence of nodes. Only ENRs that pass
+  ## verification will be added. ENRs are verified for duplicates, invalid
+  ## addresses and invalid distances if those are specified.
+  var seen: HashSet[Node]
+  var count = 0
+  for r in enrs:
+    # Check and allow for processing of maximum `findNodeResultLimit` ENRs
+    # returned. This limitation is required so no huge lists of invalid ENRs
+    # are processed for no reason, and for not overwhelming a routing table
+    # with nodes from a malicious actor.
+    # The discovery v5 specification specifies no limit on the amount of ENRs
+    # that can be returned, but clients usually stick with the bucket size limit
+    # as in original Kademlia. Because of this it is chosen not to fail
+    # immediatly, but still process maximum `findNodeResultLimit`.
+    if count >= nodesLimit:
+      debug "Too many ENRs", enrs = enrs.len(),
+        limit = nodesLimit, sender = fromNode.record.toURI
+      break
+
+    count.inc()
+
+    let node = newNode(r)
+    if node.isOk():
+      let n = node.get()
+      # Check for duplicates in the nodes reply. Duplicates are checked based
+      # on node id.
+      if n in seen:
+        trace "Duplicate node ids",
+          record = n.record.toURI, id = n.id, sender = fromNode.record.toURI
+        continue
+      # Check if the node has an address and if the address is public or from
+      # the same local network or lo network as the sender. The latter allows
+      # for local testing.
+      if not n.address.isSome() or not
+          validIp(fromNode.address.get().ip, n.address.get().ip):
+        trace "Invalid ip-address",
+          record = n.record.toURI, node = n, sender = fromNode.record.toURI
+        continue
+      # Check if returned node has one of the requested distances.
+      if distances.isSome():
+        if (not distances.get().contains(logDist(n.id, fromNode.id))):
+          debug "Incorrect distance",
+            record = n.record.toURI, sender = fromNode.record.toURI
+          continue
+
+      # No check on UDP port and thus any port is allowed, also the so called
+      # "well-known" ports.
+
+      seen.incl(n)
+      result.add(n)
+
+proc verifyNodesRecords*(enrs: openarray[Record], fromNode: Node, nodesLimit: int): seq[Node] =
+  verifyNodesRecords(enrs, fromNode, nodesLimit, none[seq[uint16]]())
+
+proc verifyNodesRecords*(enrs: openarray[Record], fromNode: Node, nodesLimit: int, distances: seq[uint16]): seq[Node] =
+  verifyNodesRecords(enrs, fromNode, nodesLimit, some[seq[uint16]](distances))

eth/p2p/discoveryv5/protocol.nim

@@ -78,7 +78,7 @@ import
   stew/shims/net as stewNet, json_serialization/std/net,
   stew/endians2, chronicles, chronos, stint, bearssl, metrics,
   ".."/../[rlp, keys, async_utils],
-  "."/[messages, encoding, node, routing_table, enr, random2, sessions, ip_vote]
+  "."/[messages, encoding, node, routing_table, enr, random2, sessions, ip_vote, nodes_verification]
 
 import nimcrypto except toHex
 
@@ -449,23 +449,6 @@ proc processClient(transp: DatagramTransport, raddr: TransportAddress):
 
   proto.receive(a, buf)
 
-proc validIp(sender, address: IpAddress): bool =
-  let
-    s = initTAddress(sender, Port(0))
-    a = initTAddress(address, Port(0))
-  if a.isAnyLocal():
-    return false
-  if a.isMulticast():
-    return false
-  if a.isLoopback() and not s.isLoopback():
-    return false
-  if a.isSiteLocal() and not s.isSiteLocal():
-    return false
-  # TODO: Also check for special reserved ip addresses:
-  # https://www.iana.org/assignments/iana-ipv4-special-registry/iana-ipv4-special-registry.xhtml
-  # https://www.iana.org/assignments/iana-ipv6-special-registry/iana-ipv6-special-registry.xhtml
-  return true
-
 proc replaceNode(d: Protocol, n: Node) =
   if n.record notin d.bootstrapRecords:
     d.routingTable.replaceNode(n)
@@ -496,58 +479,6 @@ proc waitMessage(d: Protocol, fromNode: Node, reqId: RequestId):
       res.complete(none(Message))
   d.awaitedMessages[key] = result
 
-proc verifyNodesRecords*(enrs: openarray[Record], fromNode: Node,
-    distances: varargs[uint16]): seq[Node] =
-  ## Verify and convert ENRs to a sequence of nodes. Only ENRs that pass
-  ## verification will be added. ENRs are verified for duplicates, invalid
-  ## addresses and invalid distances.
-  var seen: HashSet[Node]
-  var count = 0
-  for r in enrs:
-    # Check and allow for processing of maximum `findNodeResultLimit` ENRs
-    # returned. This limitation is required so no huge lists of invalid ENRs
-    # are processed for no reason, and for not overwhelming a routing table
-    # with nodes from a malicious actor.
-    # The discovery v5 specification specifies no limit on the amount of ENRs
-    # that can be returned, but clients usually stick with the bucket size limit
-    # as in original Kademlia. Because of this it is chosen not to fail
-    # immediatly, but still process maximum `findNodeResultLimit`.
-    if count >= findNodeResultLimit:
-      debug "Response on findnode returned too many ENRs", enrs = enrs.len(),
-        limit = findNodeResultLimit, sender = fromNode.record.toURI
-      break
-
-    count.inc()
-
-    let node = newNode(r)
-    if node.isOk():
-      let n = node.get()
-      # Check for duplicates in the nodes reply. Duplicates are checked based
-      # on node id.
-      if n in seen:
-        trace "Nodes reply contained records with duplicate node ids",
-          record = n.record.toURI, id = n.id, sender = fromNode.record.toURI
-        continue
-      # Check if the node has an address and if the address is public or from
-      # the same local network or lo network as the sender. The latter allows
-      # for local testing.
-      if not n.address.isSome() or not
-          validIp(fromNode.address.get().ip, n.address.get().ip):
-        trace "Nodes reply contained record with invalid ip-address",
-          record = n.record.toURI, node = n, sender = fromNode.record.toURI
-        continue
-      # Check if returned node has one of the requested distances.
-      if not distances.contains(logDist(n.id, fromNode.id)):
-        debug "Nodes reply contained record with incorrect distance",
-          record = n.record.toURI, sender = fromNode.record.toURI
-        continue
-
-      # No check on UDP port and thus any port is allowed, also the so called
-      # "well-known" ports.
-
-      seen.incl(n)
-      result.add(n)
-
 proc waitNodes(d: Protocol, fromNode: Node, reqId: RequestId):
     Future[DiscResult[seq[Record]]] {.async.} =
   ## Wait for one or more nodes replies.
@@ -625,7 +556,7 @@ proc findNode*(d: Protocol, toNode: Node, distances: seq[uint16]):
   let nodes = await d.waitNodes(toNode, reqId)
 
   if nodes.isOk:
-    let res = verifyNodesRecords(nodes.get(), toNode, distances)
+    let res = verifyNodesRecords(nodes.get(), toNode, findNodeResultLimit, distances)
     d.routingTable.setJustSeen(toNode)
     return ok(res)
   else:

tests/p2p/test_discoveryv5.nim

@@ -5,7 +5,7 @@ import
   chronos, chronicles, stint, testutils/unittests, stew/shims/net,
   stew/byteutils, bearssl,
   ../../eth/keys,
-  ../../eth/p2p/discoveryv5/[enr, node, routing_table, encoding, sessions, messages],
+  ../../eth/p2p/discoveryv5/[enr, node, routing_table, encoding, sessions, messages, nodes_verification],
   ../../eth/p2p/discoveryv5/protocol as discv5_protocol,
   ./discv5_test_helper
 
@@ -471,7 +471,8 @@ procSuite "Discovery v5 Tests":
         some(port), some(port))[]
       fromNode = newNode(fromNoderecord)[]
       pk = PrivateKey.random(rng[])
-      targetDistance = logDist(fromNode.id, pk.toPublicKey().toNodeId())
+      targetDistance = @[logDist(fromNode.id, pk.toPublicKey().toNodeId())]
+      limit = 16
 
     block: # Duplicates
       let
@@ -481,7 +482,7 @@ procSuite "Discovery v5 Tests":
 
       # Exact duplicates
       var records = @[record, record]
-      var nodes = verifyNodesRecords(records, fromNode, targetDistance)
+      var nodes = verifyNodesRecords(records, fromNode, limit, targetDistance)
       check nodes.len == 1
 
       # Node id duplicates
@@ -489,7 +490,7 @@ procSuite "Discovery v5 Tests":
         1, pk, some(ValidIpAddress.init("212.13.14.15")),
         some(port), some(port))[]
       records.add(recordSameId)
-      nodes = verifyNodesRecords(records, fromNode, targetDistance)
+      nodes = verifyNodesRecords(records, fromNode, limit, targetDistance)
       check nodes.len == 1
 
     block: # No address
@@ -497,7 +498,7 @@ procSuite "Discovery v5 Tests":
         recordNoAddress = enr.Record.init(
           1, pk, none(ValidIpAddress), some(port), some(port))[]
         records = [recordNoAddress]
-        test = verifyNodesRecords(records, fromNode, targetDistance)
+        test = verifyNodesRecords(records, fromNode, limit, targetDistance)
       check test.len == 0
 
     block: # Invalid address - site local
@@ -506,7 +507,7 @@ procSuite "Discovery v5 Tests":
           1, pk, some(ValidIpAddress.init("10.1.2.3")),
           some(port), some(port))[]
         records = [recordInvalidAddress]
-        test = verifyNodesRecords(records, fromNode, targetDistance)
+        test = verifyNodesRecords(records, fromNode, limit, targetDistance)
       check test.len == 0
 
     block: # Invalid address - loopback
@@ -515,7 +516,7 @@ procSuite "Discovery v5 Tests":
           1, pk, some(ValidIpAddress.init("127.0.0.1")),
           some(port), some(port))[]
         records = [recordInvalidAddress]
-        test = verifyNodesRecords(records, fromNode, targetDistance)
+        test = verifyNodesRecords(records, fromNode, limit, targetDistance)
       check test.len == 0
 
     block: # Invalid distance
@@ -524,9 +525,18 @@ procSuite "Discovery v5 Tests":
           1, pk, some(ValidIpAddress.init("12.13.14.15")),
           some(port), some(port))[]
         records = [recordInvalidDistance]
-        test = verifyNodesRecords(records, fromNode, 0'u16)
+        test = verifyNodesRecords(records, fromNode, limit, @[0'u16])
       check test.len == 0
 
+    block: # Invalid distance but distance validation is disabled
+      let
+        recordInvalidDistance = enr.Record.init(
+          1, pk, some(ValidIpAddress.init("12.13.14.15")),
+          some(port), some(port))[]
+        records = [recordInvalidDistance]
+        test = verifyNodesRecords(records, fromNode, limit)
+      check test.len == 1
+
   test "Calculate lookup distances":
     # Log distance between zeros is zero
     let dist = lookupDistances(u256(0), u256(0))