diff options
| -rw-r--r-- | doc/release-notes-19219.md | 23 | ||||
| -rw-r--r-- | src/banman.cpp | 13 | ||||
| -rw-r--r-- | src/banman.h | 45 | ||||
| -rw-r--r-- | src/init.cpp | 4 | ||||
| -rw-r--r-- | src/net.cpp | 13 | ||||
| -rw-r--r-- | src/net_processing.cpp | 21 | ||||
| -rw-r--r-- | src/netaddress.h | 1 | ||||
| -rw-r--r-- | src/rpc/net.cpp | 9 | ||||
| -rw-r--r-- | src/test/denialofservice_tests.cpp | 6 |
9 files changed, 91 insertions, 44 deletions
diff --git a/doc/release-notes-19219.md b/doc/release-notes-19219.md new file mode 100644 index 0000000000..b5ee885ddc --- /dev/null +++ b/doc/release-notes-19219.md @@ -0,0 +1,23 @@ +#### Changes regarding misbehaving peers + +Peers that misbehave (e.g. send us invalid blocks) are now referred to as +discouraged nodes in log output, as they're not (and weren't) strictly banned: +incoming connections are still allowed from them, but they're preferred for +eviction. + +Furthermore, a few additional changes are introduced to how discouraged +addresses are treated: + +- Discouraging an address does not time out automatically after 24 hours + (or the `-bantime` setting). Depending on traffic from other peers, + discouragement may time out at an indeterminate time. + +- Discouragement is not persisted over restarts. + +- There is no method to list discouraged addresses. They are not returned by + the `listbanned` RPC. That RPC also no longer reports the `ban_reason` + field, as `"manually added"` is the only remaining option. + +- Discouragement cannot be removed with the `setban remove` RPC command. + If you need to remove a discouragement, you can remove all discouragements by + stop-starting your node. diff --git a/src/banman.cpp b/src/banman.cpp index 9cc584f0e4..16bd452704 100644 --- a/src/banman.cpp +++ b/src/banman.cpp @@ -74,7 +74,6 @@ int BanMan::IsBannedLevel(CNetAddr net_addr) // 0 - Not banned // 1 - Automatic misbehavior ban // 2 - Any other ban - int level = 0; auto current_time = GetTime(); LOCK(m_cs_banned); for (const auto& it : m_banned) { @@ -82,17 +81,17 @@ int BanMan::IsBannedLevel(CNetAddr net_addr) CBanEntry ban_entry = it.second; if (current_time < ban_entry.nBanUntil && sub_net.Match(net_addr)) { - if (ban_entry.banReason != BanReasonNodeMisbehaving) return 2; - level = 1; + return 2; } } - return level; + return m_discouraged.contains(net_addr.GetAddrBytes()) ? 1 : 0; } bool BanMan::IsBanned(CNetAddr net_addr) { auto current_time = GetTime(); LOCK(m_cs_banned); + if (m_discouraged.contains(net_addr.GetAddrBytes())) return true; for (const auto& it : m_banned) { CSubNet sub_net = it.first; CBanEntry ban_entry = it.second; @@ -120,12 +119,18 @@ bool BanMan::IsBanned(CSubNet sub_net) void BanMan::Ban(const CNetAddr& net_addr, const BanReason& ban_reason, int64_t ban_time_offset, bool since_unix_epoch) { + if (ban_reason == BanReasonNodeMisbehaving) { + LOCK(m_cs_banned); + m_discouraged.insert(net_addr.GetAddrBytes()); + return; + } CSubNet sub_net(net_addr); Ban(sub_net, ban_reason, ban_time_offset, since_unix_epoch); } void BanMan::Ban(const CSubNet& sub_net, const BanReason& ban_reason, int64_t ban_time_offset, bool since_unix_epoch) { + assert(ban_reason == BanReasonManuallyAdded); CBanEntry ban_entry(GetTime(), ban_reason); int64_t normalized_ban_time_offset = ban_time_offset; diff --git a/src/banman.h b/src/banman.h index 6bea2e75e9..aad73c514f 100644 --- a/src/banman.h +++ b/src/banman.h @@ -6,6 +6,7 @@ #define BITCOIN_BANMAN_H #include <addrdb.h> +#include <bloom.h> #include <fs.h> #include <net_types.h> // For banmap_t #include <sync.h> @@ -23,20 +24,35 @@ class CClientUIInterface; class CNetAddr; class CSubNet; -// Denial-of-service detection/prevention -// The idea is to detect peers that are behaving -// badly and disconnect/ban them, but do it in a -// one-coding-mistake-won't-shatter-the-entire-network -// way. -// IMPORTANT: There should be nothing I can give a -// node that it will forward on that will make that -// node's peers drop it. If there is, an attacker -// can isolate a node and/or try to split the network. -// Dropping a node for sending stuff that is invalid -// now but might be valid in a later version is also -// dangerous, because it can cause a network split -// between nodes running old code and nodes running -// new code. +// Banman manages two related but distinct concepts: +// +// 1. Banning. This is configured manually by the user, through the setban RPC. +// If an address or subnet is banned, we never accept incoming connections from +// it and never create outgoing connections to it. We won't gossip its address +// to other peers in addr messages. Banned addresses and subnets are stored to +// banlist.dat on shutdown and reloaded on startup. Banning can be used to +// prevent connections with spy nodes or other griefers. +// +// 2. Discouragement. If a peer misbehaves enough (see Misbehaving() in +// net_processing.cpp), we'll mark that address as discouraged. We still allow +// incoming connections from them, but they're preferred for eviction when +// we receive new incoming connections. We never make outgoing connections to +// them, and do not gossip their address to other peers. This is implemented as +// a bloom filter. We can (probabilistically) test for membership, but can't +// list all discouraged addresses or unmark them as discouraged. Discouragement +// can prevent our limited connection slots being used up by incompatible +// or broken peers. +// +// Neither banning nor discouragement are protections against denial-of-service +// attacks, since if an attacker has a way to waste our resources and we +// disconnect from them and ban that address, it's trivial for them to +// reconnect from another IP address. +// +// Attempting to automatically disconnect or ban any class of peer carries the +// risk of splitting the network. For example, if we banned/disconnected for a +// transaction that fails a policy check and a future version changes the +// policy check so the transaction is accepted, then that transaction could +// cause the network to split between old nodes and new nodes. class BanMan { @@ -68,6 +84,7 @@ private: CClientUIInterface* m_client_interface = nullptr; CBanDB m_ban_db; const int64_t m_default_ban_time; + CRollingBloomFilter m_discouraged GUARDED_BY(m_cs_banned) {50000, 0.000001}; }; #endif diff --git a/src/init.cpp b/src/init.cpp index d015fb4b6b..f381a53b1b 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -422,8 +422,8 @@ void SetupServerArgs() gArgs.AddArg("-addnode=<ip>", "Add a node to connect to and attempt to keep the connection open (see the `addnode` RPC command help for more info). This option can be specified multiple times to add multiple nodes.", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION); gArgs.AddArg("-asmap=<file>", strprintf("Specify asn mapping used for bucketing of the peers (default: %s). Relative paths will be prefixed by the net-specific datadir location.", DEFAULT_ASMAP_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); - gArgs.AddArg("-banscore=<n>", strprintf("Threshold for disconnecting misbehaving peers (default: %u)", DEFAULT_BANSCORE_THRESHOLD), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); - gArgs.AddArg("-bantime=<n>", strprintf("Number of seconds to keep misbehaving peers from reconnecting (default: %u)", DEFAULT_MISBEHAVING_BANTIME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); + gArgs.AddArg("-banscore=<n>", strprintf("Threshold for disconnecting and discouraging misbehaving peers (default: %u)", DEFAULT_BANSCORE_THRESHOLD), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); + gArgs.AddArg("-bantime=<n>", strprintf("Default duration (in seconds) of manually configured bans (default: %u)", DEFAULT_MISBEHAVING_BANTIME), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); gArgs.AddArg("-bind=<addr>", "Bind to given address and always listen on it. Use [host]:port notation for IPv6", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION); gArgs.AddArg("-connect=<ip>", "Connect only to the specified node; -noconnect disables automatic connections (the rules for this peer are the same as for -addnode). This option can be specified multiple times to connect to multiple nodes.", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION); gArgs.AddArg("-discover", "Discover own IP addresses (default: 1 when listening and no -externalip or -proxy)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); diff --git a/src/net.cpp b/src/net.cpp index dcc613ba88..ce85dc1d79 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -998,15 +998,22 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) { int bannedlevel = m_banman ? m_banman->IsBannedLevel(addr) : 0; - // Don't accept connections from banned peers, but if our inbound slots aren't almost full, accept - // if the only banning reason was an automatic misbehavior ban. - if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN) && bannedlevel > ((nInbound + 1 < nMaxInbound) ? 1 : 0)) + // Don't accept connections from banned peers. + if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN) && bannedlevel == 2) { LogPrint(BCLog::NET, "connection from %s dropped (banned)\n", addr.ToString()); CloseSocket(hSocket); return; } + // Only accept connections from discouraged peers if our inbound slots aren't (almost) full. + if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN) && nInbound + 1 >= nMaxInbound && bannedlevel >= 1) + { + LogPrint(BCLog::NET, "connection from %s dropped (discouraged)\n", addr.ToString()); + CloseSocket(hSocket); + return; + } + if (nInbound >= nMaxInbound) { if (!AttemptToEvictConnection()) { diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 2a61f84a08..71d206bc1a 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -963,7 +963,7 @@ unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans) } /** - * Mark a misbehaving peer to be banned depending upon the value of `-banscore`. + * Increment peer's misbehavior score. If the new value surpasses banscore (specified on startup or by default), mark node to be discouraged, meaning the peer might be disconnected & added to the discouragement filter. */ void Misbehaving(NodeId pnode, int howmuch, const std::string& message) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { @@ -979,14 +979,14 @@ void Misbehaving(NodeId pnode, int howmuch, const std::string& message) EXCLUSIV std::string message_prefixed = message.empty() ? "" : (": " + message); if (state->nMisbehavior >= banscore && state->nMisbehavior - howmuch < banscore) { - LogPrint(BCLog::NET, "%s: %s peer=%d (%d -> %d) BAN THRESHOLD EXCEEDED%s\n", __func__, state->name, pnode, state->nMisbehavior-howmuch, state->nMisbehavior, message_prefixed); + LogPrint(BCLog::NET, "%s: %s peer=%d (%d -> %d) DISCOURAGE THRESHOLD EXCEEDED%s\n", __func__, state->name, pnode, state->nMisbehavior-howmuch, state->nMisbehavior, message_prefixed); state->fShouldBan = true; } else LogPrint(BCLog::NET, "%s: %s peer=%d (%d -> %d)%s\n", __func__, state->name, pnode, state->nMisbehavior-howmuch, state->nMisbehavior, message_prefixed); } /** - * Potentially ban a node based on the contents of a BlockValidationState object + * Potentially mark a node discouraged based on the contents of a BlockValidationState object * * @param[in] via_compact_block this bool is passed in because net_processing should * punish peers differently depending on whether the data was provided in a compact @@ -1016,7 +1016,7 @@ static bool MaybePunishNodeForBlock(NodeId nodeid, const BlockValidationState& s break; } - // Ban outbound (but not inbound) peers if on an invalid chain. + // Discourage outbound (but not inbound) peers if on an invalid chain. // Exempt HB compact block peers and manual connections. if (!via_compact_block && !node_state->m_is_inbound && !node_state->m_is_manual_connection) { Misbehaving(nodeid, 100, message); @@ -1051,7 +1051,7 @@ static bool MaybePunishNodeForBlock(NodeId nodeid, const BlockValidationState& s } /** - * Potentially ban a node based on the contents of a TxValidationState object + * Potentially disconnect and discourage a node based on the contents of a TxValidationState object * * @return Returns true if the peer was punished (probably disconnected) */ @@ -1278,7 +1278,7 @@ void PeerLogicValidation::UpdatedBlockTip(const CBlockIndex *pindexNew, const CB } /** - * Handle invalid block rejection and consequent peer banning, maintain which + * Handle invalid block rejection and consequent peer discouragement, maintain which * peers announce compact blocks. */ void PeerLogicValidation::BlockChecked(const CBlock& block, const BlockValidationState& state) { @@ -2833,7 +2833,7 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec // relayed before full validation (see BIP 152), so we don't want to disconnect // the peer if the header turns out to be for an invalid block. // Note that if a peer tries to build on an invalid chain, that - // will be detected and the peer will be banned. + // will be detected and the peer will be disconnected/discouraged. return ProcessHeadersMessage(pfrom, connman, mempool, {cmpctblock.header}, chainparams, /*via_compact_block=*/true); } @@ -2919,7 +2919,7 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec // 3. the block is otherwise invalid (eg invalid coinbase, // block is too big, too many legacy sigops, etc). // So if CheckBlock failed, #3 is the only possibility. - // Under BIP 152, we don't DoS-ban unless proof of work is + // Under BIP 152, we don't discourage the peer unless proof of work is // invalid (we don't require all the stateless checks to have // been run). This is handled below, so just treat this as // though the block was successfully read, and rely on the @@ -3267,11 +3267,12 @@ bool PeerLogicValidation::CheckIfBanned(CNode* pnode) else if (pnode->m_manual_connection) LogPrintf("Warning: not punishing manually-connected peer %s!\n", pnode->addr.ToString()); else if (pnode->addr.IsLocal()) { - // Disconnect but don't ban _this_ local node - LogPrintf("Warning: disconnecting but not banning local peer %s!\n", pnode->addr.ToString()); + // Disconnect but don't discourage this local node + LogPrintf("Warning: disconnecting but not discouraging local peer %s!\n", pnode->addr.ToString()); pnode->fDisconnect = true; } else { // Disconnect and ban all nodes sharing the address + LogPrintf("Disconnecting and discouraging peer %s!\n", pnode->addr.ToString()); if (m_banman) { m_banman->Ban(pnode->addr, BanReasonNodeMisbehaving); } diff --git a/src/netaddress.h b/src/netaddress.h index b7381c1eb4..0c8d600a4a 100644 --- a/src/netaddress.h +++ b/src/netaddress.h @@ -90,6 +90,7 @@ class CNetAddr uint32_t GetMappedAS(const std::vector<bool> &asmap) const; std::vector<unsigned char> GetGroup(const std::vector<bool> &asmap) const; + std::vector<unsigned char> GetAddrBytes() const { return {std::begin(ip), std::end(ip)}; } int GetReachabilityFrom(const CNetAddr *paddrPartner = nullptr) const; explicit CNetAddr(const struct in6_addr& pipv6Addr, const uint32_t scope = 0); diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 10562126db..3886efcadf 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -591,7 +591,8 @@ static UniValue setban(const JSONRPCRequest& request) if (strCommand == "add") { - if (isSubnet ? g_rpc_node->banman->IsBanned(subNet) : g_rpc_node->banman->IsBanned(netAddr)) { + if ((isSubnet && g_rpc_node->banman->IsBanned(subNet)) || + (!isSubnet && g_rpc_node->banman->IsBannedLevel(netAddr) == BanReasonManuallyAdded)) { throw JSONRPCError(RPC_CLIENT_NODE_ALREADY_ADDED, "Error: IP/Subnet already banned"); } @@ -618,7 +619,7 @@ static UniValue setban(const JSONRPCRequest& request) else if(strCommand == "remove") { if (!( isSubnet ? g_rpc_node->banman->Unban(subNet) : g_rpc_node->banman->Unban(netAddr) )) { - throw JSONRPCError(RPC_CLIENT_INVALID_IP_OR_SUBNET, "Error: Unban failed. Requested address/subnet was not previously banned."); + throw JSONRPCError(RPC_CLIENT_INVALID_IP_OR_SUBNET, "Error: Unban failed. Requested address/subnet was not previously manually banned."); } } return NullUniValue; @@ -627,7 +628,7 @@ static UniValue setban(const JSONRPCRequest& request) static UniValue listbanned(const JSONRPCRequest& request) { RPCHelpMan{"listbanned", - "\nList all banned IPs/Subnets.\n", + "\nList all manually banned IPs/Subnets.\n", {}, RPCResult{RPCResult::Type::ARR, "", "", { @@ -636,7 +637,6 @@ static UniValue listbanned(const JSONRPCRequest& request) {RPCResult::Type::STR, "address", ""}, {RPCResult::Type::NUM_TIME, "banned_until", ""}, {RPCResult::Type::NUM_TIME, "ban_created", ""}, - {RPCResult::Type::STR, "ban_reason", ""}, }}, }}, RPCExamples{ @@ -660,7 +660,6 @@ static UniValue listbanned(const JSONRPCRequest& request) rec.pushKV("address", entry.first.ToString()); rec.pushKV("banned_until", banEntry.nBanUntil); rec.pushKV("ban_created", banEntry.nCreateTime); - rec.pushKV("ban_reason", banEntry.banReasonToString()); bannedAddresses.push_back(rec); } diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 6314c1a42f..bbe4df8550 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -346,12 +346,6 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) } BOOST_CHECK(banman->IsBanned(addr)); - SetMockTime(nStartTime+60*60); - BOOST_CHECK(banman->IsBanned(addr)); - - SetMockTime(nStartTime+60*60*24+1); - BOOST_CHECK(!banman->IsBanned(addr)); - bool dummy; peerLogic->FinalizeNode(dummyNode.GetId(), dummy); } |