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

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 12: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:

    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:

void CConnman::AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNetAddr &addr, const std::vector<NetWhitelistPermissions>& ranges) const {
    for (const auto& subnet : ranges) {
        if (subnet.m_subnet.Match(addr)) {
            NetPermissions::AddFlag(flags, subnet.m_flags);
        }
    }
    if (NetPermissions::HasFlag(flags, NetPermissionFlags::Implicit)) {
        NetPermissions::ClearFlag(flags, NetPermissionFlags::Implicit);
        if (whitelist_forcerelay) NetPermissions::AddFlag(flags, NetPermissionFlags::ForceRelay);
        if (whitelist_relay) NetPermissions::AddFlag(flags, NetPermissionFlags::Relay);
        NetPermissions::AddFlag(flags, NetPermissionFlags::Mempool);
        NetPermissions::AddFlag(flags, NetPermissionFlags::NoBan);
    }
}

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:

    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

fanquake referenced this in commit 9e56d8889a on Sep 24, 2025

achow101 referenced this in commit a0b5730f85 on Sep 25, 2025

sedited referenced this in commit 3f14dd4b2a on Oct 7, 2025

sedited referenced this in commit 5f9d179b8d on Oct 8, 2025

sedited referenced this in commit 845b93d99e on Oct 8, 2025

yuvicc referenced this in commit ccce70c31a on Oct 8, 2025

fanquake referenced this in commit be1a94a740 on Oct 9, 2025

sedited referenced this in commit 3b1e26d5d3 on Oct 10, 2025

sedited referenced this in commit 0fa2fdaca0 on Oct 11, 2025

fanquake referenced this in commit cf3a7e97ee on Oct 13, 2025

stringintech referenced this in commit f74b275ffc on Oct 14, 2025

stickies-v referenced this in commit a785fe6759 on Nov 5, 2025

tomt1664 referenced this in commit 0f06cebc61 on Nov 25, 2025

delta1 referenced this in commit 6664587c2f on Nov 27, 2025

morozow referenced this in commit 6d44e0e0f7 on May 8, 2026

morozow referenced this in commit fe918939e8 on May 8, 2026

morozow referenced this in commit 00232b7453 on May 8, 2026

morozow referenced this in commit 224d51fe6f on May 8, 2026

morozow referenced this in commit 3848de1a09 on May 8, 2026

morozow referenced this in commit 3366c6d621 on May 8, 2026

morozow referenced this in commit 2b1439f61d on May 8, 2026

Kino1994 referenced this in commit cedbf7c30d on Jun 28, 2026

BigcoinBGC referenced this in commit c670feea9b on Jun 30, 2026

Kino1994 referenced this in commit 67339deafc on Aug 19, 2026

bitcoin locked this on Sep 18, 2026