Merge branch 'misbehaving_limit-0.20' into p2p_permission_download-0.…

…20+knots
bitcoinknots · Jun 12, 2020 · bbea249 · bbea249
2 parents 42ee92a + d53b50c
commit bbea249
Show file tree

Hide file tree

Showing 6 changed files with 183 additions and 15 deletions.
diff --git a/src/banman.cpp b/src/banman.cpp
@@ -11,6 +11,8 @@
 #include <util/time.h>
 #include <util/translation.h>
 
+#include <algorithm>
+
 
 BanMan::BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t default_ban_time)
     : m_client_interface(client_interface), m_ban_db(std::move(ban_file)), m_default_ban_time(default_ban_time)
@@ -39,6 +41,13 @@ BanMan::~BanMan()
     DumpBanlist();
 }
 
+void BanMan::SetMisbehavingLimit(const size_t limit)
+{
+    LOCK(m_cs_banned);
+    // NOTE: For now, this only works before bans are set!
+    m_misbehaving_addrs.set_capacity(limit);
+}
+
 void BanMan::DumpBanlist()
 {
     SweepBanned(); // clean unused entries (if bantime has expired)
@@ -61,7 +70,9 @@ void BanMan::ClearBanned()
 {
     {
         LOCK(m_cs_banned);
-        m_banned.clear();
+        m_banned_addrs.clear();
+        m_misbehaving_addrs.clear();
+        m_banned_subnets.clear();
         m_is_dirty = true;
     }
     DumpBanlist(); //store banlist to disk
@@ -77,7 +88,16 @@ int BanMan::IsBannedLevel(CNetAddr net_addr)
     int level = 0;
     auto current_time = GetTime();
     LOCK(m_cs_banned);
-    for (const auto& it : m_banned) {
+    const auto addr_ban_entry = m_banned_addrs.find(net_addr);
+    if (addr_ban_entry != m_banned_addrs.end()) {
+        const CBanEntry& ban_entry = addr_ban_entry->second;
+
+        if (current_time < ban_entry.nBanUntil) {
+            if (ban_entry.banReason != BanReasonNodeMisbehaving) return 2;
+            level = 1;
+        }
+    }
+    for (const auto& it : m_banned_subnets) {
         CSubNet sub_net = it.first;
         CBanEntry ban_entry = it.second;
 
@@ -93,7 +113,16 @@ bool BanMan::IsBanned(CNetAddr net_addr)
 {
     auto current_time = GetTime();
     LOCK(m_cs_banned);
-    for (const auto& it : m_banned) {
+    {
+        const auto it = m_banned_addrs.find(net_addr);
+        if (it != m_banned_addrs.end()) {
+            CBanEntry ban_entry = it->second;
+            if (current_time < ban_entry.nBanUntil) {
+                return true;
+            }
+        }
+    }
+    for (const auto& it : m_banned_subnets) {
         CSubNet sub_net = it.first;
         CBanEntry ban_entry = it.second;
 
@@ -106,10 +135,14 @@ bool BanMan::IsBanned(CNetAddr net_addr)
 
 bool BanMan::IsBanned(CSubNet sub_net)
 {
+    const CNetAddr* addr;
+    if (sub_net.IsSingleAddr(&addr)) {
+        return IsBanned(*addr);
+    }
     auto current_time = GetTime();
     LOCK(m_cs_banned);
-    banmap_t::iterator i = m_banned.find(sub_net);
-    if (i != m_banned.end()) {
+    banmap_t::iterator i = m_banned_subnets.find(sub_net);
+    if (i != m_banned_subnets.end()) {
         CBanEntry ban_entry = (*i).second;
         if (current_time < ban_entry.nBanUntil) {
             return true;
@@ -138,8 +171,36 @@ void BanMan::Ban(const CSubNet& sub_net, const BanReason& ban_reason, int64_t ba
 
     {
         LOCK(m_cs_banned);
-        if (m_banned[sub_net].nBanUntil < ban_entry.nBanUntil) {
-            m_banned[sub_net] = ban_entry;
+        const CNetAddr *addr = nullptr;
+        const bool is_single_addr = sub_net.IsSingleAddr(&addr);
+        auto& old_ban_entry = is_single_addr ? m_banned_addrs[*addr] : m_banned_subnets[sub_net];
+        if (old_ban_entry.banReason == BanReasonManuallyAdded && ban_reason != BanReasonManuallyAdded) return;
+        const bool ban_reason_upgrade = (old_ban_entry.banReason == BanReasonNodeMisbehaving && ban_reason != BanReasonNodeMisbehaving);
+        if (old_ban_entry.nBanUntil < ban_entry.nBanUntil || ban_reason_upgrade) {
+            if (m_misbehaving_addrs.capacity()) {
+                // we have a limit on misbehaving entries
+                if (old_ban_entry.nBanUntil) {
+                    // overwriting a prior ban
+                    if (ban_reason_upgrade) {
+                        // overwriting a misbehaving entry with manually-added
+                        // ensure we won't remove a manual ban later
+                        assert(is_single_addr);
+                        m_misbehaving_addrs.erase(std::find(m_misbehaving_addrs.begin(), m_misbehaving_addrs.end(), *addr));
+                    }
+                } else if (ban_reason == BanReasonNodeMisbehaving) {
+                    // completely new misbehaving entry
+                    assert(is_single_addr);
+                    if (m_misbehaving_addrs.full()) {
+                        auto old_misbehaving = m_misbehaving_addrs.front();
+                        CSubNet old_misbehaving_sub_net(old_misbehaving);
+                        LogPrint(BCLog::NET, "%s: Removed banned node ip/subnet from banlist.dat: %s\n", __func__, old_misbehaving_sub_net.ToString() + " (misbehaving ban overflow)");
+                        m_banned_addrs.erase(old_misbehaving);
+                        // push_back will overwrite
+                    }
+                    m_misbehaving_addrs.push_back(*addr);
+                }
+            }
+            old_ban_entry = ban_entry;
             m_is_dirty = true;
         } else
             return;
@@ -160,7 +221,17 @@ bool BanMan::Unban(const CSubNet& sub_net)
 {
     {
         LOCK(m_cs_banned);
-        if (m_banned.erase(sub_net) == 0) return false;
+        const CNetAddr *addr;
+        if (sub_net.IsSingleAddr(&addr)) {
+            auto it = m_banned_addrs.find(*addr);
+            if (it == m_banned_addrs.end()) return false;
+            if (it->second.banReason == BanReasonNodeMisbehaving) {
+                m_misbehaving_addrs.erase(std::find(m_misbehaving_addrs.begin(), m_misbehaving_addrs.end(), *addr));
+            }
+            m_banned_addrs.erase(it);
+        } else {
+            if (m_banned_subnets.erase(sub_net) == 0) return false;
+        }
         m_is_dirty = true;
     }
     if (m_client_interface) m_client_interface->BannedListChanged();
@@ -173,13 +244,27 @@ void BanMan::GetBanned(banmap_t& banmap)
     LOCK(m_cs_banned);
     // Sweep the banlist so expired bans are not returned
     SweepBanned();
-    banmap = m_banned; //create a thread safe copy
+    banmap = m_banned_subnets; //create a thread safe copy
+    for (const auto& addr_pair : m_banned_addrs) {
+        banmap[CSubNet(addr_pair.first)] = addr_pair.second;
+    }
 }
 
 void BanMan::SetBanned(const banmap_t& banmap)
 {
     LOCK(m_cs_banned);
-    m_banned = banmap;
+    m_banned_addrs.clear();
+    m_banned_subnets.clear();
+    const CNetAddr* addr;
+    for (const auto& sub_net_pair : banmap) {
+        const auto& sub_net = sub_net_pair.first;
+        const auto& ban_entry = sub_net_pair.second;
+        if (sub_net.IsSingleAddr(&addr)) {
+            m_banned_addrs[*addr] = ban_entry;
+        } else {
+            m_banned_subnets[sub_net] = ban_entry;
+        }
+    }
     m_is_dirty = true;
 }
 
@@ -189,18 +274,31 @@ void BanMan::SweepBanned()
     bool notify_ui = false;
     {
         LOCK(m_cs_banned);
-        banmap_t::iterator it = m_banned.begin();
-        while (it != m_banned.end()) {
+        banmap_t::iterator it = m_banned_subnets.begin();
+        while (it != m_banned_subnets.end()) {
             CSubNet sub_net = (*it).first;
             CBanEntry ban_entry = (*it).second;
             if (now > ban_entry.nBanUntil) {
-                m_banned.erase(it++);
+                m_banned_subnets.erase(it++);
                 m_is_dirty = true;
                 notify_ui = true;
                 LogPrint(BCLog::NET, "%s: Removed banned node ip/subnet from banlist.dat: %s\n", __func__, sub_net.ToString());
             } else
                 ++it;
         }
+        for (auto i = m_banned_addrs.begin(); i != m_banned_addrs.end(); ) {
+            CNetAddr addr = i->first;
+            CBanEntry ban_entry = i->second;
+            if (now > ban_entry.nBanUntil) {
+                m_banned_addrs.erase(i++);
+                m_is_dirty = true;
+                notify_ui = true;
+                CSubNet sub_net(addr);
+                LogPrint(BCLog::NET, "%s: Removed banned node ip/subnet from banlist.dat: %s\n", __func__, sub_net.ToString());
+            } else {
+                ++i;
+            }
+        }
     }
     // update UI
     if (notify_ui && m_client_interface) {
@@ -216,6 +314,6 @@ bool BanMan::BannedSetIsDirty()
 
 void BanMan::SetBannedSetDirty(bool dirty)
 {
-    LOCK(m_cs_banned); //reuse m_banned lock for the m_is_dirty flag
+    LOCK(m_cs_banned);
     m_is_dirty = dirty;
 }
diff --git a/src/banman.h b/src/banman.h
@@ -14,8 +14,13 @@
 #include <cstdint>
 #include <memory>
 
+#include <boost/circular_buffer.hpp>
+#include <boost/circular_buffer/space_optimized.hpp>
+
 // NOTE: When adjusting this, update rpcnet:setban's help ("24h")
 static constexpr unsigned int DEFAULT_MISBEHAVING_BANTIME = 60 * 60 * 24; // Default 24-hour ban
+// Maximum number of misbehaving nodes to keep track of for deprioritisation
+static constexpr size_t DEFAULT_MISBEHAVING_LIMIT = 50000;
 // How often to dump addresses to banlist.dat
 static constexpr std::chrono::minutes DUMP_BANS_INTERVAL{15};
 
@@ -43,6 +48,7 @@ class BanMan
 public:
     ~BanMan();
     BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t default_ban_time);
+    void SetMisbehavingLimit(size_t limit);
     void Ban(const CNetAddr& net_addr, const BanReason& ban_reason, int64_t ban_time_offset = 0, bool since_unix_epoch = false);
     void Ban(const CSubNet& sub_net, const BanReason& ban_reason, int64_t ban_time_offset = 0, bool since_unix_epoch = false);
     void ClearBanned();
@@ -63,7 +69,9 @@ class BanMan
     void SweepBanned();
 
     RecursiveMutex m_cs_banned;
-    banmap_t m_banned GUARDED_BY(m_cs_banned);
+    std::map<CNetAddr, CBanEntry> m_banned_addrs GUARDED_BY(m_cs_banned);
+    boost::circular_buffer_space_optimized<CNetAddr> m_misbehaving_addrs GUARDED_BY(m_cs_banned);
+    banmap_t m_banned_subnets GUARDED_BY(m_cs_banned);
     bool m_is_dirty GUARDED_BY(m_cs_banned);
     CClientUIInterface* m_client_interface = nullptr;
     CBanDB m_ban_db;

diff --git a/src/init.cpp b/src/init.cpp
@@ -438,6 +438,7 @@ void SetupServerArgs()
     gArgs.AddArg("-maxsendbuffer=<n>", strprintf("Maximum per-connection send buffer, <n>*1000 bytes (default: %u)", DEFAULT_MAXSENDBUFFER), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     gArgs.AddArg("-maxtimeadjustment", strprintf("Maximum allowed median peer time offset adjustment. Local perspective of time may be influenced by peers forward or backward by this amount. (default: %u seconds)", DEFAULT_MAX_TIME_ADJUSTMENT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     gArgs.AddArg("-maxuploadtarget=<n>", strprintf("Tries to keep outbound traffic under the given target (in MiB per 24h), 0 = no limit (default: %d)", DEFAULT_MAX_UPLOAD_TARGET), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
+    gArgs.AddArg("-misbehavinglimit=<n>", strprintf("Maximum number of misbehaving peers to deprioritise (default: %s)", DEFAULT_MISBEHAVING_LIMIT), ArgsManager::ALLOW_INT, OptionsCategory::CONNECTION);
     gArgs.AddArg("-onion=<ip:port>", "Use separate SOCKS5 proxy to reach peers via Tor hidden services, set -noonion to disable (default: -proxy)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     gArgs.AddArg("-onlynet=<net>", "Make outgoing connections only through network <net> (ipv4, ipv6 or onion). Incoming connections are not affected by this option. This option can be specified multiple times to allow multiple networks.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     gArgs.AddArg("-peerbloomfilters", strprintf("Support filtering of blocks and transaction with bloom filters (default: %u)", DEFAULT_PEERBLOOMFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
@@ -1357,6 +1358,7 @@ bool AppInitMain(NodeContext& node)
 
     assert(!node.banman);
     node.banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", &uiInterface, gArgs.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME));
+    node.banman->SetMisbehavingLimit(gArgs.GetArg("-misbehavinglimit", DEFAULT_MISBEHAVING_LIMIT));
     assert(!node.connman);
     node.connman = std::unique_ptr<CConnman>(new CConnman(GetRand(std::numeric_limits<uint64_t>::max()), GetRand(std::numeric_limits<uint64_t>::max())));
     // Make mempool generally available in the node context. For example the connection manager, wallet, or RPC threads,

diff --git a/src/netaddress.h b/src/netaddress.h
@@ -131,6 +131,13 @@ class CSubNet
 
         std::string ToString() const;
         bool IsValid() const;
+        bool IsSingleAddr(const CNetAddr** const out_addr) const {
+            for (int i = 0; i < 16; ++i) {
+                if (netmask[i] != 0xff) return false;
+            }
+            *out_addr = &network;
+            return true;
+        }
 
         friend bool operator==(const CSubNet& a, const CSubNet& b);
         friend bool operator!=(const CSubNet& a, const CSubNet& b) { return !(a == b); }

diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp
@@ -356,6 +356,57 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
     peerLogic->FinalizeNode(dummyNode.GetId(), dummy);
 }
 
+BOOST_AUTO_TEST_CASE(DoS_misbehavinglimit)
+{
+    auto banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
+
+    banman->SetMisbehavingLimit(2);
+    banman->ClearBanned();
+
+    int64_t nStartTime = GetTime();
+    SetMockTime(nStartTime); // Overrides future calls to GetTime()
+
+    std::vector<CAddress> addrs;
+    for (int i = 0; i < 4; ++i) {
+        addrs.emplace_back(ip(0xa0b0c000 + i), NODE_NONE);
+        BOOST_CHECK(!banman->IsBanned(addrs[i]));
+    }
+    banman->Ban(addrs[0], BanReasonNodeMisbehaving);
+    BOOST_CHECK(banman->IsBanned(addrs[0]));
+    banman->Ban(addrs[1], BanReasonNodeMisbehaving);
+    BOOST_CHECK(banman->IsBanned(addrs[1]));
+
+    // Should overflow misbehaving limit and evict addrs[0]
+    banman->Ban(addrs[2], BanReasonNodeMisbehaving);
+    BOOST_CHECK(banman->IsBanned(addrs[2]));
+    BOOST_CHECK(!banman->IsBanned(addrs[0]));
+
+    // Manually added shouldn't affect misbehaving limit
+    banman->Ban(addrs[0], BanReasonManuallyAdded);
+    BOOST_CHECK(banman->IsBanned(addrs[0]));
+    BOOST_CHECK(banman->IsBanned(addrs[1]));
+    BOOST_CHECK(banman->IsBanned(addrs[2]));
+
+    // Should overflow misbehaving limit and evict addrs[1]
+    banman->Ban(addrs[3], BanReasonNodeMisbehaving);
+    BOOST_CHECK(banman->IsBanned(addrs[0]));
+    BOOST_CHECK(!banman->IsBanned(addrs[1]));
+    BOOST_CHECK(banman->IsBanned(addrs[2]));
+    BOOST_CHECK(banman->IsBanned(addrs[3]));
+
+    // Unbanning addrs[3] should allow re-adding addrs[1] without overflowing
+    banman->Unban(addrs[3]);
+    BOOST_CHECK(banman->IsBanned(addrs[0]));
+    BOOST_CHECK(!banman->IsBanned(addrs[1]));
+    BOOST_CHECK(banman->IsBanned(addrs[2]));
+    BOOST_CHECK(!banman->IsBanned(addrs[3]));
+    banman->Ban(addrs[1], BanReasonNodeMisbehaving);
+    BOOST_CHECK(banman->IsBanned(addrs[0]));
+    BOOST_CHECK(banman->IsBanned(addrs[1]));
+    BOOST_CHECK(banman->IsBanned(addrs[2]));
+    BOOST_CHECK(!banman->IsBanned(addrs[3]));
+}
+
 static CTransactionRef RandomOrphan()
 {
     std::map<uint256, COrphanTx>::iterator it;

diff --git a/test/lint/lint-includes.sh b/test/lint/lint-includes.sh
@@ -53,6 +53,8 @@ EXPECTED_BOOST_INCLUDES=(
     boost/algorithm/string/classification.hpp
     boost/algorithm/string/replace.hpp
     boost/algorithm/string/split.hpp
+    boost/circular_buffer.hpp
+    boost/circular_buffer/space_optimized.hpp
     boost/date_time/posix_time/posix_time.hpp
     boost/filesystem.hpp
     boost/filesystem/fstream.hpp