net: do not apply whitelist permissions to onion inbounds

mzumsande commented at 8:14 pm on September 15, 2025: contributor

Tor inbound connections do not reveal the peer’s actual network address. Do not apply whitelist permissions to them since address-based matching is ineffective.

DrahtBot added the label P2P on Sep 15, 2025

DrahtBot commented at 8:14 pm on September 15, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33395.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	darosior, furszy, vasild
Stale ACK	achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32394 (net: make m_nodes_mutex non-recursive by vasild)
#32065 (i2p: make a time gap between creating transient sessions and using them by vasild)
#32015 (net: replace manual reference counting of CNode with shared_ptr by vasild)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

glozow requested review from vasild on Sep 15, 2025

achow101 commented at 11:18 pm on September 15, 2025: member

ACK e46a7a547371317a4d116b9b1a314917508ea480

furszy commented at 0:44 am on September 16, 2025: member

Code review ACK e46a7a547371317a4d116b9b1a314917508ea480

in src/net.cpp:1771 in e46a7a5473 outdated

1767@@ -1768,7 +1768,13 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
1768 {
1769     int nInbound = 0;
1770 
1771-    AddWhitelistPermissionFlags(permission_flags, addr, vWhitelistedRangeIncoming);
1772+    const bool inbound_onion = std::find(m_onion_binds.begin(), m_onion_binds.end(), addr_bind) != m_onion_binds.end();

vasild commented at 8:09 am on September 16, 2025:

nit, this is just moving code around without modifying it, feel free to ignore; can be written shorter as:

0    const bool inbound_onion = std::ranges::find(m_onion_binds, addr_bind) != m_onion_binds.end();

mzumsande commented at 5:45 pm on September 16, 2025:

think I’ll leave this for a refactoring PR that can apply this more systematically.

in src/net.cpp:1771 in e46a7a5473 outdated

1767@@ -1768,7 +1768,13 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
1768 {
1769     int nInbound = 0;
1770 
1771-    AddWhitelistPermissionFlags(permission_flags, addr, vWhitelistedRangeIncoming);

vasild commented at 8:21 am on September 16, 2025:

Previously AddWhitelistPermissionFlags() would have been called for incoming tor connections. It does this:

 0void CConnman::AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNetAddr &addr, const std::vector<NetWhitelistPermissions>& ranges) const {
 1    for (const auto& subnet : ranges) {
 2        if (subnet.m_subnet.Match(addr)) {
 3            NetPermissions::AddFlag(flags, subnet.m_flags);
 4        }
 5    }
 6    if (NetPermissions::HasFlag(flags, NetPermissionFlags::Implicit)) {
 7        NetPermissions::ClearFlag(flags, NetPermissionFlags::Implicit);
 8        if (whitelist_forcerelay) NetPermissions::AddFlag(flags, NetPermissionFlags::ForceRelay);
 9        if (whitelist_relay) NetPermissions::AddFlag(flags, NetPermissionFlags::Relay);
10        NetPermissions::AddFlag(flags, NetPermissionFlags::Mempool);
11        NetPermissions::AddFlag(flags, NetPermissionFlags::NoBan);
12    }
13}

there would be a match in the if inside the for loop which would be wrong, we want to avoid that. But what about the second section if (NetPermissions::HasFlag(flags, NetPermissionFlags::Implicit))? I think there is no need to omit that for incoming tor connections.

mzumsande commented at 5:40 pm on September 16, 2025:

Done with latest push, the section will now no longer be omitted (in the specific case of onion inbounds, I think it’s not possible to have other prior implicit permissions here so it also won’t actually be executed, but I agree it’s a cleaner approach).

darosior approved

darosior commented at 3:43 pm on September 16, 2025: member

utACK e46a7a547371317a4d116b9b1a314917508ea480

Happy to re-ACK if you take Vasil’s suggestion. I don’t think there is any functional difference, but the separation of concerns between -whitelist and -whitebind permissions is slightly neater.

net: Do not apply whitelist permission to onion inbounds

Tor inbound connections do not reveal the peer's actual network address.
Therefore do not apply whitelist permissions to them.

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

f563ce9081

mzumsande force-pushed on Sep 16, 2025

mzumsande commented at 5:42 pm on September 16, 2025: contributor

e46a7a5 to f563ce9: Addressed @vasild suggestion.

darosior approved

darosior commented at 6:14 pm on September 16, 2025: member

ACK f563ce90818d486d2a199439d2f6ba39cd106352

DrahtBot requested review from achow101 on Sep 16, 2025

DrahtBot requested review from furszy on Sep 16, 2025

in src/net.cpp:1775 in f563ce9081

1767@@ -1768,7 +1768,11 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
1768 {
1769     int nInbound = 0;
1770 
1771-    AddWhitelistPermissionFlags(permission_flags, addr, vWhitelistedRangeIncoming);
1772+    const bool inbound_onion = std::find(m_onion_binds.begin(), m_onion_binds.end(), addr_bind) != m_onion_binds.end();
1773+
1774+    // Tor inbound connections do not reveal the peer's actual network address.
1775+    // Therefore do not apply address-based whitelist permissions to them.
1776+    AddWhitelistPermissionFlags(permission_flags, inbound_onion ? std::optional<CNetAddr>{} : addr, vWhitelistedRangeIncoming);

furszy commented at 6:23 pm on September 16, 2025:

nano nit: usually, it is better to be explicit with the empty optionals:

0    AddWhitelistPermissionFlags(permission_flags, inbound_onion ? std::nullopt : std::make_optional(addr), vWhitelistedRangeIncoming);

but it is a non-blocking comment.

in src/net.cpp:579 in f563ce9081

573@@ -574,9 +574,9 @@ void CNode::CloseSocketDisconnect()
574     m_i2p_sam_session.reset();
575 }
576 
577-void CConnman::AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNetAddr &addr, const std::vector<NetWhitelistPermissions>& ranges) const {
578+void CConnman::AddWhitelistPermissionFlags(NetPermissionFlags& flags, std::optional<CNetAddr> addr, const std::vector<NetWhitelistPermissions>& ranges) const {
579     for (const auto& subnet : ranges) {
580-        if (subnet.m_subnet.Match(addr)) {
581+        if (addr.has_value() && subnet.m_subnet.Match(addr.value())) {

furszy commented at 6:26 pm on September 16, 2025:

nano nit for later: could skip the loop when addr == std::nullopt

furszy commented at 6:32 pm on September 16, 2025: member

ACK f563ce90818d486d2a199439d2f6ba39cd106352

Comments are tiny nits, not-blocking. The code is good as is.

vasild approved

vasild commented at 6:40 pm on September 16, 2025: contributor

ACK f563ce90818d486d2a199439d2f6ba39cd106352

fanquake merged this on Sep 17, 2025

fanquake closed this on Sep 17, 2025

fanquake referenced this in commit 2327b2b0db on Sep 17, 2025

fanquake referenced this in commit 61cdc04a83 on Sep 17, 2025

fanquake referenced this in commit 69ce524e46 on Sep 17, 2025

glozow referenced this in commit 7e1eca4882 on Sep 17, 2025

fanquake referenced this in commit 745fd1e064 on Sep 18, 2025

mzumsande deleted the branch on Sep 18, 2025

net: do not apply whitelist permissions to onion inbounds #33395

Code Coverage & Benchmarks

Reviews

Conflicts