Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 17, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions src/net.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,9 +574,9 @@ void CNode::CloseSocketDisconnect()
m_i2p_sam_session.reset();
}

void CConnman::AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNetAddr &addr, const std::vector<NetWhitelistPermissions>& ranges) const {
void CConnman::AddWhitelistPermissionFlags(NetPermissionFlags& flags, std::optional<CNetAddr> addr, const std::vector<NetWhitelistPermissions>& ranges) const {
for (const auto& subnet : ranges) {
if (subnet.m_subnet.Match(addr)) {
if (addr.has_value() && subnet.m_subnet.Match(addr.value())) {
Comment on lines 578 to +579
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

NetPermissions::AddFlag(flags, subnet.m_flags);
}
}
Expand Down Expand Up @@ -1768,7 +1768,11 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
{
int nInbound = 0;

AddWhitelistPermissionFlags(permission_flags, addr, vWhitelistedRangeIncoming);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const bool inbound_onion = std::find(m_onion_binds.begin(), m_onion_binds.end(), addr_bind) != m_onion_binds.end();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Tor inbound connections do not reveal the peer's actual network address.
// Therefore do not apply address-based whitelist permissions to them.
AddWhitelistPermissionFlags(permission_flags, inbound_onion ? std::optional<CNetAddr>{} : addr, vWhitelistedRangeIncoming);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
AddWhitelistPermissionFlags(permission_flags, inbound_onion ? std::optional<CNetAddr>{} : addr, vWhitelistedRangeIncoming);
AddWhitelistPermissionFlags(permission_flags, inbound_onion ? std::nullopt : std::make_optional(addr), vWhitelistedRangeIncoming);

but it is a non-blocking comment.


{
LOCK(m_nodes_mutex);
Expand Down Expand Up @@ -1823,7 +1827,6 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
NodeId id = GetNewNodeId();
uint64_t nonce = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(id).Finalize();

const bool inbound_onion = std::find(m_onion_binds.begin(), m_onion_binds.end(), addr_bind) != m_onion_binds.end();
// The V2Transport transparently falls back to V1 behavior when an incoming V1 connection is
// detected, so use it whenever we signal NODE_P2P_V2.
ServiceFlags local_services = GetLocalServices();
Expand Down
2 changes: 1 addition & 1 deletion src/net.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1377,7 +1377,7 @@ class CConnman

bool AttemptToEvictConnection();
CNode* ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, ConnectionType conn_type, bool use_v2transport) EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex);
void AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNetAddr &addr, const std::vector<NetWhitelistPermissions>& ranges) const;
void AddWhitelistPermissionFlags(NetPermissionFlags& flags, std::optional<CNetAddr> addr, const std::vector<NetWhitelistPermissions>& ranges) const;

void DeleteNode(CNode* pnode);

Expand Down
Loading