During node discovery, skip those with invalid addresses

Sometimes a NEIGHBOURS response from remote peers will include nodes
with a loopback or private network address that is not reachable to us,
so we must skip those. We must also skip those on reserved networks or
with an unspecified address (0.0.0.0).

Closes: #821

Author: gsalgado

p2p/discovery.py

@@ -463,7 +463,7 @@ def recv_pong_v4(self, node: kademlia.Node, payload: Tuple[Any, ...], _: Hash32)
     def recv_neighbours_v4(self, node: kademlia.Node, payload: Tuple[Any, ...], _: Hash32) -> None:
         # The neighbours payload should have 2 elements: nodes, expiration
         nodes, _ = payload
-        neighbours = _extract_nodes_from_payload(nodes)
+        neighbours = _extract_nodes_from_payload(node.address, nodes, self.logger)
         self.logger.trace('<<< neighbours from %s: %s', node, neighbours)
         self.process_neighbours(node, neighbours)
 
@@ -493,7 +493,7 @@ def send_ping_v4(self, node: kademlia.Node) -> Hash32:
         self.send(node, message)
         # Return the msg hash, which is used as a token to identify pongs.
         token = message[:MAC_SIZE]
-        self.logger.trace('>>> ping (v4) %s (token == %s)', node, encode_hex(token))
+        self.logger.debug('>>> ping (v4) %s (token == %s)', node, encode_hex(token))
         # XXX: This hack is needed because there are lots of parity 1.10 nodes out there that send
         # the wrong token on pong msgs (https://github.com/paritytech/parity/issues/8038). We
         # should get rid of this once there are no longer too many parity 1.10 nodes out there.
@@ -694,7 +694,7 @@ def recv_topic_query(self, node: kademlia.Node, payload: Tuple[Any, ...],
     def recv_topic_nodes(self, node: kademlia.Node, payload: Tuple[Any, ...],
                          _: Hash32, _m: bytes) -> None:
         echo, raw_nodes = payload
-        nodes = _extract_nodes_from_payload(raw_nodes)
+        nodes = _extract_nodes_from_payload(node.address, raw_nodes, self.logger)
         self.logger.trace('<<< topic_nodes from %s: %s', node, nodes)
         try:
             callback = self.topic_nodes_callbacks.get_callback(node)
@@ -1083,11 +1083,16 @@ def __repr__(self) -> str:
 
 @to_list
 def _extract_nodes_from_payload(
-        payload: List[Tuple[str, str, str, str]]) -> Iterator[kademlia.Node]:
+        sender: kademlia.Address,
+        payload: List[Tuple[str, str, str, str]],
+        logger: TraceLogger) -> Iterator[kademlia.Node]:
     for item in payload:
         ip, udp_port, tcp_port, node_id = item
         address = kademlia.Address.from_endpoint(ip, udp_port, tcp_port)
-        yield kademlia.Node(keys.PublicKey(node_id), address)
+        if kademlia.check_relayed_addr(sender, address):
+            yield kademlia.Node(keys.PublicKey(node_id), address)
+        else:
+            logger.debug("Skipping invalid address %s relayed by %s", address, sender)
 
 
 def _get_max_neighbours_per_packet() -> int:

p2p/kademlia.py

@@ -57,6 +57,22 @@ def __init__(self, ip: str, udp_port: int, tcp_port: int = 0) -> None:
         self.tcp_port = tcp_port
         self._ip = ipaddress.ip_address(ip)
 
+    @property
+    def is_loopback(self) -> bool:
+        return self._ip.is_loopback
+
+    @property
+    def is_unspecified(self) -> bool:
+        return self._ip.is_unspecified
+
+    @property
+    def is_reserved(self) -> bool:
+        return self._ip.is_reserved
+
+    @property
+    def is_private(self) -> bool:
+        return self._ip.is_private
+
     @property
     def ip(self) -> str:
         return str(self._ip)
@@ -302,6 +318,23 @@ def neighbours(self, node_id: int, k: int = k_bucket_size) -> List[Node]:
         return sort_by_distance(nodes, node_id)[:k]
 
 
+def check_relayed_addr(sender: Address, addr: Address) -> bool:
+    """Check if an address relayed by the given sender is valid.
+
+    Reserved and unspecified addresses are always invalid.
+    Private addresses are valid if the sender is a private host.
+    Loopback addresses are valid if the sender is a loopback host.
+    All other addresses are valid.
+    """
+    if addr.is_unspecified or addr.is_reserved:
+        return False
+    if addr.is_private and not sender.is_private:
+        return False
+    if addr.is_loopback and not sender.is_loopback:
+        return False
+    return True
+
+
 def binary_get_bucket_for_node(buckets: List[KBucket], node: Node) -> KBucket:
     """Given a list of ordered buckets, returns the bucket for a given node."""
     bucket_ends = [bucket.end for bucket in buckets]

tests/p2p/test_discovery.py

@@ -84,7 +84,8 @@ def _test_find_node_neighbours(use_v5):
     for packet in [packet1, packet2]:
         node, payload = packet
         assert node == bob.this_node
-        neighbours.extend(discovery._extract_nodes_from_payload(payload[0]))
+        neighbours.extend(discovery._extract_nodes_from_payload(
+            node.address, payload[0], bob.logger))
     assert len(neighbours) == kademlia.k_bucket_size
 
 

tests/p2p/test_kademlia.py

@@ -207,6 +207,25 @@ def test_compute_shared_prefix_bits():
     assert kademlia._compute_shared_prefix_bits(nodes) == kademlia.k_id_size - 3
 
 
+def test_check_relayed_addr():
+    public_host = kademlia.Address('8.8.8.8', 80)
+    local_host = kademlia.Address('127.0.0.1', 80)
+    assert kademlia.check_relayed_addr(local_host, local_host)
+    assert not kademlia.check_relayed_addr(public_host, local_host)
+
+    private = kademlia.Address('192.168.1.1', 80)
+    assert kademlia.check_relayed_addr(private, private)
+    assert not kademlia.check_relayed_addr(public_host, private)
+
+    reserved = kademlia.Address('240.0.0.1', 80)
+    assert not kademlia.check_relayed_addr(local_host, reserved)
+    assert not kademlia.check_relayed_addr(public_host, reserved)
+
+    unspecified = kademlia.Address('0.0.0.0', 80)
+    assert not kademlia.check_relayed_addr(local_host, unspecified)
+    assert not kademlia.check_relayed_addr(public_host, unspecified)
+
+
 def random_pubkey():
     pk = int_to_big_endian(random.getrandbits(kademlia.k_pubkey_size))
     return keys.PublicKey(b'\x00' * (kademlia.k_pubkey_size // 8 - len(pk)) + pk)