merge bitcoin#18991: Cache responses to GETADDR to prevent topology leaks

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)

src/net.cpp

@@ -3352,7 +3352,24 @@ CConnman::~CConnman()
 
 std::vector<CAddress> CConnman::GetAddresses()
 {
-    return addrman.GetAddr();
+    std::vector<CAddress> 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<CAddress> CConnman::GetAddresses(Network requestor_network)
+{
+    const auto current_time = GetTime<std::chrono::microseconds>();
+    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)

src/net.h

@@ -31,6 +31,7 @@
 #include <atomic>
 #include <cstdint>
 #include <deque>
+#include <map>
 #include <thread>
 #include <memory>
 #include <condition_variable>
@@ -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<CAddress> 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<CAddress> 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<NodeId> 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<CAddress> 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<Network, CachedAddrResponse> m_addr_response_caches;
+
     /**
      * Services this instance offers.
      *

src/net_permissions.cpp

@@ -15,6 +15,7 @@ const std::vector<std::string> 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<std::string> 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;
 }
 

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

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<CAddress> vAddr = m_connman.GetAddresses();
+        std::vector<CAddress> 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();

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,
                                                                                          }) :

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)

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()

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)

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',