From e5a7dfd79f618b04e0140ec2c50c6e95c2a2e2e4 Mon Sep 17 00:00:00 2001 From: Daniela Brozzoni Date: Mon, 21 Jul 2025 14:46:36 +0200 Subject: [PATCH 1/2] p2p: rename GetAddresses -> GetAddressesUnsafe Rename GetAddresses to GetAddressesUnsafe to make it clearer that this function should only be used in trusted contexts. This helps avoid accidental privacy leaks by preventing the uncached version from being used in non-trusted scenarios, like P2P. --- src/net.cpp | 8 ++++---- src/net.h | 2 +- src/net_processing.cpp | 2 +- src/rpc/net.cpp | 2 +- src/test/fuzz/connman.cpp | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 490b766cb1d..5e6bc86372a 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3496,7 +3496,7 @@ CConnman::~CConnman() Stop(); } -std::vector CConnman::GetAddresses(size_t max_addresses, size_t max_pct, std::optional network, const bool filtered) const +std::vector CConnman::GetAddressesUnsafe(size_t max_addresses, size_t max_pct, std::optional network, const bool filtered) const { std::vector addresses = addrman.GetAddr(max_addresses, max_pct, network, filtered); if (m_banman) { @@ -3521,7 +3521,7 @@ std::vector CConnman::GetAddresses(CNode& requestor, size_t max_addres auto r = m_addr_response_caches.emplace(cache_id, CachedAddrResponse{}); CachedAddrResponse& cache_entry = r.first->second; if (cache_entry.m_cache_entry_expiration < current_time) { // If emplace() added new one it has expiration 0. - cache_entry.m_addrs_response_cache = GetAddresses(max_addresses, max_pct, /*network=*/std::nullopt); + cache_entry.m_addrs_response_cache = GetAddressesUnsafe(max_addresses, max_pct, /*network=*/std::nullopt); // Choosing a proper cache lifetime is a trade-off between the privacy leak minimization // and the usefulness of ADDR responses to honest users. // @@ -3964,8 +3964,8 @@ void CConnman::PerformReconnections() void CConnman::ASMapHealthCheck() { - const std::vector v4_addrs{GetAddresses(/*max_addresses=*/ 0, /*max_pct=*/ 0, Network::NET_IPV4, /*filtered=*/ false)}; - const std::vector v6_addrs{GetAddresses(/*max_addresses=*/ 0, /*max_pct=*/ 0, Network::NET_IPV6, /*filtered=*/ false)}; + const std::vector v4_addrs{GetAddressesUnsafe(/*max_addresses=*/0, /*max_pct=*/0, Network::NET_IPV4, /*filtered=*/false)}; + const std::vector v6_addrs{GetAddressesUnsafe(/*max_addresses=*/0, /*max_pct=*/0, Network::NET_IPV6, /*filtered=*/false)}; std::vector clearnet_addrs; clearnet_addrs.reserve(v4_addrs.size() + v6_addrs.size()); std::transform(v4_addrs.begin(), v4_addrs.end(), std::back_inserter(clearnet_addrs), diff --git a/src/net.h b/src/net.h index 4cb4cb906e2..20ed321164c 100644 --- a/src/net.h +++ b/src/net.h @@ -1176,7 +1176,7 @@ public: * @param[in] network Select only addresses of this network (nullopt = all). * @param[in] filtered Select only addresses that are considered high quality (false = all). */ - std::vector GetAddresses(size_t max_addresses, size_t max_pct, std::optional network, const bool filtered = true) const; + std::vector GetAddressesUnsafe(size_t max_addresses, size_t max_pct, std::optional network, const bool filtered = true) const; /** * Cache is used to minimize topology leaks, so it should * be used for all non-trusted calls, for example, p2p. diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 5e0651aa3a3..2d7c80b4b66 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4715,7 +4715,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, peer->m_addrs_to_send.clear(); std::vector vAddr; if (pfrom.HasPermission(NetPermissionFlags::Addr)) { - vAddr = m_connman.GetAddresses(MAX_ADDR_TO_SEND, MAX_PCT_ADDR_TO_SEND, /*network=*/std::nullopt); + vAddr = m_connman.GetAddressesUnsafe(MAX_ADDR_TO_SEND, MAX_PCT_ADDR_TO_SEND, /*network=*/std::nullopt); } else { vAddr = m_connman.GetAddresses(pfrom, MAX_ADDR_TO_SEND, MAX_PCT_ADDR_TO_SEND); } diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 2739b0d7dd0..2432d634593 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -953,7 +953,7 @@ static RPCHelpMan getnodeaddresses() } // returns a shuffled list of CAddress - const std::vector vAddr{connman.GetAddresses(count, /*max_pct=*/0, network)}; + const std::vector vAddr{connman.GetAddressesUnsafe(count, /*max_pct=*/0, network)}; UniValue ret(UniValue::VARR); for (const CAddress& addr : vAddr) { diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index a62d227da8e..c7509c54947 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -113,7 +113,7 @@ FUZZ_TARGET(connman, .init = initialize_connman) auto max_addresses = fuzzed_data_provider.ConsumeIntegral(); auto max_pct = fuzzed_data_provider.ConsumeIntegralInRange(0, 100); auto filtered = fuzzed_data_provider.ConsumeBool(); - (void)connman.GetAddresses(max_addresses, max_pct, /*network=*/std::nullopt, filtered); + (void)connman.GetAddressesUnsafe(max_addresses, max_pct, /*network=*/std::nullopt, filtered); }, [&] { auto max_addresses = fuzzed_data_provider.ConsumeIntegral(); From 1cb23997033c395d9ecd7bf2f54787b134485f41 Mon Sep 17 00:00:00 2001 From: Daniela Brozzoni Date: Mon, 21 Jul 2025 14:48:06 +0200 Subject: [PATCH 2/2] doc: clarify the GetAddresses/GetAddressesUnsafe documentation Better reflect in the documentation that the two methods should be used in different contexts. Also update the outdated "call the function without a parameter" phrasing in the cached version. This wording was accurate when the cache was introduced in #18991, but became outdated after later commits (f26502e9fc8a669b30717525597e3f468eaecf79, 81b00f87800f40cb14f2131ff27668bd2bb9e551) added parameters to each function, and the previous commit changed the function naming completely. Co-Authored-By: stickies-v --- src/net.h | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/net.h b/src/net.h index 20ed321164c..fa29c4c5e20 100644 --- a/src/net.h +++ b/src/net.h @@ -1169,7 +1169,10 @@ public: // Addrman functions /** - * Return all or many randomly selected addresses, optionally by network. + * Return randomly selected addresses. This function does not use the address response cache and + * should only be used in trusted contexts. + * + * An untrusted caller (e.g. from p2p) should instead use @ref GetAddresses to use the cache. * * @param[in] max_addresses Maximum number of addresses to return (0 = all). * @param[in] max_pct Maximum percentage of addresses to return (0 = all). Value must be from 0 to 100. @@ -1178,10 +1181,18 @@ public: */ std::vector GetAddressesUnsafe(size_t max_addresses, size_t max_pct, std::optional network, const bool filtered = true) const; /** - * 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. + * Return addresses from the per-requestor cache. If no cache entry exists, it is populated with + * randomly selected addresses. This function can be used in untrusted contexts. + * + * A trusted caller (e.g. from RPC or a peer with addr permission) can use + * @ref GetAddressesUnsafe to avoid using the cache. + * + * @param[in] requestor The requesting peer. Used to key the cache to prevent privacy leaks. + * @param[in] max_addresses Maximum number of addresses to return (0 = all). Ignored when cache + * already contains an entry for requestor. + * @param[in] max_pct Maximum percentage of addresses to return (0 = all). Value must be + * from 0 to 100. Ignored when cache already contains an entry for + * requestor. */ std::vector GetAddresses(CNode& requestor, size_t max_addresses, size_t max_pct);