Merge pull request #961 from ssiloti/dht-sanitizing

remove DHT nodes which change their id

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)
 {
@@ -625,16 +648,12 @@ routing_table::add_node_status_t routing_table::add_node_impl(node_entry e)
 	if (m_router_nodes.find(e.ep()) != m_router_nodes.end())
 		return failed_to_add;
 
-	// don't add ourself
-	if (e.id == m_id) return failed_to_add;
-
 	// 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
+		// This exact IP already exists in the table. A node with the same IP and
-		// that the node changed IP. If pinged is true, and the port also
+		// port but a different ID may be a sign of a malicious node. To be
-		// matches then we assume it's in fact the same node, and just update
+		// conservative in this case the node is removed.
-		// the routing table
 		// 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
@@ -651,10 +670,13 @@ routing_table::add_node_status_t routing_table::add_node_impl(node_entry e)
 			if (m_settings.restrict_routing_ips)
 			{
 #ifndef TORRENT_DISABLE_LOGGING
-				char hex_id[41];
+				if (m_log)
-				aux::to_hex(reinterpret_cast<char const*>(&e.id[0]), 20, hex_id);
+				{
-				m_log->log(dht_logger::routing_table, "ignoring node (duplicate IP): %s %s"
+					char hex_id[41];
-					, hex_id, print_address(e.addr()).c_str());
+					aux::to_hex(reinterpret_cast<char const*>(&e.id[0]), 20, hex_id);
+					m_log->log(dht_logger::routing_table, "ignoring node (duplicate IP): %s %s"
+						, hex_id, print_address(e.addr()).c_str());
+				}
 #endif
 				return failed_to_add;
 			}
@@ -682,13 +704,40 @@ 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
+			// This is the same IP and port, but with a new node ID.
-			// a new node ID. remove the old entry and
+			// This may indicate a malicious node so remove the entry.
-			// replace it with this new ID
+#ifndef TORRENT_DISABLE_LOGGING
+			if (m_log)
+			{
+				char hex_id_new[41];
+				char hex_id_old[41];
+				aux::to_hex(e.id.data(), 20, hex_id_new);
+				aux::to_hex(existing->id.data(), 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);
+
+			// when we detect possible malicious activity in a bucket,
+			// schedule the other nodes in the bucket to be pinged soon
+			// to clean out any other malicious nodes
+			auto now = aux::time_now();
+			for (auto& node : existing_bucket->live_nodes)
+			{
+				if (now - node.last_queried > minutes(5))
+					node.last_queried = min_time();
+			}
+
+			return failed_to_add;
 		}
 	}
 
+	// don't add ourself
+	if (e.id == m_id) return failed_to_add;
+
 	table_t::iterator i = find_bucket(e.id);
 	bucket_t& b = i->live_nodes;
 	bucket_t& rb = i->replacements;
@@ -755,13 +804,16 @@ routing_table::add_node_status_t routing_table::add_node_impl(node_entry e)
 		// close to this one. We know that it's not the same, because
 		// it claims a different node-ID. Ignore this to avoid attacks
 #ifndef TORRENT_DISABLE_LOGGING
-		char hex_id1[41];
+		if (m_log)
-		aux::to_hex(e.id.data(), 20, hex_id1);
+		{
-		char hex_id2[41];
+			char hex_id1[41];
-		aux::to_hex(j->id.data(), 20, hex_id2);
+			aux::to_hex(e.id.data(), 20, hex_id1);
-		m_log->log(dht_logger::routing_table, "ignoring node: %s %s existing node: %s %s"
+			char hex_id2[41];
-			, hex_id1, print_address(e.addr()).c_str()
+			aux::to_hex(j->id.data(), 20, hex_id2);
-			, hex_id2, print_address(j->addr()).c_str());
+			m_log->log(dht_logger::routing_table, "ignoring node: %s %s existing node: %s %s"
+				, hex_id1, print_address(e.addr()).c_str()
+				, hex_id2, print_address(j->addr()).c_str());
+		}
 #endif
 		return failed_to_add;
 	}
@@ -1155,13 +1207,16 @@ void routing_table::node_failed(node_id const& nid, udp::endpoint const& ep)
 		j->timed_out();
 
 #ifndef TORRENT_DISABLE_LOGGING
-		char hex_id[41];
+		if (m_log)
-		aux::to_hex(nid.data(), 20, hex_id);
+		{
-		m_log->log(dht_logger::routing_table, "NODE FAILED id: %s ip: %s fails: %d pinged: %d up-time: %d"
+			char hex_id[41];
-			, hex_id, print_endpoint(j->ep()).c_str()
+			aux::to_hex(nid.data(), 20, hex_id);
-			, int(j->fail_count())
+			m_log->log(dht_logger::routing_table, "NODE FAILED id: %s ip: %s fails: %d pinged: %d up-time: %d"
-			, int(j->pinged())
+				, hex_id, print_endpoint(j->ep()).c_str()
-			, int(total_seconds(aux::time_now() - j->first_seen)));
+				, int(j->fail_count())
+				, int(j->pinged())
+				, int(total_seconds(aux::time_now() - j->first_seen)));
+		}
 #endif
 		return;
 	}
@@ -1176,13 +1231,16 @@ void routing_table::node_failed(node_id const& nid, udp::endpoint const& ep)
 		j->timed_out();
 
 #ifndef TORRENT_DISABLE_LOGGING
-		char hex_id[41];
+		if (m_log)
-		aux::to_hex(nid.data(), 20, hex_id);
+		{
-		m_log->log(dht_logger::routing_table, "NODE FAILED id: %s ip: %s fails: %d pinged: %d up-time: %d"
+			char hex_id[41];
-			, hex_id, print_endpoint(j->ep()).c_str()
+			aux::to_hex(nid.data(), 20, hex_id);
-			, int(j->fail_count())
+			m_log->log(dht_logger::routing_table, "NODE FAILED id: %s ip: %s fails: %d pinged: %d up-time: %d"
-			, int(j->pinged())
+				, hex_id, print_endpoint(j->ep()).c_str()
-			, int(total_seconds(aux::time_now() - j->first_seen)));
+				, int(j->fail_count())
+				, int(j->pinged())
+				, int(total_seconds(aux::time_now() - j->first_seen)));
+		}
 #endif
 
 		// if this node has failed too many times, or if this node
@@ -1198,16 +1256,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
+	fill_from_replacements(i);
-	// 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);
 }
 
 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)