From fef1b947f342772353bff6656b18604823b92ef1 Mon Sep 17 00:00:00 2001 From: Arvid Norberg Date: Sat, 18 Aug 2018 10:36:38 +0200 Subject: [PATCH] fix issue in self-connection detection introduced with the change to generate unique peer-ids for each connection. Now, the torrent keeps track of all of our peer-ids generated for outgoing (bittorrent) connections, and we check them against incoming peers' peer-ids --- include/libtorrent/bt_peer_connection.hpp | 4 +- include/libtorrent/peer_connection.hpp | 1 + .../libtorrent/peer_connection_interface.hpp | 1 + include/libtorrent/torrent.hpp | 9 +++ include/libtorrent/vector_utils.hpp | 4 +- include/libtorrent/web_connection_base.hpp | 2 + simulation/libsimulator | 2 +- simulation/test_swarm.cpp | 55 +++++++++++++++ src/bt_peer_connection.cpp | 8 ++- src/peer_connection.cpp | 10 ++- src/session_impl.cpp | 24 ++++--- src/torrent.cpp | 69 ++++++++++++------- test/test_peer_list.cpp | 3 + 13 files changed, 148 insertions(+), 44 deletions(-) diff --git a/include/libtorrent/bt_peer_connection.hpp b/include/libtorrent/bt_peer_connection.hpp index 5ae6185b4..8f9df204f 100644 --- a/include/libtorrent/bt_peer_connection.hpp +++ b/include/libtorrent/bt_peer_connection.hpp @@ -108,6 +108,8 @@ namespace libtorrent { ~bt_peer_connection() override; + peer_id our_pid() const override { return m_our_peer_id; } + #if !defined TORRENT_DISABLE_ENCRYPTION bool supports_encryption() const { return m_encrypted; } @@ -414,7 +416,7 @@ namespace libtorrent { std::string m_client_version; // the peer ID we advertise for ourself - peer_id m_our_peer_id; + peer_id const m_our_peer_id; // this is a queue of ranges that describes // where in the send buffer actual payload diff --git a/include/libtorrent/peer_connection.hpp b/include/libtorrent/peer_connection.hpp index 3499685e8..d2f5b6fad 100644 --- a/include/libtorrent/peer_connection.hpp +++ b/include/libtorrent/peer_connection.hpp @@ -146,6 +146,7 @@ namespace aux { std::shared_ptr s; tcp::endpoint endp; torrent_peer* peerinfo; + peer_id our_peer_id; }; struct TORRENT_EXTRA_EXPORT peer_connection_hot_members diff --git a/include/libtorrent/peer_connection_interface.hpp b/include/libtorrent/peer_connection_interface.hpp index dda4f049b..20958d310 100644 --- a/include/libtorrent/peer_connection_interface.hpp +++ b/include/libtorrent/peer_connection_interface.hpp @@ -52,6 +52,7 @@ namespace libtorrent { virtual void disconnect(error_code const& ec , operation_t op, int error = 0) = 0; virtual peer_id const& pid() const = 0; + virtual peer_id our_pid() const = 0; virtual void set_holepunch_mode() = 0; virtual torrent_peer* peer_info_struct() const = 0; virtual void set_peer_info(torrent_peer* pi) = 0; diff --git a/include/libtorrent/torrent.hpp b/include/libtorrent/torrent.hpp index 2ca975db1..012f43c79 100644 --- a/include/libtorrent/torrent.hpp +++ b/include/libtorrent/torrent.hpp @@ -387,6 +387,10 @@ namespace libtorrent { bt_peer_connection* find_peer(tcp::endpoint const& ep) const; peer_connection* find_peer(peer_id const& pid); + // checks to see if this peer id is used in one of our own outgoing + // connections. + bool is_self_connection(peer_id const& pid) const; + void on_resume_data_checked(status_t status, storage_error const& error); void on_force_recheck(status_t status, storage_error const& error); void on_piece_hashed(piece_index_t piece, sha1_hash const& piece_hash @@ -1433,6 +1437,11 @@ namespace libtorrent { aux::handler_storage<64> m_deferred_handler_storage; #endif + // these are the peer IDs we've used for our outgoing peer connections for + // this torrent. If we get an incoming peer claiming to have one of these, + // it's a connection to ourself, and we should reject it. + std::set m_outgoing_pids; + // for torrents who have a bandwidth limit, this is != 0 // and refers to a peer_class in the session. peer_class_t m_peer_class{0}; diff --git a/include/libtorrent/vector_utils.hpp b/include/libtorrent/vector_utils.hpp index c1bc5f353..3d392ce68 100644 --- a/include/libtorrent/vector_utils.hpp +++ b/include/libtorrent/vector_utils.hpp @@ -48,8 +48,8 @@ namespace libtorrent { return i; } - template - void sorted_insert(std::vector& container, T v) + template + void sorted_insert(std::vector& container, U v) { auto i = std::lower_bound(container.begin(), container.end(), v); container.insert(i, v); diff --git a/include/libtorrent/web_connection_base.hpp b/include/libtorrent/web_connection_base.hpp index 7d91c404d..6ceb28288 100644 --- a/include/libtorrent/web_connection_base.hpp +++ b/include/libtorrent/web_connection_base.hpp @@ -75,6 +75,8 @@ namespace libtorrent { bool in_handshake() const override; + peer_id our_pid() const override { return peer_id(); } + // the following functions appends messages // to the send buffer void write_choke() override {} diff --git a/simulation/libsimulator b/simulation/libsimulator index ffaccc122..e4fb5e171 160000 --- a/simulation/libsimulator +++ b/simulation/libsimulator @@ -1 +1 @@ -Subproject commit ffaccc122691d10131a95bbe085ab325f57a602b +Subproject commit e4fb5e171a6ed885e7de3192153ba77835d23dee diff --git a/simulation/test_swarm.cpp b/simulation/test_swarm.cpp index 7f2f85518..e1fd547a9 100644 --- a/simulation/test_swarm.cpp +++ b/simulation/test_swarm.cpp @@ -44,6 +44,7 @@ POSSIBILITY OF SUCH DAMAGE. #include "setup_transfer.hpp" // for ep() #include "fake_peer.hpp" +#include "simulator/nat.hpp" #include "simulator/queue.hpp" #include "utils.hpp" @@ -462,6 +463,60 @@ TORRENT_TEST(dead_peers) TEST_EQUAL(num_connect_timeout, 3); } +// the address 50.0.0.1 sits behind a NAT. All of its outgoing connections have +// their source address rewritten to 51.51.51.51 +struct nat_config : sim::default_config +{ + nat_config() : m_nat_hop(std::make_shared(addr("51.51.51.51"))) {} + + sim::route outgoing_route(lt::address ip) override + { + // This is extremely simplistic. It will simply alter the percieved source + // IP of the connecting client. + sim::route r; + if (ip == addr("50.0.0.1")) r.append(m_nat_hop); + return r; + } + std::shared_ptr m_nat_hop; +}; + +TORRENT_TEST(self_connect) +{ + int num_self_connection_disconnects = 0; + + nat_config network_cfg; + sim::simulation sim{network_cfg}; + + setup_swarm(1, swarm_test::download, sim + // add session + , [](lt::settings_pack& p) { + p.set_bool(settings_pack::enable_incoming_utp, false); + p.set_bool(settings_pack::enable_outgoing_utp, false); + } + // add torrent + , [](lt::add_torrent_params& params) { + // this is our own address and listen port, just to make sure we get + // ourself as a peer (which normally happens one way or another in the + // wild) + params.peers.assign({ep("50.0.0.1", 6881)}); + } + // on alert + , [&](lt::alert const* a, lt::session&) { + auto* e = alert_cast(a); + if (e + && e->op == operation_t::bittorrent + && e->error == error_code(errors::self_connection)) + { + ++num_self_connection_disconnects; + } + } + // terminate + , [](int t, lt::session&) -> bool + { return t > 100; }); + + TEST_EQUAL(num_self_connection_disconnects, 1); +} + TORRENT_TEST(delete_files) { std::string save_path; diff --git a/src/bt_peer_connection.cpp b/src/bt_peer_connection.cpp index 2c41452d5..266b1288c 100644 --- a/src/bt_peer_connection.cpp +++ b/src/bt_peer_connection.cpp @@ -155,7 +155,7 @@ namespace { , m_rc4_encrypted(false) , m_recv_buffer(peer_connection::m_recv_buffer) #endif - , m_our_peer_id(aux::generate_peer_id(*pack.sett)) + , m_our_peer_id(pack.our_peer_id) { #ifndef TORRENT_DISABLE_LOGGING peer_log(peer_log_alert::info, "CONSTRUCT", "bt_peer_connection"); @@ -3180,6 +3180,12 @@ namespace { if (max_out_request_queue() > 50) max_out_request_queue(50); } + if (t->is_self_connection(pid)) + { + disconnect(errors::self_connection, operation_t::bittorrent); + return; + } + #ifndef TORRENT_DISABLE_EXTENSIONS for (auto i = m_extensions.begin() , end(m_extensions.end()); i != end;) diff --git a/src/peer_connection.cpp b/src/peer_connection.cpp index 9c574311b..530abefe9 100644 --- a/src/peer_connection.cpp +++ b/src/peer_connection.cpp @@ -4303,6 +4303,13 @@ namespace libtorrent { } std::shared_ptr t = m_torrent.lock(); + + if (ec == errors::self_connection) + { + // don't try to connect to ourself again + if (m_peer_info && t) t->ban_peer(m_peer_info); + } + if (m_connecting) { m_counters.inc_stats_counter(counters::num_peers_half_open, -1); @@ -6108,9 +6115,6 @@ namespace libtorrent { if (m_remote == m_socket->local_endpoint(ec)) { - // if the remote endpoint is the same as the local endpoint, we're connected - // to ourselves - if (m_peer_info && t) t->ban_peer(m_peer_info); disconnect(errors::self_connection, operation_t::bittorrent, 1); return; } diff --git a/src/session_impl.cpp b/src/session_impl.cpp index aa0dead96..51b710238 100644 --- a/src/session_impl.cpp +++ b/src/session_impl.cpp @@ -2941,19 +2941,21 @@ namespace aux { #endif } - peer_connection_args pack; - pack.ses = this; - pack.sett = &m_settings; - pack.stats_counters = &m_stats_counters; - pack.disk_thread = &m_disk_thread; - pack.ios = &m_io_service; - pack.tor = std::weak_ptr(); - pack.s = s; - pack.endp = endp; - pack.peerinfo = nullptr; + peer_connection_args pack{ + this + , &m_settings + , &m_stats_counters + , &m_disk_thread + , &m_io_service + , std::weak_ptr() + , s + , endp + , nullptr + , aux::generate_peer_id(m_settings) + }; std::shared_ptr c - = std::make_shared(pack); + = std::make_shared(std::move(pack)); if (!c->is_disconnecting()) { diff --git a/src/torrent.cpp b/src/torrent.cpp index d26833863..4583916d3 100644 --- a/src/torrent.cpp +++ b/src/torrent.cpp @@ -1963,11 +1963,14 @@ bool is_downloading_state(int const st) return nullptr; } + bool torrent::is_self_connection(peer_id const& pid) const + { + return m_outgoing_pids.count(pid) > 0; + } + void torrent::on_resume_data_checked(status_t const status , storage_error const& error) try { - // hold a reference until this function returns - #if TORRENT_USE_ASSERTS TORRENT_ASSERT(m_outstanding_check_files); m_outstanding_check_files = false; @@ -4412,6 +4415,7 @@ bool is_downloading_state(int const st) // have been destructed if (m_peer_list) m_peer_list->clear(); m_connections.clear(); + m_outgoing_pids.clear(); m_peers_to_disconnect.clear(); m_num_uploads = 0; m_num_connecting = 0; @@ -5454,6 +5458,12 @@ bool is_downloading_state(int const st) TORRENT_ASSERT(std::count(m_peers_to_disconnect.begin() , m_peers_to_disconnect.end(), p) == 0); + auto it = m_outgoing_pids.find(p->our_pid()); + if (it != m_outgoing_pids.end()) + { + m_outgoing_pids.erase(it); + } + // only schedule the peer for actual removal if in fact // we can be sure peer_connection will be kept alive until // the deferred function is called. If a peer_connection @@ -6018,24 +6028,27 @@ bool is_downloading_state(int const st) return; } + peer_connection_args pack{ + &m_ses + , &settings() + , &m_ses.stats_counters() + , &m_ses.disk_thread() + , &m_ses.get_io_service() + , shared_from_this() + , s + , a + , &web->peer_info + , aux::generate_peer_id(settings()) + }; + std::shared_ptr c; - peer_connection_args pack; - pack.ses = &m_ses; - pack.sett = &settings(); - pack.stats_counters = &m_ses.stats_counters(); - pack.disk_thread = &m_ses.disk_thread(); - pack.ios = &m_ses.get_io_service(); - pack.tor = shared_from_this(); - pack.s = s; - pack.endp = a; - pack.peerinfo = &web->peer_info; if (web->type == web_seed_entry::url_seed) { - c = std::make_shared(pack, *web); + c = std::make_shared(std::move(pack), *web); } else if (web->type == web_seed_entry::http_seed) { - c = std::make_shared(pack, *web); + c = std::make_shared(std::move(pack), *web); } if (!c) return; @@ -6630,18 +6643,21 @@ bool is_downloading_state(int const st) #endif } - peer_connection_args pack; - pack.ses = &m_ses; - pack.sett = &settings(); - pack.stats_counters = &m_ses.stats_counters(); - pack.disk_thread = &m_ses.disk_thread(); - pack.ios = &m_ses.get_io_service(); - pack.tor = shared_from_this(); - pack.s = s; - pack.endp = a; - pack.peerinfo = peerinfo; + peer_id const our_pid = aux::generate_peer_id(settings()); + peer_connection_args pack{ + &m_ses + , &settings() + , &m_ses.stats_counters() + , &m_ses.disk_thread() + , &m_ses.get_io_service() + , shared_from_this() + , s + , a + , peerinfo + , our_pid + }; - std::shared_ptr c = std::make_shared(pack); + auto c = std::make_shared(std::move(pack)); #if TORRENT_USE_ASSERTS c->m_in_constructor = false; @@ -6672,6 +6688,7 @@ bool is_downloading_state(int const st) sorted_insert(m_connections, c.get()); TORRENT_TRY { + m_outgoing_pids.insert(our_pid); m_ses.insert_peer(c); need_peer_list(); m_peer_list->set_connection(peerinfo, c.get()); @@ -7760,6 +7777,8 @@ bool is_downloading_state(int const st) #if TORRENT_USE_INVARIANT_CHECKS void torrent::check_invariant() const { + TORRENT_ASSERT(m_connections.size() >= m_outgoing_pids.size()); + // the piece picker and the file progress states are supposed to be // created in sync TORRENT_ASSERT(has_picker() == !m_file_progress.empty()); diff --git a/test/test_peer_list.cpp b/test/test_peer_list.cpp index 18c9e805d..34d6dcb76 100644 --- a/test/test_peer_list.cpp +++ b/test/test_peer_list.cpp @@ -63,6 +63,7 @@ struct mock_peer_connection , m_tp(nullptr) , m_remote(remote) , m_local(ep("127.0.0.1", 8080)) + , m_our_id(nullptr) , m_disconnect_called(false) , m_torrent(*tor) { @@ -102,6 +103,7 @@ struct mock_peer_connection tcp::endpoint m_remote; tcp::endpoint m_local; peer_id m_id; + peer_id m_our_id; bool m_disconnect_called; mock_torrent& m_torrent; @@ -111,6 +113,7 @@ struct mock_peer_connection void disconnect(error_code const& ec , operation_t op, int error = 0) override; peer_id const& pid() const override { return m_id; } + peer_id our_pid() const override { return m_our_id; } void set_holepunch_mode() override {} torrent_peer* peer_info_struct() const override { return m_tp; } void set_peer_info(torrent_peer* pi) override { m_tp = pi; }