From 40fa4e48afd8e06a1e8c0545f5cd137334903ec4 Mon Sep 17 00:00:00 2001 From: Peter John Bushnell Date: Wed, 15 Mar 2023 12:53:36 +0000 Subject: [PATCH] net: rate limit the processing of incoming addr messages (#1818) * Add ADDR rate limiter * Change limit on regtest only * Add flags for MAX_ADDR_RATE_PER_SECOND and MAX_ADDR_PROCESSING_TOKEN_BUCKET in defid * Increase MAX_ADDR_RATE_PER_SECOND_REGTEST * Add GetDoubleArg() to fix the issue * lint: add locale dependancy --------- Co-authored-by: Mihailo Milenkovic --- src/init.cpp | 13 +++++++++ src/net.cpp | 4 +++ src/net.h | 12 +++++++++ src/net_processing.cpp | 42 +++++++++++++++++++++++++++++ src/net_processing.h | 16 +++++++++++ src/rpc/net.cpp | 4 +++ src/util/system.cpp | 8 ++++++ src/util/system.h | 11 +++++++- test/lint/lint-locale-dependence.sh | 1 + 9 files changed, 110 insertions(+), 1 deletion(-) diff --git a/src/init.cpp b/src/init.cpp index 7aa21f25db..b3c49ec789 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -635,6 +635,8 @@ void SetupServerArgs() gArgs.AddArg("-negativeinterest", "(experimental) Track negative interest values", ArgsManager::ALLOW_ANY, OptionsCategory::HIDDEN); gArgs.AddArg("-rpc-governance-accept-neutral", "Allow voting with neutral votes for JellyFish purpose", ArgsManager::ALLOW_ANY, OptionsCategory::HIDDEN); gArgs.AddArg("-dftxworkers=", strprintf("No. of parallel workers associated with the DfTx related work pool. Stock splits, parallel processing of the chain where appropriate, etc use this worker pool (default: %d)", DEFAULT_DFTX_WORKERS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); + gArgs.AddArg("-maxaddrratepersecond=", strprintf("Sets MAX_ADDR_RATE_PER_SECOND limit for ADDR messages(default: %f)", MAX_ADDR_RATE_PER_SECOND), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); + gArgs.AddArg("-maxaddrprocessingtokenbucket=", strprintf("Sets MAX_ADDR_PROCESSING_TOKEN_BUCKET limit for ADDR messages(default: %d)", MAX_ADDR_PROCESSING_TOKEN_BUCKET), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); #if HAVE_DECL_DAEMON gArgs.AddArg("-daemon", "Run in the background as a daemon and accept commands", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); @@ -1248,6 +1250,17 @@ bool AppInitParameterInteraction() return InitError("peertimeout cannot be configured with a negative value."); } + maxAddrRatePerSecond = gArgs.GetDoubleArg("-maxaddrratepersecond", Params().NetworkIDString() == CBaseChainParams::REGTEST ? MAX_ADDR_RATE_PER_SECOND_REGTEST : MAX_ADDR_RATE_PER_SECOND); + std::cout << maxAddrRatePerSecond << std::endl; + if (maxAddrRatePerSecond <= static_cast(0)) { + return InitError("maxaddrratepersecond cannot be configured with a negative value."); + } + + maxAddrProcessingTokenBucket = gArgs.GetArg("-maxaddrprocessingtokenbucket", MAX_ADDR_PROCESSING_TOKEN_BUCKET); + if (maxAddrProcessingTokenBucket <= 0) { + return InitError("maxaddrprocessingtokenbucket cannot be configured with a negative value."); + } + if (gArgs.IsArgSet("-minrelaytxfee")) { CAmount n = 0; if (!ParseMoney(gArgs.GetArg("-minrelaytxfee", ""), n)) { diff --git a/src/net.cpp b/src/net.cpp index 01e5381e87..e6daa84b14 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -533,6 +533,8 @@ void CNode::copyStats(CNodeStats &stats) LOCK(cs_feeFilter); X(minFeeFilter); } + X(nProcessedAddrs); + X(nRatelimitedAddrs); // It is common for nodes with good ping times to suddenly become lagged, // due to a new block arriving or other large transfer. @@ -2648,6 +2650,8 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn } else { LogPrint(BCLog::NET, "Added connection peer=%d\n", id); } + + nAddrTokenTimestamp = GetTimeMicros(); } CNode::~CNode() diff --git a/src/net.h b/src/net.h index 0fa835ccd1..392bf060d5 100644 --- a/src/net.h +++ b/src/net.h @@ -576,6 +576,8 @@ class CNodeStats CAddress addr; // Bind address of our side of the connection CAddress addrBind; + uint64_t nProcessedAddrs; + uint64_t nRatelimitedAddrs; }; @@ -709,6 +711,16 @@ class CNode std::vector vAddrToSend; CRollingBloomFilter addrKnown; bool fGetAddr{false}; + + /** Number of addresses that can be processed from this peer. */ + double nAddrTokenBucket{1}; // initialize to 1 to allow self-announcement + + /** When nAddrTokenBucket was last updated, in microseconds */ + int64_t nAddrTokenTimestamp; + + std::atomic nProcessedAddrs{}; + std::atomic nRatelimitedAddrs{}; + std::set setKnown; int64_t nNextAddrSend GUARDED_BY(cs_sendProcessing){0}; int64_t nNextLocalAddrSend GUARDED_BY(cs_sendProcessing){0}; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 295584cb56..4ff9ccd846 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -83,6 +83,9 @@ static_assert(INBOUND_PEER_TX_DELAY >= MAX_GETDATA_RANDOM_DELAY, /** Limit to avoid sending big packets. Not used in processing incoming GETDATA for compatibility */ static const unsigned int MAX_GETDATA_SZ = 1000; +double maxAddrRatePerSecond; +size_t maxAddrProcessingTokenBucket; + struct COrphanTx { // When modifying, adapt the copy of this definition in tests/DoS_tests. @@ -2138,6 +2141,10 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr { connman->PushMessage(pfrom, CNetMsgMaker(nSendVersion).Make(NetMsgType::GETADDR)); pfrom->fGetAddr = true; + + // When requesting a getaddr, accept an additional MAX_ADDR_TO_SEND addresses in response + // (bypassing the MAX_ADDR_PROCESSING_TOKEN_BUCKET limit). + pfrom->nAddrTokenBucket += MAX_ADDR_TO_SEND; } connman->MarkAddressGood(pfrom->addr); } @@ -2252,11 +2259,40 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr std::vector vAddrOk; int64_t nNow = GetAdjustedTime(); int64_t nSince = nNow - 10 * 60; + + // track rate limiting within this message + uint64_t nProcessedAddrs = 0; + uint64_t nRatelimitedAddrs = 0; + + // Update/increment addr rate limiting bucket. + const uint64_t nCurrentTime = GetTimeMicros(); + if (pfrom->nAddrTokenBucket < maxAddrProcessingTokenBucket) { + const uint64_t nTimeElapsed = std::max(nCurrentTime - pfrom->nAddrTokenTimestamp, uint64_t(0)); + const double nIncrement = nTimeElapsed * maxAddrRatePerSecond / 1e6; + pfrom->nAddrTokenBucket = std::min(pfrom->nAddrTokenBucket + nIncrement, maxAddrProcessingTokenBucket); + } + pfrom->nAddrTokenTimestamp = nCurrentTime; + + // Randomize entries before processing, to prevent an attacker to + // determine which entries will make it through the rate limit + random_shuffle(vAddr.begin(), vAddr.end(), GetRandInt); + for (CAddress& addr : vAddr) { if (interruptMsgProc) return true; + // Apply rate limiting + if (!pfrom->HasPermission(PF_NOBAN)) { + if (pfrom->nAddrTokenBucket < 1.0) { + nRatelimitedAddrs++; + continue; + } + pfrom->nAddrTokenBucket -= 1.0; + } + + nProcessedAddrs++; + // We only bother storing full nodes, though this may include // things which we would not make an outbound connection to, in // part because we may make feeler connections to them. @@ -2277,6 +2313,12 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr if (fReachable) vAddrOk.push_back(addr); } + pfrom->nProcessedAddrs += nProcessedAddrs; + pfrom->nRatelimitedAddrs += nRatelimitedAddrs; + + LogPrint(BCLog::NET, "Received addr: %u addresses (%u processed, %u rate-limited) peer=%d\n", + vAddr.size(), nProcessedAddrs, nRatelimitedAddrs, pfrom->GetId()); + connman->AddNewAddresses(vAddrOk, pfrom->addr, 2 * 60 * 60); if (vAddr.size() < 1000) pfrom->fGetAddr = false; diff --git a/src/net_processing.h b/src/net_processing.h index 9636b9f4d3..dc69b38e0d 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -21,6 +21,22 @@ static const unsigned int DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN = 100; static constexpr bool DEFAULT_ENABLE_BIP61{false}; static const bool DEFAULT_PEERBLOOMFILTERS = false; +/** The maximum rate of address records we're willing to process on average. Can be bypassed using + * the NetPermissionFlags::Addr permission. */ +static constexpr double MAX_ADDR_RATE_PER_SECOND{0.1}; + +/** The maximum rate of address records we're willing to process on average. Can be bypassed using + * the NetPermissionFlags::Addr permission. */ +static constexpr double MAX_ADDR_RATE_PER_SECOND_REGTEST{100000}; + +/** The soft limit of the address processing token bucket (the regular MAX_ADDR_RATE_PER_SECOND + * based increments won't go above this, but the MAX_ADDR_TO_SEND increment following GETADDR + * is exempt from this limit. */ +static constexpr size_t MAX_ADDR_PROCESSING_TOKEN_BUCKET{MAX_ADDR_TO_SEND}; + +extern double maxAddrRatePerSecond; +extern size_t maxAddrProcessingTokenBucket; + class PeerLogicValidation final : public CValidationInterface, public NetEventsInterface { private: CConnman* const connman; diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index a2ff3f4072..81b4e0b488 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -105,6 +105,8 @@ static UniValue getpeerinfo(const JSONRPCRequest& request) " n, (numeric) The heights of blocks we're currently asking from this peer\n" " ...\n" " ],\n" + " \"addr_processed\": n, (numeric) The total number of addresses processed, excluding those dropped due to rate limiting\n" + " \"addr_rate_limited\": n, (numeric) The total number of addresses dropped due to rate limiting\n" " \"whitelisted\": true|false, (boolean) Whether the peer is whitelisted\n" " \"minfeefilter\": n, (numeric) The minimum fee rate for transactions this peer accepts\n" " \"bytessent_per_msg\": {\n" @@ -179,6 +181,8 @@ static UniValue getpeerinfo(const JSONRPCRequest& request) } obj.pushKV("inflight", heights); } + obj.pushKV("addr_processed", stats.nProcessedAddrs); + obj.pushKV("addr_rate_limited", stats.nRatelimitedAddrs); obj.pushKV("whitelisted", stats.m_legacyWhitelisted); UniValue permissions(UniValue::VARR); for (const auto& permission : NetPermissions::ToStrings(stats.m_permissionFlags)) { diff --git a/src/util/system.cpp b/src/util/system.cpp index 072b7738a5..206a0acf2a 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -510,6 +510,14 @@ int64_t ArgsManager::GetArg(const std::string& strArg, int64_t nDefault) const return nDefault; } +double ArgsManager::GetDoubleArg(const std::string& strArg, double fDefault) const +{ + if (IsArgNegated(strArg)) return 0; + std::pair found_res = ArgsManagerHelper::GetArg(*this, strArg); + if (found_res.first) return atof(found_res.second.c_str()); + return fDefault; +} + bool ArgsManager::GetBoolArg(const std::string& strArg, bool fDefault) const { if (IsArgNegated(strArg)) return false; diff --git a/src/util/system.h b/src/util/system.h index f409b953ca..8d52486ccf 100644 --- a/src/util/system.h +++ b/src/util/system.h @@ -47,7 +47,7 @@ bool error(const char* fmt, const Args&... args) return false; } -// Adding some generic function pointers to keep things contained without taking +// Adding some generic function pointers to keep things contained without taking // dependencies on HTTPServer on libs that don't need it typedef std::function(const std::string&)> HTTPHeaderQueryFunc; typedef std::function HTTPHeaderWriterFunc; @@ -235,6 +235,15 @@ class ArgsManager */ int64_t GetArg(const std::string& strArg, int64_t nDefault) const; + /** + * Return integer argument or default value + * + * @param strArg Argument to get (e.g. "-foo") + * @param fDefault (e.g. 1.0) + * @return command-line argument (0.0 if invalid number) or default value + */ + double GetDoubleArg(const std::string& strArg, double fDefault) const; + /** * Return boolean argument or default value * diff --git a/test/lint/lint-locale-dependence.sh b/test/lint/lint-locale-dependence.sh index 8c39c5a443..95ae6ae44a 100755 --- a/test/lint/lint-locale-dependence.sh +++ b/test/lint/lint-locale-dependence.sh @@ -22,6 +22,7 @@ KNOWN_VIOLATIONS=( "src/util/strencodings.cpp:.*strtoul" "src/util/strencodings.h:.*atoi" "src/util/system.cpp:.*atoi" + "src/util/system.cpp:.*atof" "src/validation.cpp:.*stoi" "src/masternodes/govvariables/attributes.cpp:.*isspace" "src/masternodes/validation.cpp:.*stoi"