diff --git a/src/addrman.h b/src/addrman.h index 53cb22c194..2139186bea 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -158,7 +158,7 @@ public: #define ADDRMAN_GETADDR_MAX_PCT 23 //! the maximum number of nodes to return in a getaddr call -#define ADDRMAN_GETADDR_MAX 2500 +#define ADDRMAN_GETADDR_MAX 1000 //! Convenience #define ADDRMAN_TRIED_BUCKET_COUNT (1 << ADDRMAN_TRIED_BUCKET_COUNT_LOG2) diff --git a/src/net.cpp b/src/net.cpp index a5b28c6d72..6e63645012 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3352,7 +3352,24 @@ CConnman::~CConnman() std::vector CConnman::GetAddresses() { - return addrman.GetAddr(); + std::vector addresses = addrman.GetAddr(); + if (m_banman) { + addresses.erase(std::remove_if(addresses.begin(), addresses.end(), + [this](const CAddress& addr){return m_banman->IsDiscouraged(addr) || m_banman->IsBanned(addr);}), + addresses.end()); + } + return addresses; +} + +std::vector CConnman::GetAddresses(Network requestor_network) +{ + const auto current_time = GetTime(); + if (m_addr_response_caches.find(requestor_network) == m_addr_response_caches.end() || + m_addr_response_caches[requestor_network].m_update_addr_response < current_time) { + m_addr_response_caches[requestor_network].m_addrs_response_cache = GetAddresses(); + m_addr_response_caches[requestor_network].m_update_addr_response = current_time + std::chrono::hours(21) + GetRandMillis(std::chrono::hours(6)); + } + return m_addr_response_caches[requestor_network].m_addrs_response_cache; } bool CConnman::AddNode(const std::string& strNode) diff --git a/src/net.h b/src/net.h index 525b489ac2..cff17a38e0 100644 --- a/src/net.h +++ b/src/net.h @@ -31,6 +31,7 @@ #include #include #include +#include #include #include #include @@ -67,6 +68,9 @@ static const int FEELER_INTERVAL = 120; static const unsigned int MAX_INV_SZ = 50000; /** The maximum number of new addresses to accumulate before announcing. */ static const unsigned int MAX_ADDR_TO_SEND = 1000; +// TODO: remove ADDRMAN_GETADDR_MAX and let the caller specify this limit with MAX_ADDR_TO_SEND. +static_assert(MAX_ADDR_TO_SEND == ADDRMAN_GETADDR_MAX, + "Max allowed ADDR message size should be equal to the max number of records returned from AddrMan."); /** Maximum length of incoming protocol messages (no message over 3 MiB is currently acceptable). */ static const unsigned int MAX_PROTOCOL_MESSAGE_LENGTH = 3 * 1024 * 1024; /** Maximum length of the user agent string in `version` message */ @@ -397,6 +401,13 @@ public: // Addrman functions std::vector GetAddresses(); + /** + * Cache is used to minimize topology leaks, so it should + * be used for all non-trusted calls, for example, p2p. + * A non-malicious call (from RPC or a peer with addr permission) should + * call the function without a parameter to avoid using the cache. + */ + std::vector GetAddresses(Network requestor_network); // This allows temporarily exceeding m_max_outbound_full_relay, with the goal of finding // a peer that is better than all our current peers. @@ -592,6 +603,29 @@ private: std::atomic nLastNodeId{0}; unsigned int nPrevNodeCount{0}; + /** + * Cache responses to addr requests to minimize privacy leak. + * Attack example: scraping addrs in real-time may allow an attacker + * to infer new connections of the victim by detecting new records + * with fresh timestamps (per self-announcement). + */ + struct CachedAddrResponse { + std::vector m_addrs_response_cache; + std::chrono::microseconds m_update_addr_response{0}; + }; + + /** + * Addr responses stored in different caches + * per network prevent cross-network node identification. + * If a node for example is multi-homed under Tor and IPv6, + * a single cache (or no cache at all) would let an attacker + * to easily detect that it is the same node by comparing responses. + * The used memory equals to 1000 CAddress records (or around 32 bytes) per + * distinct Network (up to 5) we have/had an inbound peer from, + * resulting in at most ~160 KB. + */ + std::map m_addr_response_caches; + /** * Services this instance offers. * diff --git a/src/net_permissions.cpp b/src/net_permissions.cpp index 0973d66425..f2e519eb26 100644 --- a/src/net_permissions.cpp +++ b/src/net_permissions.cpp @@ -15,6 +15,7 @@ const std::vector NET_PERMISSIONS_DOC{ "relay (relay even in -blocksonly mode)", "mempool (allow requesting BIP35 mempool contents)", "download (allow getheaders during IBD, no disconnect after maxuploadtarget limit)", + "addr (responses to GETADDR avoid hitting the cache and contain random records with the most up-to-date info)" }; namespace { @@ -50,6 +51,7 @@ bool TryParsePermissionFlags(const std::string& str, NetPermissionFlags& output, else if (permission == "download") NetPermissions::AddFlag(flags, PF_DOWNLOAD); else if (permission == "all") NetPermissions::AddFlag(flags, PF_ALL); else if (permission == "relay") NetPermissions::AddFlag(flags, PF_RELAY); + else if (permission == "addr") NetPermissions::AddFlag(flags, PF_ADDR); else if (permission.length() == 0); // Allow empty entries else { error = strprintf(_("Invalid P2P permission: '%s'"), permission); @@ -75,6 +77,7 @@ std::vector NetPermissions::ToStrings(NetPermissionFlags flags) if (NetPermissions::HasFlag(flags, PF_RELAY)) strings.push_back("relay"); if (NetPermissions::HasFlag(flags, PF_MEMPOOL)) strings.push_back("mempool"); if (NetPermissions::HasFlag(flags, PF_DOWNLOAD)) strings.push_back("download"); + if (NetPermissions::HasFlag(flags, PF_ADDR)) strings.push_back("addr"); return strings; } diff --git a/src/net_permissions.h b/src/net_permissions.h index 78efc97dbe..cb50744906 100644 --- a/src/net_permissions.h +++ b/src/net_permissions.h @@ -29,10 +29,12 @@ enum NetPermissionFlags { PF_NOBAN = (1U << 4) | PF_DOWNLOAD, // Can query the mempool PF_MEMPOOL = (1U << 5), + // Can request addrs without hitting a privacy-preserving cache + PF_ADDR = (1U << 7), // True if the user did not specifically set fine grained permissions PF_ISIMPLICIT = (1U << 31), - PF_ALL = PF_BLOOMFILTER | PF_FORCERELAY | PF_RELAY | PF_NOBAN | PF_MEMPOOL | PF_DOWNLOAD, + PF_ALL = PF_BLOOMFILTER | PF_FORCERELAY | PF_RELAY | PF_NOBAN | PF_MEMPOOL | PF_DOWNLOAD | PF_ADDR, }; class NetPermissions diff --git a/src/net_processing.cpp b/src/net_processing.cpp index a47a45a754..294d2eb308 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3076,7 +3076,7 @@ void PeerManagerImpl::ProcessMessage( if (!pfrom.IsAddrRelayPeer()) { return; } - if (vAddr.size() > 1000) + if (vAddr.size() > MAX_ADDR_TO_SEND) { Misbehaving(pfrom.GetId(), 20, strprintf("%s message size = %u", msg_type, vAddr.size())); return; @@ -4026,12 +4026,15 @@ void PeerManagerImpl::ProcessMessage( pfrom.fSentAddr = true; pfrom.vAddrToSend.clear(); - std::vector vAddr = m_connman.GetAddresses(); + std::vector vAddr; + if (pfrom.HasPermission(PF_ADDR)) { + vAddr = m_connman.GetAddresses(); + } else { + vAddr = m_connman.GetAddresses(pfrom.addr.GetNetwork()); + } FastRandomContext insecure_rand; for (const CAddress &addr : vAddr) { - if (!m_banman->IsDiscouraged(addr) && !m_banman->IsBanned(addr)) { - pfrom.PushAddress(addr, insecure_rand); - } + pfrom.PushAddress(addr, insecure_rand); } return; } @@ -4674,8 +4677,8 @@ bool PeerManagerImpl::SendMessages(CNode* pto) { pto->m_addr_known->insert(addr.GetKey()); vAddr.push_back(addr); - // receiver rejects addr messages larger than 1000 - if (vAddr.size() >= 1000) + // receiver rejects addr messages larger than MAX_ADDR_TO_SEND + if (vAddr.size() >= MAX_ADDR_TO_SEND) { m_connman.PushMessage(pto, msgMaker.Make(make_flags, msg_type, vAddr)); vAddr.clear(); diff --git a/src/test/fuzz/net_permissions.cpp b/src/test/fuzz/net_permissions.cpp index 9410c2f55d..3620e16d30 100644 --- a/src/test/fuzz/net_permissions.cpp +++ b/src/test/fuzz/net_permissions.cpp @@ -24,6 +24,7 @@ FUZZ_TARGET(net_permissions) NetPermissionFlags::PF_FORCERELAY, NetPermissionFlags::PF_NOBAN, NetPermissionFlags::PF_MEMPOOL, + NetPermissionFlags::PF_ADDR, NetPermissionFlags::PF_ISIMPLICIT, NetPermissionFlags::PF_ALL, }) : diff --git a/src/test/netbase_tests.cpp b/src/test/netbase_tests.cpp index 43f1ff27a9..3150d772d5 100644 --- a/src/test/netbase_tests.cpp +++ b/src/test/netbase_tests.cpp @@ -521,13 +521,14 @@ BOOST_AUTO_TEST_CASE(netpermissions_test) BOOST_CHECK(NetWhitelistPermissions::TryParse("bloom,forcerelay,noban,relay,mempool@1.2.3.4/32", whitelistPermissions, error)); const auto strings = NetPermissions::ToStrings(PF_ALL); - BOOST_CHECK_EQUAL(strings.size(), 6); + BOOST_CHECK_EQUAL(strings.size(), 7); BOOST_CHECK(std::find(strings.begin(), strings.end(), "bloomfilter") != strings.end()); BOOST_CHECK(std::find(strings.begin(), strings.end(), "forcerelay") != strings.end()); BOOST_CHECK(std::find(strings.begin(), strings.end(), "relay") != strings.end()); BOOST_CHECK(std::find(strings.begin(), strings.end(), "noban") != strings.end()); BOOST_CHECK(std::find(strings.begin(), strings.end(), "mempool") != strings.end()); BOOST_CHECK(std::find(strings.begin(), strings.end(), "download") != strings.end()); + BOOST_CHECK(std::find(strings.begin(), strings.end(), "addr") != strings.end()); } BOOST_AUTO_TEST_CASE(netbase_dont_resolve_strings_with_embedded_nul_characters) diff --git a/test/functional/p2p_getaddr_caching.py b/test/functional/p2p_getaddr_caching.py new file mode 100755 index 0000000000..9197845d82 --- /dev/null +++ b/test/functional/p2p_getaddr_caching.py @@ -0,0 +1,108 @@ +#!/usr/bin/env python3 +# Copyright (c) 2020 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +"""Test addr response caching""" + +import time +from test_framework.messages import ( + CAddress, + NODE_NETWORK, + msg_addr, + msg_getaddr, +) +from test_framework.mininode import ( + P2PInterface, + mininode_lock +) +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import ( + assert_equal, +) + +MAX_ADDR_TO_SEND = 1000 + +def gen_addrs(n): + addrs = [] + for i in range(n): + addr = CAddress() + addr.time = int(time.time()) + addr.nServices = NODE_NETWORK + # Use first octets to occupy different AddrMan buckets + first_octet = i >> 8 + second_octet = i % 256 + addr.ip = "{}.{}.1.1".format(first_octet, second_octet) + addr.port = 9999 + addrs.append(addr) + return addrs + +class AddrReceiver(P2PInterface): + + def __init__(self): + super().__init__() + self.received_addrs = None + + def get_received_addrs(self): + with mininode_lock: + return self.received_addrs + + def on_addr(self, message): + self.received_addrs = [] + for addr in message.addrs: + self.received_addrs.append(addr.ip) + + def addr_received(self): + return self.received_addrs is not None + + +class AddrTest(BitcoinTestFramework): + def set_test_params(self): + self.setup_clean_chain = False + self.num_nodes = 1 + + def run_test(self): + self.log.info('Create connection that sends and requests addr messages') + addr_source = self.nodes[0].add_p2p_connection(P2PInterface()) + + msg_send_addrs = msg_addr() + self.log.info('Fill peer AddrMan with a lot of records') + # Since these addrs are sent from the same source, not all of them will be stored, + # because we allocate a limited number of AddrMan buckets per addr source. + total_addrs = 10000 + addrs = gen_addrs(total_addrs) + for i in range(int(total_addrs/MAX_ADDR_TO_SEND)): + msg_send_addrs.addrs = addrs[i * MAX_ADDR_TO_SEND:(i + 1) * MAX_ADDR_TO_SEND] + addr_source.send_and_ping(msg_send_addrs) + + responses = [] + self.log.info('Send many addr requests within short time to receive same response') + N = 5 + cur_mock_time = int(time.time()) + for i in range(N): + addr_receiver = self.nodes[0].add_p2p_connection(AddrReceiver()) + addr_receiver.send_and_ping(msg_getaddr()) + # Trigger response + cur_mock_time += 5 * 60 + self.nodes[0].setmocktime(cur_mock_time) + addr_receiver.wait_until(addr_receiver.addr_received) + responses.append(addr_receiver.get_received_addrs()) + for response in responses[1:]: + assert_equal(response, responses[0]) + assert(len(response) < MAX_ADDR_TO_SEND) + + cur_mock_time += 3 * 24 * 60 * 60 + self.nodes[0].setmocktime(cur_mock_time) + + self.log.info('After time passed, see a new response to addr request') + last_addr_receiver = self.nodes[0].add_p2p_connection(AddrReceiver()) + last_addr_receiver.send_and_ping(msg_getaddr()) + # Trigger response + cur_mock_time += 5 * 60 + self.nodes[0].setmocktime(cur_mock_time) + last_addr_receiver.wait_until(last_addr_receiver.addr_received) + # new response is different + assert(set(responses[0]) != set(last_addr_receiver.get_received_addrs())) + + +if __name__ == '__main__': + AddrTest().main() diff --git a/test/functional/p2p_permissions.py b/test/functional/p2p_permissions.py index c5ad7111e5..b8abd465d3 100755 --- a/test/functional/p2p_permissions.py +++ b/test/functional/p2p_permissions.py @@ -94,7 +94,7 @@ class P2PPermissionsTests(BitcoinTestFramework): self.checkpermission( # all permission added ["-whitelist=all@127.0.0.1"], - ["forcerelay", "noban", "mempool", "bloomfilter", "relay", "download"], + ["forcerelay", "noban", "mempool", "bloomfilter", "relay", "download", "addr"], False) self.stop_node(1) diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 506a473b3d..9ed602f6bf 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -173,6 +173,7 @@ BASE_SCRIPTS = [ 'rpc_blockchain.py', 'rpc_deprecated.py', 'wallet_disable.py', + 'p2p_getaddr_caching.py', 'p2p_getdata.py', 'rpc_net.py', 'wallet_keypool.py',