remove nodes which change their id

This is based on a suggestion from the8472 to prevent malicious nodes from
polluting the routing table.

https://github.com/the8472/mldht/blob/sanitizing-docs/docs/sanitizing-algorithms.rst

include/libtorrent/kademlia/routing_table.hpp

@@ -293,6 +293,10 @@ private:
 	node_entry* find_node(udp::endpoint const& ep
 		, routing_table::table_t::iterator* bucket);
 
+	// if the bucket is not full, try to fill it with nodes from the
+	// replacement list
+	void fill_from_replacements(table_t::iterator bucket);
+
 	dht_settings const& m_settings;
 
 	// (k-bucket, replacement cache) pairs

src/kademlia/routing_table.cpp

@@ -547,6 +547,29 @@ node_entry* routing_table::find_node(udp::endpoint const& ep
 	return nullptr;
 }
 
+void routing_table::fill_from_replacements(table_t::iterator bucket)
+{
+	bucket_t& b = bucket->live_nodes;
+	bucket_t& rb = bucket->replacements;
+	int const bucket_size = bucket_limit(std::distance(m_buckets.begin(), bucket));
+
+	if (b.size() >= bucket_size) return;
+
+	// sort by RTT first, to find the node with the lowest
+	// RTT that is pinged
+	std::sort(rb.begin(), rb.end()
+		, [](node_entry const& lhs, node_entry const& rhs)
+			{ return lhs.rtt < rhs.rtt; });
+
+	while (b.size() < bucket_size && !rb.empty())
+	{
+		bucket_t::iterator j = std::find_if(rb.begin(), rb.end(), std::bind(&node_entry::pinged, _1));
+		if (j == rb.end()) j = rb.begin();
+		b.push_back(*j);
+		rb.erase(j);
+	}
+}
+
 void routing_table::remove_node(node_entry* n
 	, routing_table::table_t::iterator bucket)
 {
@@ -631,10 +654,9 @@ routing_table::add_node_status_t routing_table::add_node_impl(node_entry e)
 	// do we already have this IP in the table?
 	if (m_ips.count(e.addr()) > 0)
 	{
-		// this exact IP already exists in the table. It might be the case
-		// that the node changed IP. If pinged is true, and the port also
-		// matches then we assume it's in fact the same node, and just update
-		// the routing table
+		// This exact IP already exists in the table. A node with the same IP and
+		// port but a different ID may be a sign of a malicious node. To be
+		// conservative in this case the node is removed.
 		// pinged means that we have sent a message to the IP, port and received
 		// a response with a correct transaction ID, i.e. it is verified to not
 		// be the result of a poisoned routing table
@@ -682,10 +704,20 @@ routing_table::add_node_status_t routing_table::add_node_impl(node_entry e)
 		else
 		{
 			TORRENT_ASSERT(existing->id != e.id);
-			// this is the same IP and port, but with
-			// a new node ID. remove the old entry and
-			// replace it with this new ID
+			// This is the same IP and port, but with a new node ID.
+			// This may indicate a malicious node so remove the entry.
+#ifndef TORRENT_DISABLE_LOGGING
+			char hex_id_new[41];
+			char hex_id_old[41];
+			aux::to_hex(reinterpret_cast<char const*>(&e.id[0]), 20, hex_id_new);
+			aux::to_hex(reinterpret_cast<char const*>(&existing->id[0]), 20, hex_id_old);
+			m_log->log(dht_logger::routing_table, "evicting node (changed ID): old: %s new: %s %s"
+				, hex_id_old, hex_id_new, print_address(e.addr()).c_str());
+#endif
+
 			remove_node(existing, existing_bucket);
+			fill_from_replacements(existing_bucket);
+			return failed_to_add;
 		}
 	}
 
@@ -1198,16 +1230,7 @@ void routing_table::node_failed(node_id const& nid, udp::endpoint const& ep)
 	m_ips.erase(j->addr());
 	b.erase(j);
 
-	// sort by RTT first, to find the node with the lowest
-	// RTT that is pinged
-	std::sort(rb.begin(), rb.end()
-		, [](node_entry const& lhs, node_entry const& rhs)
-		{ return lhs.rtt < rhs.rtt; });
-
-	j = std::find_if(rb.begin(), rb.end(), std::bind(&node_entry::pinged, _1));
-	if (j == rb.end()) j = rb.begin();
-	b.push_back(*j);
-	rb.erase(j);
+	fill_from_replacements(i);
 }
 
 void routing_table::add_router_node(udp::endpoint router)

test/test_dht.cpp

@@ -1392,19 +1392,6 @@ void test_routing_table(address(&rand_addr)())
 		TEST_EQUAL(nodes[0].timeout_count, 0);
 	}
 
-	// test adding the same IP:port again with a new node ID (should replace the old one)
-	add_and_replace(tmp, diff);
-	table.node_seen(tmp, udp::endpoint(node_addr, 4), 10);
-	table.find_node(id, nodes, 0, 10);
-	TEST_EQUAL(table.bucket_size(0), 1);
-	TEST_EQUAL(nodes.size(), 1);
-	if (!nodes.empty())
-	{
-		TEST_EQUAL(nodes[0].id, tmp);
-		TEST_EQUAL(nodes[0].addr(), node_addr);
-		TEST_EQUAL(nodes[0].port(), 4);
-	}
-
 	// test adding the same node ID again with a different IP (should be ignored)
 	table.node_seen(tmp, udp::endpoint(node_addr, 5), 10);
 	table.find_node(id, nodes, 0, 10);
@@ -1429,6 +1416,13 @@ void test_routing_table(address(&rand_addr)())
 		TEST_EQUAL(nodes[0].port(), 4);
 	}
 
+	// test adding the same IP:port again with a new node ID (should remove the node)
+	add_and_replace(tmp, diff);
+	table.node_seen(tmp, udp::endpoint(node_addr, 4), 10);
+	table.find_node(id, nodes, 0, 10);
+	TEST_EQUAL(table.bucket_size(0), 0);
+	TEST_EQUAL(nodes.size(), 0);
+
 	s.restrict_routing_ips = false;
 
 	init_rand_address();
@@ -3006,14 +3000,6 @@ TORRENT_TEST(dht_verify_node_address)
 	TEST_EQUAL(std::get<0>(table.size()), 1);
 	TEST_EQUAL(nodes.size(), 1);
 
-	// incorrect data, wrong id
-	table.node_seen(to_hash("0123456789abcdef01232456789abcdef0123456")
-		, udp::endpoint(addr("4.4.4.4"), 4), 10);
-	table.find_node(id, nodes, 0, 10);
-
-	TEST_EQUAL(std::get<0>(table.size()), 1);
-	TEST_EQUAL(nodes.size(), 1);
-
 	// incorrect data, wrong IP
 	table.node_seen(tmp
 		, udp::endpoint(addr("4.4.4.6"), 4), 10);
@@ -3021,6 +3007,14 @@ TORRENT_TEST(dht_verify_node_address)
 
 	TEST_EQUAL(std::get<0>(table.size()), 1);
 	TEST_EQUAL(nodes.size(), 1);
+
+	// incorrect data, wrong id, should cause node to be removed
+	table.node_seen(to_hash("0123456789abcdef01232456789abcdef0123456")
+					, udp::endpoint(addr("4.4.4.4"), 4), 10);
+	table.find_node(id, nodes, 0, 10);
+
+	TEST_EQUAL(std::get<0>(table.size()), 0);
+	TEST_EQUAL(nodes.size(), 0);
 }
 
 TORRENT_TEST(generate_prefix_mask)