Replace automatic bans with discouragement filter

sipa · sipa · commit b691f2df5f7d · 2020-07-03T20:43:55.000-07:00
This patch improves performance and resource usage around IP
addresses that are banned for misbehavior. They're already not
actually banned, as connections from them are still allowed,
but they are preferred for eviction if the inbound connection
slots are full.

Stop treating these like manually banned IP ranges, and instead
just keep them in a rolling Bloom filter of misbehaving nodes,
which isn't persisted to disk or exposed through the ban
framework. The effect remains the same: preferred for eviction,
avoided for outgoing connections, and not relayed to other peers.

Also change the name of this mechanism to better reflect reality;
they're not banned, just discouraged.

Contains release notes and several interface improvements by
John Newbery.
diff --git a/doc/release-notes-19219.md 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
@@ -74,25 +74,24 @@ 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) {
         CSubNet sub_net = it.first;
         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
@@ -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 @@ class BanMan
     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
@@ -430,8 +430,8 @@ void SetupServerArgs(NodeContext& node)
 
     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
@@ -1012,15 +1012,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
@@ -1019,7 +1019,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)
 {
@@ -1035,14 +1035,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
@@ -1072,7 +1072,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);
@@ -1107,7 +1107,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)
  */
@@ -1339,7 +1339,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) {
@@ -3118,7 +3118,7 @@ void ProcessMessage(
             // 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, chainman, mempool, {cmpctblock.header}, chainparams, /*via_compact_block=*/true);
         }
 
@@ -3204,7 +3204,7 @@ void ProcessMessage(
                 // 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
@@ -3576,11 +3576,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
@@ -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
@@ -601,7 +601,8 @@ static UniValue setban(const JSONRPCRequest& request)
 
     if (strCommand == "add")
     {
-        if (isSubnet ? node.banman->IsBanned(subNet) : node.banman->IsBanned(netAddr)) {
+        if ((isSubnet && node.banman->IsBanned(subNet)) ||
+            (!isSubnet && node.banman->IsBannedLevel(netAddr) == BanReasonManuallyAdded)) {
             throw JSONRPCError(RPC_CLIENT_NODE_ALREADY_ADDED, "Error: IP/Subnet already banned");
         }
 
@@ -628,7 +629,7 @@ static UniValue setban(const JSONRPCRequest& request)
     else if(strCommand == "remove")
     {
         if (!( isSubnet ? node.banman->Unban(subNet) : 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;
@@ -637,7 +638,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, "", "",
             {
@@ -646,7 +647,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{
@@ -671,7 +671,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
@@ -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);
 }

Original file line number	Diff line number	Diff line change
`@@ -601,7 +601,8 @@ static UniValue setban(const JSONRPCRequest& request)`
`601`	`601`
`602`	`602`	`if (strCommand == "add")`
`603`	`603`	`{`
`604`		`- if (isSubnet ? node.banman->IsBanned(subNet) : node.banman->IsBanned(netAddr)) {`
	`604`	`+ if ((isSubnet && node.banman->IsBanned(subNet)) \|\|`
	`605`	`+ (!isSubnet && node.banman->IsBannedLevel(netAddr) == BanReasonManuallyAdded)) {`
`605`	`606`	`throw JSONRPCError(RPC_CLIENT_NODE_ALREADY_ADDED, "Error: IP/Subnet already banned");`
`606`	`607`	`}`
`607`	`608`
`@@ -628,7 +629,7 @@ static UniValue setban(const JSONRPCRequest& request)`
`628`	`629`	`else if(strCommand == "remove")`
`629`	`630`	`{`
`630`	`631`	`if (!( isSubnet ? node.banman->Unban(subNet) : node.banman->Unban(netAddr) )) {`
`631`		`- throw JSONRPCError(RPC_CLIENT_INVALID_IP_OR_SUBNET, "Error: Unban failed. Requested address/subnet was not previously banned.");`
	`632`	`+ throw JSONRPCError(RPC_CLIENT_INVALID_IP_OR_SUBNET, "Error: Unban failed. Requested address/subnet was not previously manually banned.");`
`632`	`633`	`}`
`633`	`634`	`}`
`634`	`635`	`return NullUniValue;`
`@@ -637,7 +638,7 @@ static UniValue setban(const JSONRPCRequest& request)`
`637`	`638`	`static UniValue listbanned(const JSONRPCRequest& request)`
`638`	`639`	`{`
`639`	`640`	`RPCHelpMan{"listbanned",`
`640`		`- "\nList all banned IPs/Subnets.\n",`
	`641`	`+ "\nList all manually banned IPs/Subnets.\n",`
`641`	`642`	`{},`
`642`	`643`	`RPCResult{RPCResult::Type::ARR, "", "",`
`643`	`644`	`{`
`@@ -646,7 +647,6 @@ static UniValue listbanned(const JSONRPCRequest& request)`
`646`	`647`	`{RPCResult::Type::STR, "address", ""},`
`647`	`648`	`{RPCResult::Type::NUM_TIME, "banned_until", ""},`
`648`	`649`	`{RPCResult::Type::NUM_TIME, "ban_created", ""},`
`649`		`- {RPCResult::Type::STR, "ban_reason", ""},`
`650`	`650`	`}},`
`651`	`651`	`}},`
`652`	`652`	`RPCExamples{`
`@@ -671,7 +671,6 @@ static UniValue listbanned(const JSONRPCRequest& request)`
`671`	`671`	`rec.pushKV("address", entry.first.ToString());`
`672`	`672`	`rec.pushKV("banned_until", banEntry.nBanUntil);`
`673`	`673`	`rec.pushKV("ban_created", banEntry.nCreateTime);`
`674`		`- rec.pushKV("ban_reason", banEntry.banReasonToString());`
`675`	`674`
`676`	`675`	`bannedAddresses.push_back(rec);`
`677`	`676`	`}`
Original file line number	Diff line number	Diff line change
`@@ -346,12 +346,6 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)`
`346`	`346`	`}`
`347`	`347`	`BOOST_CHECK(banman->IsBanned(addr));`
`348`	`348`
`349`		`- SetMockTime(nStartTime+60*60);`
`350`		`- BOOST_CHECK(banman->IsBanned(addr));`
`351`		`-`
`352`		`- SetMockTime(nStartTime+606024+1);`
`353`		`- BOOST_CHECK(!banman->IsBanned(addr));`
`354`		`-`
`355`	`349`	`bool dummy;`
`356`	`350`	`peerLogic->FinalizeNode(dummyNode.GetId(), dummy);`
`357`	`351`	`}`