Improvements on dropping of challenges and handling of too large distance (#296)

- drop handshake challenge on invalid handshake
- send empty nodes reponse when distance is > 256
- misc

eth/p2p/discoveryv5/encoding.nim

@@ -72,7 +72,7 @@ proc deriveKeys(n1, n2: NodeID, priv: PrivateKey, pub: PublicKey,
     idNonce: openarray[byte]): HandshakeSecrets =
   let eph = ecdhRawFull(priv, pub)
 
-  var info = newSeqOfCap[byte](idNoncePrefix.len + 32 * 2)
+  var info = newSeqOfCap[byte](keyAgreementPrefix.len + 32 * 2)
   for i, c in keyAgreementPrefix: info.add(byte(c))
   info.add(n1.toByteArrayBE())
   info.add(n2.toByteArrayBE())
@@ -318,9 +318,24 @@ proc decodePacket*(c: var Codec,
     authTag = auth.auth
 
     let key = HandShakeKey(nodeId: fromId, address: $fromAddr)
-    let challenge = c.handshakes.getOrDefault(key)
-    if challenge.isNil:
-      trace "Decoding failed (no challenge)"
+    var challenge: Whoareyou
+    # Note: We remove (pop) the stored handshake data here on failure on purpose
+    # as mitigation for a DoS attack where an invalid handshake is send
+    # repeatedly, which causes the signature verification to be done until
+    # handshake timeout, in case the stored data is not removed at first fail.
+    # See also more info here: https://github.com/prysmaticlabs/prysm/issues/7346
+    #
+    # It should be noted though that this means that now it might be possible to
+    # drop a handshake on purpose by a malicious party. But only if that
+    # attacker manages to spoof the IP-address of a peer A, and manages to
+    # listen to traffic between peer A and B that are starting a handshake, and
+    # next manages to be faster in sending out the (invalid) handshake. And this
+    # for each attempt in order to deny the peers setting up a session.
+    # However, this looks like a much more difficult scenario to pull off than
+    # the more convenient DoS attack. The DoS attack might have less heavy
+    # consequences though.
+    if not c.handshakes.pop(key, challenge):
+      debug "Decoding failed (no previous stored handshake challenge)"
       return err(HandshakeError)
 
     if auth.idNonce != challenge.idNonce:

eth/p2p/discoveryv5/protocol.nim

@@ -328,9 +328,12 @@ proc handleFindNode(d: Protocol, fromId: NodeId, fromAddr: Address,
   if fn.distance == 0:
     d.sendNodes(fromId, fromAddr, reqId, [d.localNode])
   else:
-    let distance = min(fn.distance, 256)
+    if fn.distance <= 256:
       d.sendNodes(fromId, fromAddr, reqId,
-      d.routingTable.neighboursAtDistance(distance, seenOnly = true))
+        d.routingTable.neighboursAtDistance(fn.distance, seenOnly = true))
+    else:
+      # The polite node we are, still respond with empty nodes.
+      d.sendNodes(fromId, fromAddr, reqId, [])
 
 proc receive*(d: Protocol, a: Address, packet: openArray[byte]) {.gcsafe,
   raises: [
@@ -533,8 +536,8 @@ proc verifyNodesRecords*(enrs: openarray[Record], fromNode: Node,
         trace "Nodes reply contained record with invalid ip-address",
           record = n.record.toURI, sender = fromNode.record.toURI, node = $n
         continue
-      # Check if returned node has the requested distance.
-      if logDist(n.id, fromNode.id) != min(distance, 256):
+      # Check if returned node has exactly the requested distance.
+      if logDist(n.id, fromNode.id) != distance:
         warn "Nodes reply contained record with incorrect distance",
           record = n.record.toURI, sender = fromNode.record.toURI
         continue

tests/p2p/test_discoveryv5.nim

@@ -212,9 +212,16 @@ procSuite "Discovery v5 Tests":
     for n in nodes:
       check discovered[].contains(n)
 
-    # Too high logarithmic distance, caps at 256
+    # Too high logarithmic distance, should return no nodes.
     discovered =
       await discv5_protocol.findNode(testNode, mainNode.localNode, 4294967295'u32)
+    check:
+      discovered.isOk
+      discovered[].len == 0
+
+    # Logarithmic distance of 256 should only return the testNode
+    discovered =
+      await discv5_protocol.findNode(testNode, mainNode.localNode, 256)
     check:
       discovered.isOk
       discovered[].len == 1