From e5b26deaaa6842f7dd7c4537ede000f965ea0189 Mon Sep 17 00:00:00 2001 From: "nicolas.dorier" Date: Thu, 20 Jun 2019 18:37:51 +0900 Subject: [PATCH 1/4] Make whitebind/whitelist permissions more flexible --- src/Makefile.am | 2 + src/init.cpp | 20 +++---- src/net.cpp | 59 ++++++++++++++------- src/net.h | 30 +++++++---- src/net_permissions.cpp | 106 +++++++++++++++++++++++++++++++++++++ src/net_permissions.h | 62 ++++++++++++++++++++++ src/rpc/net.cpp | 6 +++ src/test/netbase_tests.cpp | 79 +++++++++++++++++++++++++++ 8 files changed, 321 insertions(+), 43 deletions(-) create mode 100644 src/net_permissions.cpp create mode 100644 src/net_permissions.h diff --git a/src/Makefile.am b/src/Makefile.am index ef5b1900d9d..141d8e68ea2 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -150,6 +150,7 @@ BITCOIN_CORE_H = \ merkleblock.h \ miner.h \ net.h \ + net_permissions.h \ net_processing.h \ netaddress.h \ netbase.h \ @@ -454,6 +455,7 @@ libbitcoin_common_a_SOURCES = \ merkleblock.cpp \ netaddress.cpp \ netbase.cpp \ + net_permissions.cpp \ outputtype.cpp \ policy/feerate.cpp \ policy/policy.cpp \ diff --git a/src/init.cpp b/src/init.cpp index bb3ff8d88f1..25c964205a5 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -1775,21 +1776,16 @@ bool AppInitMain(InitInterfaces& interfaces) connOptions.vBinds.push_back(addrBind); } for (const std::string& strBind : gArgs.GetArgs("-whitebind")) { - CService addrBind; - if (!Lookup(strBind.c_str(), addrBind, 0, false)) { - return InitError(ResolveErrMsg("whitebind", strBind)); - } - if (addrBind.GetPort() == 0) { - return InitError(strprintf(_("Need to specify a port with -whitebind: '%s'").translated, strBind)); - } - connOptions.vWhiteBinds.push_back(addrBind); + NetWhitebindPermissions whitebind; + std::string error; + if (!NetWhitebindPermissions::TryParse(strBind, whitebind, error)) return InitError(error); + connOptions.vWhiteBinds.push_back(whitebind); } for (const auto& net : gArgs.GetArgs("-whitelist")) { - CSubNet subnet; - LookupSubNet(net.c_str(), subnet); - if (!subnet.IsValid()) - return InitError(strprintf(_("Invalid netmask specified in -whitelist: '%s'").translated, net)); + NetWhitelistPermissions subnet; + std::string error; + if (!NetWhitelistPermissions::TryParse(net, subnet, error)) return InitError(error); connOptions.vWhitelistedRange.push_back(subnet); } diff --git a/src/net.cpp b/src/net.cpp index 7d6eb31a7c6..fb04650ed75 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -67,7 +68,6 @@ enum BindFlags { BF_NONE = 0, BF_EXPLICIT = (1U << 0), BF_REPORT_ERROR = (1U << 1), - BF_WHITELIST = (1U << 2), }; // The set of sockets cannot be modified while waiting @@ -459,12 +459,10 @@ void CNode::CloseSocketDisconnect() } } -bool CConnman::IsWhitelistedRange(const CNetAddr &addr) { - for (const CSubNet& subnet : vWhitelistedRange) { - if (subnet.Match(addr)) - return true; +void CConnman::AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNetAddr &addr) const { + for (const auto& subnet : vWhitelistedRange) { + if (subnet.m_subnet.Match(addr)) NetPermissions::AddFlag(flags, subnet.m_flags); } - return false; } std::string CNode::GetAddrName() const { @@ -529,6 +527,7 @@ void CNode::copyStats(CNodeStats &stats) X(nRecvBytes); } X(fWhitelisted); + X(m_permissionFlags); { LOCK(cs_feeFilter); X(minFeeFilter); @@ -904,7 +903,20 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) { } } - bool whitelisted = hListenSocket.whitelisted || IsWhitelistedRange(addr); + NetPermissionFlags permissionFlags = NetPermissionFlags::PF_NONE; + hListenSocket.AddSocketPermissionFlags(permissionFlags); + AddWhitelistPermissionFlags(permissionFlags, addr); + const bool noban = NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN); + bool legacyWhitelisted = false; + if (NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_ISIMPLICIT)) { + NetPermissions::ClearFlag(permissionFlags, PF_ISIMPLICIT); + if (gArgs.GetBoolArg("-whitelistforcerelay", false)) NetPermissions::AddFlag(permissionFlags, PF_FORCERELAY); + if (gArgs.GetBoolArg("-whitelistrelay", false)) NetPermissions::AddFlag(permissionFlags, PF_RELAY); + NetPermissions::AddFlag(permissionFlags, PF_MEMPOOL); + NetPermissions::AddFlag(permissionFlags, PF_NOBAN); + legacyWhitelisted = true; + } + { LOCK(cs_vNodes); for (const CNode* pnode : vNodes) { @@ -941,7 +953,7 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) { // 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 (!whitelisted && bannedlevel > ((nInbound + 1 < nMaxInbound) ? 1 : 0)) + if (!noban && bannedlevel > ((nInbound + 1 < nMaxInbound) ? 1 : 0)) { LogPrint(BCLog::NET, "connection from %s dropped (banned)\n", addr.ToString()); CloseSocket(hSocket); @@ -962,9 +974,15 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) { uint64_t nonce = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(id).Finalize(); CAddress addr_bind = GetBindAddress(hSocket); - CNode* pnode = new CNode(id, nLocalServices, GetBestHeight(), hSocket, addr, CalculateKeyedNetGroup(addr), nonce, addr_bind, "", true); + ServiceFlags nodeServices = nLocalServices; + if (NetPermissions::HasFlag(permissionFlags, PF_BLOOMFILTER)) { + nodeServices = static_cast(nodeServices | NODE_BLOOM); + } + CNode* pnode = new CNode(id, nodeServices, GetBestHeight(), hSocket, addr, CalculateKeyedNetGroup(addr), nonce, addr_bind, "", true); pnode->AddRef(); - pnode->fWhitelisted = whitelisted; + pnode->m_permissionFlags = permissionFlags; + // If this flag is present, the user probably expect that RPC and QT report it as whitelisted (backward compatibility) + pnode->fWhitelisted = legacyWhitelisted; pnode->m_prefer_evict = bannedlevel > 0; m_msgproc->InitializeNode(pnode); @@ -1983,7 +2001,7 @@ void CConnman::ThreadMessageHandler() -bool CConnman::BindListenPort(const CService &addrBind, std::string& strError, bool fWhitelisted) +bool CConnman::BindListenPort(const CService& addrBind, std::string& strError, NetPermissionFlags permissions) { strError = ""; int nOne = 1; @@ -2044,9 +2062,9 @@ bool CConnman::BindListenPort(const CService &addrBind, std::string& strError, b return false; } - vhListenSocket.push_back(ListenSocket(hListenSocket, fWhitelisted)); + vhListenSocket.push_back(ListenSocket(hListenSocket, permissions)); - if (addrBind.IsRoutable() && fDiscover && !fWhitelisted) + if (addrBind.IsRoutable() && fDiscover && (permissions & PF_NOBAN) == 0) AddLocal(addrBind, LOCAL_BIND); return true; @@ -2130,11 +2148,11 @@ NodeId CConnman::GetNewNodeId() } -bool CConnman::Bind(const CService &addr, unsigned int flags) { +bool CConnman::Bind(const CService &addr, unsigned int flags, NetPermissionFlags permissions) { if (!(flags & BF_EXPLICIT) && !IsReachable(addr)) return false; std::string strError; - if (!BindListenPort(addr, strError, (flags & BF_WHITELIST) != 0)) { + if (!BindListenPort(addr, strError, permissions)) { if ((flags & BF_REPORT_ERROR) && clientInterface) { clientInterface->ThreadSafeMessageBox(strError, "", CClientUIInterface::MSG_ERROR); } @@ -2143,20 +2161,21 @@ bool CConnman::Bind(const CService &addr, unsigned int flags) { return true; } -bool CConnman::InitBinds(const std::vector& binds, const std::vector& whiteBinds) { +bool CConnman::InitBinds(const std::vector& binds, const std::vector& whiteBinds) +{ bool fBound = false; for (const auto& addrBind : binds) { - fBound |= Bind(addrBind, (BF_EXPLICIT | BF_REPORT_ERROR)); + fBound |= Bind(addrBind, (BF_EXPLICIT | BF_REPORT_ERROR), NetPermissionFlags::PF_NONE); } for (const auto& addrBind : whiteBinds) { - fBound |= Bind(addrBind, (BF_EXPLICIT | BF_REPORT_ERROR | BF_WHITELIST)); + fBound |= Bind(addrBind.m_service, (BF_EXPLICIT | BF_REPORT_ERROR), addrBind.m_flags); } if (binds.empty() && whiteBinds.empty()) { struct in_addr inaddr_any; inaddr_any.s_addr = INADDR_ANY; struct in6_addr inaddr6_any = IN6ADDR_ANY_INIT; - fBound |= Bind(CService(inaddr6_any, GetListenPort()), BF_NONE); - fBound |= Bind(CService(inaddr_any, GetListenPort()), !fBound ? BF_REPORT_ERROR : BF_NONE); + fBound |= Bind(CService(inaddr6_any, GetListenPort()), BF_NONE, NetPermissionFlags::PF_NONE); + fBound |= Bind(CService(inaddr_any, GetListenPort()), !fBound ? BF_REPORT_ERROR : BF_NONE, NetPermissionFlags::PF_NONE); } return fBound; } diff --git a/src/net.h b/src/net.h index 37aaf1a63bc..8e4521694ec 100644 --- a/src/net.h +++ b/src/net.h @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -138,8 +139,9 @@ public: uint64_t nMaxOutboundLimit = 0; int64_t m_peer_connect_timeout = DEFAULT_PEER_CONNECT_TIMEOUT; std::vector vSeedNodes; - std::vector vWhitelistedRange; - std::vector vBinds, vWhiteBinds; + std::vector vWhitelistedRange; + std::vector vWhiteBinds; + std::vector vBinds; bool m_use_addrman_outgoing = true; std::vector m_specified_outgoing; std::vector m_added_nodes; @@ -314,15 +316,17 @@ public: private: struct ListenSocket { + public: SOCKET socket; - bool whitelisted; - - ListenSocket(SOCKET socket_, bool whitelisted_) : socket(socket_), whitelisted(whitelisted_) {} + inline void AddSocketPermissionFlags(NetPermissionFlags& flags) const { NetPermissions::AddFlag(flags, m_permissions); } + ListenSocket(SOCKET socket_, NetPermissionFlags permissions_) : socket(socket_), m_permissions(permissions_) {} + private: + NetPermissionFlags m_permissions; }; - bool BindListenPort(const CService &bindAddr, std::string& strError, bool fWhitelisted = false); - bool Bind(const CService &addr, unsigned int flags); - bool InitBinds(const std::vector& binds, const std::vector& whiteBinds); + bool BindListenPort(const CService& bindAddr, std::string& strError, NetPermissionFlags permissions); + bool Bind(const CService& addr, unsigned int flags, NetPermissionFlags permissions); + bool InitBinds(const std::vector& binds, const std::vector& whiteBinds); void ThreadOpenAddedConnections(); void AddOneShot(const std::string& strDest); void ProcessOneShot(); @@ -347,7 +351,7 @@ private: bool AttemptToEvictConnection(); CNode* ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, bool manual_connection); - bool IsWhitelistedRange(const CNetAddr &addr); + void AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNetAddr &addr) const; void DeleteNode(CNode* pnode); @@ -380,7 +384,7 @@ private: // Whitelisted ranges. Any node connecting from these is automatically // whitelisted (as well as those connecting to whitelisted binds). - std::vector vWhitelistedRange; + std::vector vWhitelistedRange; unsigned int nSendBufferMaxSize{0}; unsigned int nReceiveFloodSize{0}; @@ -448,7 +452,6 @@ void StartMapPort(); void InterruptMapPort(); void StopMapPort(); unsigned short GetListenPort(); -bool BindListenPort(const CService &bindAddr, std::string& strError, bool fWhitelisted = false); struct CombinerAll { @@ -555,6 +558,7 @@ public: mapMsgCmdSize mapSendBytesPerMsgCmd; uint64_t nRecvBytes; mapMsgCmdSize mapRecvBytesPerMsgCmd; + NetPermissionFlags m_permissionFlags; bool fWhitelisted; double dPingTime; double dPingWait; @@ -657,6 +661,9 @@ public: */ std::string cleanSubVer GUARDED_BY(cs_SubVer){}; bool m_prefer_evict{false}; // This peer is preferred for eviction. + bool HasPermission(NetPermissionFlags permission) const { + return NetPermissions::HasFlag(m_permissionFlags, permission); + } bool fWhitelisted{false}; // This peer can bypass DoS banning. bool fFeeler{false}; // If true this node is being used as a short lived feeler. bool fOneShot{false}; @@ -753,6 +760,7 @@ private: const ServiceFlags nLocalServices; const int nMyStartingHeight; int nSendVersion{0}; + NetPermissionFlags m_permissionFlags{ PF_NONE }; std::list vRecvMsg; // Used only by SocketHandler thread mutable CCriticalSection cs_addrName; diff --git a/src/net_permissions.cpp b/src/net_permissions.cpp new file mode 100644 index 00000000000..736f19293a1 --- /dev/null +++ b/src/net_permissions.cpp @@ -0,0 +1,106 @@ +// Copyright (c) 2009-2018 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include +#include + +// The parse the following format "perm1,perm2@xxxxxx" +bool TryParsePermissionFlags(const std::string str, NetPermissionFlags& output, size_t& readen, std::string& error) +{ + NetPermissionFlags flags = PF_NONE; + const auto atSeparator = str.find('@'); + + // if '@' is not found (ie, "xxxxx"), the caller should apply implicit permissions + if (atSeparator == std::string::npos) { + NetPermissions::AddFlag(flags, PF_ISIMPLICIT); + readen = 0; + } + // else (ie, "perm1,perm2@xxxxx"), let's enumerate the permissions by splitting by ',' and calculate the flags + else { + readen = 0; + // permissions == perm1,perm2 + const auto permissions = str.substr(0, atSeparator); + while (readen < permissions.length()) { + const auto commaSeparator = permissions.find(',', readen); + const auto len = commaSeparator == std::string::npos ? permissions.length() - readen : commaSeparator - readen; + // permission == perm1 + const auto permission = permissions.substr(readen, len); + readen += len; // We read "perm1" + if (commaSeparator != std::string::npos) readen++; // We read "," + + if (permission == "bloomfilter" || permission == "bloom") NetPermissions::AddFlag(flags, PF_BLOOMFILTER); + else if (permission == "noban") NetPermissions::AddFlag(flags, PF_NOBAN); + else if (permission == "forcerelay") NetPermissions::AddFlag(flags, PF_FORCERELAY); + else if (permission == "mempool") NetPermissions::AddFlag(flags, PF_MEMPOOL); + else if (permission == "all") NetPermissions::AddFlag(flags, PF_ALL); + else if (permission == "relay") NetPermissions::AddFlag(flags, PF_RELAY); + else if (permission.length() == 0); // Allow empty entries + else { + error = strprintf(_("Invalid P2P permission: '%s'").translated, permission); + return false; + } + } + readen++; + } + + output = flags; + error = ""; + return true; +} + +std::vector NetPermissions::ToStrings(NetPermissionFlags flags) +{ + std::vector strings; + if (NetPermissions::HasFlag(flags, PF_BLOOMFILTER)) strings.push_back("bloomfilter"); + if (NetPermissions::HasFlag(flags, PF_NOBAN)) strings.push_back("noban"); + if (NetPermissions::HasFlag(flags, PF_FORCERELAY)) strings.push_back("forcerelay"); + if (NetPermissions::HasFlag(flags, PF_RELAY)) strings.push_back("relay"); + if (NetPermissions::HasFlag(flags, PF_MEMPOOL)) strings.push_back("mempool"); + return strings; +} + +bool NetWhitebindPermissions::TryParse(const std::string str, NetWhitebindPermissions& output, std::string& error) +{ + NetPermissionFlags flags; + size_t offset; + if (!TryParsePermissionFlags(str, flags, offset, error)) return false; + + const std::string strBind = str.substr(offset); + CService addrBind; + if (!Lookup(strBind.c_str(), addrBind, 0, false)) { + error = strprintf(_("Cannot resolve -%s address: '%s'").translated, "whitebind", strBind); + return false; + } + if (addrBind.GetPort() == 0) { + error = strprintf(_("Need to specify a port with -whitebind: '%s'").translated, strBind); + return false; + } + + output.m_flags = flags; + output.m_service = addrBind; + error = ""; + return true; +} + +bool NetWhitelistPermissions::TryParse(const std::string str, NetWhitelistPermissions& output, std::string& error) +{ + NetPermissionFlags flags; + size_t offset; + if (!TryParsePermissionFlags(str, flags, offset, error)) return false; + + const std::string net = str.substr(offset); + CSubNet subnet; + LookupSubNet(net.c_str(), subnet); + if (!subnet.IsValid()) { + error = strprintf(_("Invalid netmask specified in -whitelist: '%s'").translated, net); + return false; + } + + output.m_flags = flags; + output.m_subnet = subnet; + error = ""; + return true; +} diff --git a/src/net_permissions.h b/src/net_permissions.h new file mode 100644 index 00000000000..b3987de65f7 --- /dev/null +++ b/src/net_permissions.h @@ -0,0 +1,62 @@ +// Copyright (c) 2009-2018 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include + +#ifndef BITCOIN_NET_PERMISSIONS_H +#define BITCOIN_NET_PERMISSIONS_H +enum NetPermissionFlags +{ + PF_NONE = 0, + // Can query bloomfilter even if -peerbloomfilters is false + PF_BLOOMFILTER = (1U << 1), + // Relay and accept transactions from this peer, even if -blocksonly is true + PF_RELAY = (1U << 3), + // Always relay transactions from this peer, even if already in mempool or rejected from policy + // Keep parameter interaction: forcerelay implies relay + PF_FORCERELAY = (1U << 2) | PF_RELAY, + // Can't be banned for misbehavior + PF_NOBAN = (1U << 4), + // Can query the mempool + PF_MEMPOOL = (1U << 5), + + // True if the user did not specifically set fine grained permissions + PF_ISIMPLICIT = (1U << 31), + PF_ALL = PF_BLOOMFILTER | PF_FORCERELAY | PF_RELAY | PF_NOBAN | PF_MEMPOOL, +}; +class NetPermissions +{ +public: + NetPermissionFlags m_flags; + static std::vector ToStrings(NetPermissionFlags flags); + static inline bool HasFlag(const NetPermissionFlags& flags, NetPermissionFlags f) + { + return (flags & f) == f; + } + static inline void AddFlag(NetPermissionFlags& flags, NetPermissionFlags f) + { + flags = static_cast(flags | f); + } + static inline void ClearFlag(NetPermissionFlags& flags, NetPermissionFlags f) + { + flags = static_cast(flags & ~f); + } +}; +class NetWhitebindPermissions : public NetPermissions +{ +public: + static bool TryParse(const std::string str, NetWhitebindPermissions& output, std::string& error); + CService m_service; +}; + +class NetWhitelistPermissions : public NetPermissions +{ +public: + static bool TryParse(const std::string str, NetWhitelistPermissions& output, std::string& error); + CSubNet m_subnet; +}; + +#endif // BITCOIN_NET_PERMISSIONS_H \ No newline at end of file diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 16b59e3d581..196e69066bc 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -178,6 +179,11 @@ static UniValue getpeerinfo(const JSONRPCRequest& request) obj.pushKV("inflight", heights); } obj.pushKV("whitelisted", stats.fWhitelisted); + UniValue permissions(UniValue::VARR); + for (const auto& permission : NetPermissions::ToStrings(stats.m_permissionFlags)) { + permissions.push_back(permission); + } + obj.pushKV("permissions", permissions); obj.pushKV("minfeefilter", ValueFromAmount(stats.minFeeFilter)); UniValue sendPerMsgCmd(UniValue::VOBJ); diff --git a/src/test/netbase_tests.cpp b/src/test/netbase_tests.cpp index 86c0cecbf14..a3d08316247 100644 --- a/src/test/netbase_tests.cpp +++ b/src/test/netbase_tests.cpp @@ -3,6 +3,7 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#include #include #include @@ -321,4 +322,82 @@ BOOST_AUTO_TEST_CASE(netbase_parsenetwork) BOOST_CHECK_EQUAL(ParseNetwork(""), NET_UNROUTABLE); } +BOOST_AUTO_TEST_CASE(netpermissions_test) +{ + std::string error; + NetWhitebindPermissions whitebindPermissions; + NetWhitelistPermissions whitelistPermissions; + + // Detect invalid white bind + BOOST_CHECK(!NetWhitebindPermissions::TryParse("", whitebindPermissions, error)); + BOOST_CHECK(error.find("Cannot resolve -whitebind address") != std::string::npos); + BOOST_CHECK(!NetWhitebindPermissions::TryParse("127.0.0.1", whitebindPermissions, error)); + BOOST_CHECK(error.find("Need to specify a port with -whitebind") != std::string::npos); + BOOST_CHECK(!NetWhitebindPermissions::TryParse("", whitebindPermissions, error)); + + // If no permission flags, assume backward compatibility + BOOST_CHECK(NetWhitebindPermissions::TryParse("1.2.3.4:32", whitebindPermissions, error)); + BOOST_CHECK(error.empty()); + BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_ISIMPLICIT); + BOOST_CHECK(NetPermissions::HasFlag(whitebindPermissions.m_flags, PF_ISIMPLICIT)); + NetPermissions::ClearFlag(whitebindPermissions.m_flags, PF_ISIMPLICIT); + BOOST_CHECK(!NetPermissions::HasFlag(whitebindPermissions.m_flags, PF_ISIMPLICIT)); + BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_NONE); + NetPermissions::AddFlag(whitebindPermissions.m_flags, PF_ISIMPLICIT); + BOOST_CHECK(NetPermissions::HasFlag(whitebindPermissions.m_flags, PF_ISIMPLICIT)); + + // Can set one permission + BOOST_CHECK(NetWhitebindPermissions::TryParse("bloom@1.2.3.4:32", whitebindPermissions, error)); + BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_BLOOMFILTER); + BOOST_CHECK(NetWhitebindPermissions::TryParse("@1.2.3.4:32", whitebindPermissions, error)); + BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_NONE); + + // Happy path, can parse flags + BOOST_CHECK(NetWhitebindPermissions::TryParse("bloom,forcerelay@1.2.3.4:32", whitebindPermissions, error)); + // forcerelay should also activate the relay permission + BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_BLOOMFILTER | PF_FORCERELAY | PF_RELAY); + BOOST_CHECK(NetWhitebindPermissions::TryParse("bloom,relay,noban@1.2.3.4:32", whitebindPermissions, error)); + BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_BLOOMFILTER | PF_RELAY | PF_NOBAN); + BOOST_CHECK(NetWhitebindPermissions::TryParse("bloom,forcerelay,noban@1.2.3.4:32", whitebindPermissions, error)); + BOOST_CHECK(NetWhitebindPermissions::TryParse("all@1.2.3.4:32", whitebindPermissions, error)); + BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_ALL); + + // Allow dups + BOOST_CHECK(NetWhitebindPermissions::TryParse("bloom,relay,noban,noban@1.2.3.4:32", whitebindPermissions, error)); + BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_BLOOMFILTER | PF_RELAY | PF_NOBAN); + + // Allow empty + BOOST_CHECK(NetWhitebindPermissions::TryParse("bloom,relay,,noban@1.2.3.4:32", whitebindPermissions, error)); + BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_BLOOMFILTER | PF_RELAY | PF_NOBAN); + BOOST_CHECK(NetWhitebindPermissions::TryParse(",@1.2.3.4:32", whitebindPermissions, error)); + BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_NONE); + BOOST_CHECK(NetWhitebindPermissions::TryParse(",,@1.2.3.4:32", whitebindPermissions, error)); + BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_NONE); + + // Detect invalid flag + BOOST_CHECK(!NetWhitebindPermissions::TryParse("bloom,forcerelay,oopsie@1.2.3.4:32", whitebindPermissions, error)); + BOOST_CHECK(error.find("Invalid P2P permission") != std::string::npos); + + // Check whitelist error + BOOST_CHECK(!NetWhitelistPermissions::TryParse("bloom,forcerelay,noban@1.2.3.4:32", whitelistPermissions, error)); + BOOST_CHECK(error.find("Invalid netmask specified in -whitelist") != std::string::npos); + + // Happy path for whitelist parsing + BOOST_CHECK(NetWhitelistPermissions::TryParse("noban@1.2.3.4", whitelistPermissions, error)); + BOOST_CHECK_EQUAL(whitelistPermissions.m_flags, PF_NOBAN); + BOOST_CHECK(NetWhitelistPermissions::TryParse("bloom,forcerelay,noban,relay@1.2.3.4/32", whitelistPermissions, error)); + BOOST_CHECK_EQUAL(whitelistPermissions.m_flags, PF_BLOOMFILTER | PF_FORCERELAY | PF_NOBAN | PF_RELAY); + BOOST_CHECK(error.empty()); + BOOST_CHECK_EQUAL(whitelistPermissions.m_subnet.ToString(), "1.2.3.4/32"); + BOOST_CHECK(NetWhitelistPermissions::TryParse("bloom,forcerelay,noban,relay,mempool@1.2.3.4/32", whitelistPermissions, error)); + + const auto strings = NetPermissions::ToStrings(PF_ALL); + BOOST_CHECK_EQUAL(strings.size(), 5); + BOOST_CHECK(std::find(strings.begin(), strings.end(), "bloomfilter") != strings.end()); + BOOST_CHECK(std::find(strings.begin(), strings.end(), "forcerelay") != strings.end()); + BOOST_CHECK(std::find(strings.begin(), strings.end(), "relay") != strings.end()); + BOOST_CHECK(std::find(strings.begin(), strings.end(), "noban") != strings.end()); + BOOST_CHECK(std::find(strings.begin(), strings.end(), "mempool") != strings.end()); +} + BOOST_AUTO_TEST_SUITE_END() From ecd5cf7ea4c3644a30092100ffc399e30e193275 Mon Sep 17 00:00:00 2001 From: "nicolas.dorier" Date: Fri, 21 Jun 2019 11:31:12 +0900 Subject: [PATCH 2/4] Do not disconnect peer for asking mempool if it has NO_BAN permission --- src/net_processing.cpp | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 5efb4adee60..5edb6ecf9bc 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3012,15 +3012,21 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr if (strCommand == NetMsgType::MEMPOOL) { if (!(pfrom->GetLocalServices() & NODE_BLOOM) && !pfrom->fWhitelisted) { - LogPrint(BCLog::NET, "mempool request with bloom filters disabled, disconnect peer=%d\n", pfrom->GetId()); - pfrom->fDisconnect = true; + if (!pfrom->HasPermission(PF_NOBAN)) + { + LogPrint(BCLog::NET, "mempool request with bloom filters disabled, disconnect peer=%d\n", pfrom->GetId()); + pfrom->fDisconnect = true; + } return true; } if (connman->OutboundTargetReached(false) && !pfrom->fWhitelisted) { - LogPrint(BCLog::NET, "mempool request with bandwidth limit reached, disconnect peer=%d\n", pfrom->GetId()); - pfrom->fDisconnect = true; + if (!pfrom->HasPermission(PF_NOBAN)) + { + LogPrint(BCLog::NET, "mempool request with bandwidth limit reached, disconnect peer=%d\n", pfrom->GetId()); + pfrom->fDisconnect = true; + } return true; } From d541fa391844f658bd7035659b5b16695733dd56 Mon Sep 17 00:00:00 2001 From: "nicolas.dorier" Date: Fri, 21 Jun 2019 11:42:04 +0900 Subject: [PATCH 3/4] Replace the use of fWhitelisted by permission checks --- src/net.cpp | 6 +++--- src/net.h | 5 +++-- src/net_processing.cpp | 26 +++++++++++++------------- src/qt/rpcconsole.cpp | 2 +- src/rpc/net.cpp | 2 +- 5 files changed, 21 insertions(+), 20 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index fb04650ed75..0464a6e9ea7 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -526,7 +526,7 @@ void CNode::copyStats(CNodeStats &stats) X(mapRecvBytesPerMsgCmd); X(nRecvBytes); } - X(fWhitelisted); + X(m_legacyWhitelisted); X(m_permissionFlags); { LOCK(cs_feeFilter); @@ -812,7 +812,7 @@ bool CConnman::AttemptToEvictConnection() LOCK(cs_vNodes); for (const CNode* node : vNodes) { - if (node->fWhitelisted) + if (node->HasPermission(PF_NOBAN)) continue; if (!node->fInbound) continue; @@ -982,7 +982,7 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) { pnode->AddRef(); pnode->m_permissionFlags = permissionFlags; // If this flag is present, the user probably expect that RPC and QT report it as whitelisted (backward compatibility) - pnode->fWhitelisted = legacyWhitelisted; + pnode->m_legacyWhitelisted = legacyWhitelisted; pnode->m_prefer_evict = bannedlevel > 0; m_msgproc->InitializeNode(pnode); diff --git a/src/net.h b/src/net.h index 8e4521694ec..75c05c9cb5d 100644 --- a/src/net.h +++ b/src/net.h @@ -559,7 +559,7 @@ public: uint64_t nRecvBytes; mapMsgCmdSize mapRecvBytesPerMsgCmd; NetPermissionFlags m_permissionFlags; - bool fWhitelisted; + bool m_legacyWhitelisted; double dPingTime; double dPingWait; double dMinPing; @@ -664,7 +664,8 @@ public: bool HasPermission(NetPermissionFlags permission) const { return NetPermissions::HasFlag(m_permissionFlags, permission); } - bool fWhitelisted{false}; // This peer can bypass DoS banning. + // This boolean is unusued in actual processing, only present for backward compatibility at RPC/QT level + bool m_legacyWhitelisted{false}; bool fFeeler{false}; // If true this node is being used as a short lived feeler. bool fOneShot{false}; bool m_manual_connection{false}; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 5edb6ecf9bc..3db460d4445 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -408,7 +408,7 @@ static void UpdatePreferredDownload(CNode* node, CNodeState* state) EXCLUSIVE_LO nPreferredDownload -= state->fPreferredDownload; // Whether this node should be marked as a preferred download node. - state->fPreferredDownload = (!node->fInbound || node->fWhitelisted) && !node->fOneShot && !node->fClient; + state->fPreferredDownload = (!node->fInbound || node->HasPermission(PF_NOBAN)) && !node->fOneShot && !node->fClient; nPreferredDownload += state->fPreferredDownload; } @@ -1398,7 +1398,7 @@ void static ProcessGetBlockData(CNode* pfrom, const CChainParams& chainparams, c const CNetMsgMaker msgMaker(pfrom->GetSendVersion()); // disconnect node in case we have reached the outbound limit for serving historical blocks // never disconnect whitelisted nodes - if (send && connman->OutboundTargetReached(true) && ( ((pindexBestHeader != nullptr) && (pindexBestHeader->GetBlockTime() - pindex->GetBlockTime() > HISTORICAL_BLOCK_AGE)) || inv.type == MSG_FILTERED_BLOCK) && !pfrom->fWhitelisted) + if (send && connman->OutboundTargetReached(true) && ( ((pindexBestHeader != nullptr) && (pindexBestHeader->GetBlockTime() - pindex->GetBlockTime() > HISTORICAL_BLOCK_AGE)) || inv.type == MSG_FILTERED_BLOCK) && !pfrom->HasPermission(PF_NOBAN)) { LogPrint(BCLog::NET, "historical block serving limit reached, disconnect peer=%d\n", pfrom->GetId()); @@ -1407,7 +1407,7 @@ void static ProcessGetBlockData(CNode* pfrom, const CChainParams& chainparams, c send = false; } // Avoid leaking prune-height by never sending blocks below the NODE_NETWORK_LIMITED threshold - if (send && !pfrom->fWhitelisted && ( + if (send && !pfrom->HasPermission(PF_NOBAN) && ( (((pfrom->GetLocalServices() & NODE_NETWORK_LIMITED) == NODE_NETWORK_LIMITED) && ((pfrom->GetLocalServices() & NODE_NETWORK) != NODE_NETWORK) && (::ChainActive().Tip()->nHeight - pindex->nHeight > (int)NODE_NETWORK_LIMITED_MIN_BLOCKS + 2 /* add two blocks buffer extension for possible races */) ) )) { LogPrint(BCLog::NET, "Ignore block request below NODE_NETWORK_LIMITED threshold from peer=%d\n", pfrom->GetId()); @@ -2217,7 +2217,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr bool fBlocksOnly = !g_relay_txes; // Allow whitelisted peers to send data other than blocks in blocks only mode if whitelistrelay is true - if (pfrom->fWhitelisted && gArgs.GetBoolArg("-whitelistrelay", DEFAULT_WHITELISTRELAY)) + if (pfrom->HasPermission(PF_RELAY)) fBlocksOnly = false; LOCK(cs_main); @@ -2412,7 +2412,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr } LOCK(cs_main); - if (::ChainstateActive().IsInitialBlockDownload() && !pfrom->fWhitelisted) { + if (::ChainstateActive().IsInitialBlockDownload() && !pfrom->HasPermission(PF_NOBAN)) { LogPrint(BCLog::NET, "Ignoring getheaders from peer=%d because node is in initial block download\n", pfrom->GetId()); return true; } @@ -2470,7 +2470,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr if (strCommand == NetMsgType::TX) { // Stop processing the transaction early if // We are in blocks only mode and peer is either not whitelisted or whitelistrelay is off - if (!g_relay_txes && (!pfrom->fWhitelisted || !gArgs.GetBoolArg("-whitelistrelay", DEFAULT_WHITELISTRELAY))) + if (!g_relay_txes && !pfrom->HasPermission(PF_RELAY)) { LogPrint(BCLog::NET, "transaction sent in violation of protocol peer=%d\n", pfrom->GetId()); return true; @@ -2565,7 +2565,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr AddToCompactExtraTransactions(ptx); } - if (pfrom->fWhitelisted && gArgs.GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY)) { + if (pfrom->HasPermission(PF_FORCERELAY)) { // Always relay transactions received from whitelisted peers, even // if they were already in the mempool or rejected from it due // to policy, allowing the node to function as a gateway for @@ -3010,7 +3010,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr } if (strCommand == NetMsgType::MEMPOOL) { - if (!(pfrom->GetLocalServices() & NODE_BLOOM) && !pfrom->fWhitelisted) + if (!(pfrom->GetLocalServices() & NODE_BLOOM) && !pfrom->HasPermission(PF_MEMPOOL)) { if (!pfrom->HasPermission(PF_NOBAN)) { @@ -3020,7 +3020,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr return true; } - if (connman->OutboundTargetReached(false) && !pfrom->fWhitelisted) + if (connman->OutboundTargetReached(false) && !pfrom->HasPermission(PF_MEMPOOL)) { if (!pfrom->HasPermission(PF_NOBAN)) { @@ -3222,7 +3222,7 @@ bool PeerLogicValidation::SendRejectsAndCheckIfBanned(CNode* pnode, bool enable_ if (state.fShouldBan) { state.fShouldBan = false; - if (pnode->fWhitelisted) + if (pnode->HasPermission(PF_NOBAN)) LogPrintf("Warning: not punishing whitelisted peer %s!\n", pnode->addr.ToString()); else if (pnode->m_manual_connection) LogPrintf("Warning: not punishing manually-connected peer %s!\n", pnode->addr.ToString()); @@ -3792,7 +3792,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto) pto->vInventoryBlockToSend.clear(); // Check whether periodic sends should happen - bool fSendTrickle = pto->fWhitelisted; + bool fSendTrickle = pto->HasPermission(PF_NOBAN); if (pto->nNextInvSend < nNow) { fSendTrickle = true; if (pto->fInbound) { @@ -3948,7 +3948,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto) // Note: If all our peers are inbound, then we won't // disconnect our sync peer for stalling; we have bigger // problems if we can't get any outbound peers. - if (!pto->fWhitelisted) { + if (!pto->HasPermission(PF_NOBAN)) { LogPrintf("Timeout downloading headers from peer=%d, disconnecting\n", pto->GetId()); pto->fDisconnect = true; return true; @@ -4066,7 +4066,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto) // // We don't want white listed peers to filter txs to us if we have -whitelistforcerelay if (pto->nVersion >= FEEFILTER_VERSION && gArgs.GetBoolArg("-feefilter", DEFAULT_FEEFILTER) && - !(pto->fWhitelisted && gArgs.GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY))) { + !pto->HasPermission(PF_FORCERELAY)) { CAmount currentFilter = mempool.GetMinFee(gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFeePerK(); int64_t timeNow = GetTimeMicros(); if (timeNow > pto->nextSendTimeFeeFilter) { diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp index cdf84eae9a8..19b11ba1cda 100644 --- a/src/qt/rpcconsole.cpp +++ b/src/qt/rpcconsole.cpp @@ -1120,7 +1120,7 @@ void RPCConsole::updateNodeDetail(const CNodeCombinedStats *stats) ui->peerSubversion->setText(QString::fromStdString(stats->nodeStats.cleanSubVer)); ui->peerDirection->setText(stats->nodeStats.fInbound ? tr("Inbound") : tr("Outbound")); ui->peerHeight->setText(QString("%1").arg(QString::number(stats->nodeStats.nStartingHeight))); - ui->peerWhitelisted->setText(stats->nodeStats.fWhitelisted ? tr("Yes") : tr("No")); + ui->peerWhitelisted->setText(stats->nodeStats.m_legacyWhitelisted ? tr("Yes") : tr("No")); // This check fails for example if the lock was busy and // nodeStateStats couldn't be fetched. diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 196e69066bc..25dda924a4c 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -178,7 +178,7 @@ static UniValue getpeerinfo(const JSONRPCRequest& request) } obj.pushKV("inflight", heights); } - obj.pushKV("whitelisted", stats.fWhitelisted); + obj.pushKV("whitelisted", stats.m_legacyWhitelisted); UniValue permissions(UniValue::VARR); for (const auto& permission : NetPermissions::ToStrings(stats.m_permissionFlags)) { permissions.push_back(permission); From c5b404e8f1973afe071a07c63ba1038eefe13f0f Mon Sep 17 00:00:00 2001 From: "nicolas.dorier" Date: Fri, 21 Jun 2019 13:15:26 +0900 Subject: [PATCH 4/4] Add functional tests for flexible whitebind/list --- test/functional/p2p_permissions.py | 97 +++++++++++++++++++++ test/functional/test_framework/test_node.py | 1 + test/functional/test_runner.py | 1 + 3 files changed, 99 insertions(+) create mode 100644 test/functional/p2p_permissions.py diff --git a/test/functional/p2p_permissions.py b/test/functional/p2p_permissions.py new file mode 100644 index 00000000000..10130554204 --- /dev/null +++ b/test/functional/p2p_permissions.py @@ -0,0 +1,97 @@ +#!/usr/bin/env python3 +# Copyright (c) 2015-2018 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +"""Test p2p permission message. + +Test that permissions are correctly calculated and applied +""" + +from test_framework.test_node import ErrorMatch +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import ( + assert_equal, + connect_nodes, + p2p_port, +) + +class P2PPermissionsTests(BitcoinTestFramework): + def set_test_params(self): + self.num_nodes = 2 + self.setup_clean_chain = True + self.extra_args = [[],[]] + + def run_test(self): + self.checkpermission( + # relay permission added + ["-whitelist=127.0.0.1", "-whitelistrelay"], + ["relay", "noban", "mempool"], + True) + + self.checkpermission( + # forcerelay and relay permission added + # Legacy parameter interaction which set whitelistrelay to true + # if whitelistforcerelay is true + ["-whitelist=127.0.0.1", "-whitelistforcerelay"], + ["forcerelay", "relay", "noban", "mempool"], + True) + + # Let's make sure permissions are merged correctly + # For this, we need to use whitebind instead of bind + # by modifying the configuration file. + ip_port = "127.0.0.1:{}".format(p2p_port(1)) + self.replaceinconfig(1, "bind=127.0.0.1", "whitebind=bloomfilter,forcerelay@" + ip_port) + self.checkpermission( + ["-whitelist=noban@127.0.0.1" ], + # Check parameter interaction forcerelay should activate relay + ["noban", "bloomfilter", "forcerelay", "relay" ], + False) + self.replaceinconfig(1, "whitebind=bloomfilter,forcerelay@" + ip_port, "bind=127.0.0.1") + + self.checkpermission( + # legacy whitelistrelay should be ignored + ["-whitelist=noban,mempool@127.0.0.1", "-whitelistrelay"], + ["noban", "mempool"], + False) + + self.checkpermission( + # legacy whitelistforcerelay should be ignored + ["-whitelist=noban,mempool@127.0.0.1", "-whitelistforcerelay"], + ["noban", "mempool"], + False) + + self.checkpermission( + # missing mempool permission to be considered legacy whitelisted + ["-whitelist=noban@127.0.0.1"], + ["noban"], + False) + + self.checkpermission( + # all permission added + ["-whitelist=all@127.0.0.1"], + ["forcerelay", "noban", "mempool", "bloomfilter", "relay"], + False) + + self.stop_node(1) + self.nodes[1].assert_start_raises_init_error(["-whitelist=oopsie@127.0.0.1"], "Invalid P2P permission", match=ErrorMatch.PARTIAL_REGEX) + self.nodes[1].assert_start_raises_init_error(["-whitelist=noban@127.0.0.1:230"], "Invalid netmask specified in", match=ErrorMatch.PARTIAL_REGEX) + self.nodes[1].assert_start_raises_init_error(["-whitebind=noban@127.0.0.1/10"], "Cannot resolve -whitebind address", match=ErrorMatch.PARTIAL_REGEX) + + def checkpermission(self, args, expectedPermissions, whitelisted): + self.restart_node(1, args) + connect_nodes(self.nodes[0], 1) + peerinfo = self.nodes[1].getpeerinfo()[0] + assert_equal(peerinfo['whitelisted'], whitelisted) + assert_equal(len(expectedPermissions), len(peerinfo['permissions'])) + for p in expectedPermissions: + if not p in peerinfo['permissions']: + raise AssertionError("Expected permissions %r is not granted." % p) + + def replaceinconfig(self, nodeid, old, new): + with open(self.nodes[nodeid].bitcoinconf, encoding="utf8") as f: + newText=f.read().replace(old, new) + with open(self.nodes[nodeid].bitcoinconf, 'w', encoding="utf8") as f: + f.write(newText) + +if __name__ == '__main__': + P2PPermissionsTests().main() diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index cac52817640..df027397d27 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -68,6 +68,7 @@ class TestNode(): self.index = i self.datadir = datadir + self.bitcoinconf = os.path.join(self.datadir, "bitcoin.conf") self.stdout_dir = os.path.join(self.datadir, "stdout") self.stderr_dir = os.path.join(self.datadir, "stderr") self.chain = chain diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 79efa6131c1..1742a8e3195 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -200,6 +200,7 @@ BASE_SCRIPTS = [ 'rpc_scantxoutset.py', 'feature_logging.py', 'p2p_node_network_limited.py', + 'p2p_permissions.py', 'feature_blocksdir.py', 'feature_config_args.py', 'rpc_help.py',