Connection slot exhaustion DoS mitigation #6374
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -81,7 +81,6 @@ uint64_t nLocalHostNonce = 0; | |
| static std::vector<ListenSocket> vhListenSocket; | ||
| CAddrMan addrman; | ||
| int nMaxConnections = DEFAULT_MAX_PEER_CONNECTIONS; | ||
| bool fAddressesInitialized = false; | ||
| std::string strSubVersion; | ||
|
|
||
|
|
@@ -776,6 +775,222 @@ void SocketSendData(CNode *pnode) | |
|
|
||
| static list<CNode*> vNodesDisconnected; | ||
|
|
||
| class CNodeRef { | ||
| public: | ||
| CNodeRef(CNode *pnode) : _pnode(pnode) { | ||
| LOCK(cs_vNodes); | ||
| _pnode->AddRef(); | ||
| } | ||
|
|
||
| ~CNodeRef() { | ||
| LOCK(cs_vNodes); | ||
| _pnode->Release(); | ||
| } | ||
|
|
||
| CNode& operator *() const {return *_pnode;}; | ||
| CNode* operator ->() const {return _pnode;}; | ||
|
Member
There was a problem hiding this comment. This class needs an explicit copy constructor and assignment operator to be safe to use inside STL containers: CNodeRef& operator =(const CNodeRef& other)
{
if (this != &other) {
_pnode->Release();
_pnode = other._pnode;
_pnode->AddRef();
}
return *this;
}
CNodeRef(const CNodeRef& other):
_pnode(other._pnode)
{
_pnode->AddRef();
}Otherwise, copying it won't properly update reference counts. |
||
|
|
||
| CNodeRef& operator =(const CNodeRef& other) | ||
| { | ||
| if (this != &other) { | ||
| LOCK(cs_vNodes); | ||
|
|
||
| _pnode->Release(); | ||
| _pnode = other._pnode; | ||
| _pnode->AddRef(); | ||
| } | ||
| return *this; | ||
| } | ||
|
|
||
| CNodeRef(const CNodeRef& other): | ||
| _pnode(other._pnode) | ||
| { | ||
| LOCK(cs_vNodes); | ||
| _pnode->AddRef(); | ||
| } | ||
| private: | ||
| CNode *_pnode; | ||
| }; | ||
|
|
||
| static bool ReverseCompareNodeMinPingTime(const CNodeRef &a, const CNodeRef &b) | ||
| { | ||
| return a->nMinPingUsecTime > b->nMinPingUsecTime; | ||
| } | ||
|
|
||
| static bool ReverseCompareNodeTimeConnected(const CNodeRef &a, const CNodeRef &b) | ||
| { | ||
| return a->nTimeConnected > b->nTimeConnected; | ||
| } | ||
|
|
||
| class CompareNetGroupKeyed | ||
| { | ||
| std::vector<unsigned char> vchSecretKey; | ||
| public: | ||
| CompareNetGroupKeyed() | ||
| { | ||
| vchSecretKey.resize(32, 0); | ||
| GetRandBytes(vchSecretKey.data(), vchSecretKey.size()); | ||
| } | ||
|
|
||
| bool operator()(const CNodeRef &a, const CNodeRef &b) | ||
| { | ||
| std::vector<unsigned char> vchGroupA, vchGroupB; | ||
| CSHA256 hashA, hashB; | ||
| std::vector<unsigned char> vchA(32), vchB(32); | ||
|
|
||
| vchGroupA = a->addr.GetGroup(); | ||
| vchGroupB = b->addr.GetGroup(); | ||
|
|
||
| hashA.Write(begin_ptr(vchGroupA), vchGroupA.size()); | ||
| hashB.Write(begin_ptr(vchGroupB), vchGroupB.size()); | ||
|
|
||
| hashA.Write(begin_ptr(vchSecretKey), vchSecretKey.size()); | ||
| hashB.Write(begin_ptr(vchSecretKey), vchSecretKey.size()); | ||
|
|
||
| hashA.Finalize(begin_ptr(vchA)); | ||
| hashB.Finalize(begin_ptr(vchB)); | ||
|
|
||
| return vchA < vchB; | ||
| } | ||
| }; | ||
|
|
||
| static bool AttemptToEvictConnection(bool fPreferNewConnection) { | ||
| std::vector<CNodeRef> vEvictionCandidates; | ||
| { | ||
| LOCK(cs_vNodes); | ||
|
|
||
| BOOST_FOREACH(CNode *node, vNodes) { | ||
| if (node->fWhitelisted) | ||
| continue; | ||
| if (!node->fInbound) | ||
| continue; | ||
| if (node->fDisconnect) | ||
| continue; | ||
| if (node->addr.IsLocal()) | ||
| continue; | ||
| vEvictionCandidates.push_back(CNodeRef(node)); | ||
| } | ||
| } | ||
|
|
||
| if (vEvictionCandidates.empty()) return false; | ||
|
|
||
| // Protect connections with certain characteristics | ||
|
|
||
| // Deterministically select 4 peers to protect by netgroup. | ||
| // An attacker cannot predict which netgroups will be protected. | ||
| static CompareNetGroupKeyed comparerNetGroupKeyed; | ||
| std::sort(vEvictionCandidates.begin(), vEvictionCandidates.end(), comparerNetGroupKeyed); | ||
| vEvictionCandidates.erase(vEvictionCandidates.end() - std::min(4, static_cast<int>(vEvictionCandidates.size())), vEvictionCandidates.end()); | ||
|
|
||
| if (vEvictionCandidates.empty()) return false; | ||
|
|
||
| // Protect the 8 nodes with the best ping times. | ||
| // An attacker cannot manipulate this metric without physically moving nodes closer to the target. | ||
| std::sort(vEvictionCandidates.begin(), vEvictionCandidates.end(), ReverseCompareNodeMinPingTime); | ||
| vEvictionCandidates.erase(vEvictionCandidates.end() - std::min(8, static_cast<int>(vEvictionCandidates.size())), vEvictionCandidates.end()); | ||
|
|
||
| if (vEvictionCandidates.empty()) return false; | ||
|
|
||
| // Protect the half of the remaining nodes which have been connected the longest. | ||
| // This replicates the existing implicit behavior. | ||
| std::sort(vEvictionCandidates.begin(), vEvictionCandidates.end(), ReverseCompareNodeTimeConnected); | ||
| vEvictionCandidates.erase(vEvictionCandidates.end() - static_cast<int>(vEvictionCandidates.size() / 2), vEvictionCandidates.end()); | ||
|
|
||
| if (vEvictionCandidates.empty()) return false; | ||
|
|
||
| // Identify the network group with the most connections | ||
| std::vector<unsigned char> naMostConnections; | ||
| unsigned int nMostConnections = 0; | ||
| std::map<std::vector<unsigned char>, std::vector<CNodeRef> > mapAddrCounts; | ||
| BOOST_FOREACH(const CNodeRef &node, vEvictionCandidates) { | ||
| mapAddrCounts[node->addr.GetGroup()].push_back(node); | ||
|
|
||
| if (mapAddrCounts[node->addr.GetGroup()].size() > nMostConnections) { | ||
| nMostConnections = mapAddrCounts[node->addr.GetGroup()].size(); | ||
| naMostConnections = node->addr.GetGroup(); | ||
| } | ||
| } | ||
|
|
||
| // Reduce to the network group with the most connections | ||
| vEvictionCandidates = mapAddrCounts[naMostConnections]; | ||
|
|
||
| // Do not disconnect peers if there is only 1 connection from their network group | ||
| if (vEvictionCandidates.size() <= 1) | ||
| // unless we prefer the new connection (for whitelisted peers) | ||
| if (!fPreferNewConnection) | ||
| return false; | ||
|
|
||
| // Disconnect the most recent connection from the network group with the most connections | ||
| std::sort(vEvictionCandidates.begin(), vEvictionCandidates.end(), ReverseCompareNodeTimeConnected); | ||
| vEvictionCandidates[0]->fDisconnect = true; | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| static void AcceptConnection(const ListenSocket& hListenSocket) { | ||
| struct sockaddr_storage sockaddr; | ||
| socklen_t len = sizeof(sockaddr); | ||
| SOCKET hSocket = accept(hListenSocket.socket, (struct sockaddr*)&sockaddr, &len); | ||
| CAddress addr; | ||
| int nInbound = 0; | ||
| int nMaxInbound = nMaxConnections - MAX_OUTBOUND_CONNECTIONS; | ||
|
|
||
| if (hSocket != INVALID_SOCKET) | ||
| if (!addr.SetSockAddr((const struct sockaddr*)&sockaddr)) | ||
| LogPrintf("Warning: Unknown socket family\n"); | ||
|
|
||
| bool whitelisted = hListenSocket.whitelisted || CNode::IsWhitelistedRange(addr); | ||
|
There was a problem hiding this comment. You seem to use this bool first at line 916, why not move the check there? At first look I had the impression it belongs to the LOCK block.
Contributor
There was a problem hiding this comment. I also had this impression, though that is a common theme with several of the variables ( edit: Though to be fair, this code is simply copied, so this is not @pstratem's fault.
Author
There was a problem hiding this comment. This is all copied directly from what was there before. I can clean up the logic a bit more later, but for now it's nice that the diff is minimal.
Contributor
There was a problem hiding this comment. Agreed on the clean diff, @pstratem could you put a TODO in there (or make an issue?) though such that this isn't just forgotten after this though? |
||
| { | ||
| LOCK(cs_vNodes); | ||
| BOOST_FOREACH(CNode* pnode, vNodes) | ||
| if (pnode->fInbound) | ||
| nInbound++; | ||
| } | ||
|
|
||
| if (hSocket == INVALID_SOCKET) | ||
|
There was a problem hiding this comment. Is this the check for the accept call? Would be cleaner if it was below the call. You also wouldn't need the
Author
There was a problem hiding this comment. I agree, but I think that's for a separate pull request.
Contributor
There was a problem hiding this comment. Do we file these as issues or
Author
There was a problem hiding this comment. I would say file them as a single issue. something like "clean up logic/code in AcceptConnection" |
||
| { | ||
| int nErr = WSAGetLastError(); | ||
| if (nErr != WSAEWOULDBLOCK) | ||
| LogPrintf("socket error accept failed: %s\n", NetworkErrorString(nErr)); | ||
| return; | ||
| } | ||
|
|
||
| if (!IsSelectableSocket(hSocket)) | ||
| { | ||
| LogPrintf("connection from %s dropped: non-selectable socket\n", addr.ToString()); | ||
| CloseSocket(hSocket); | ||
| return; | ||
| } | ||
|
|
||
| if (CNode::IsBanned(addr) && !whitelisted) | ||
| { | ||
| LogPrintf("connection from %s dropped (banned)\n", addr.ToString()); | ||
| CloseSocket(hSocket); | ||
| return; | ||
| } | ||
|
|
||
| if (nInbound >= nMaxInbound) | ||
| { | ||
| if (!AttemptToEvictConnection(whitelisted)) { | ||
| // No connection to evict, disconnect the new connection | ||
| LogPrint("net", "failed to find an eviction candidate - connection dropped (full)\n"); | ||
| CloseSocket(hSocket); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| CNode* pnode = new CNode(hSocket, addr, "", true); | ||
| pnode->AddRef(); | ||
| pnode->fWhitelisted = whitelisted; | ||
|
|
||
| LogPrint("net", "connection from %s accepted\n", addr.ToString()); | ||
|
|
||
| { | ||
| LOCK(cs_vNodes); | ||
| vNodes.push_back(pnode); | ||
| } | ||
| } | ||
|
|
||
| void ThreadSocketHandler() | ||
| { | ||
| unsigned int nPrevNodeCount = 0; | ||
|
|
@@ -933,64 +1148,7 @@ void ThreadSocketHandler() | |
| { | ||
| if (hListenSocket.socket != INVALID_SOCKET && FD_ISSET(hListenSocket.socket, &fdsetRecv)) | ||
| { | ||
| AcceptConnection(hListenSocket); | ||
| } | ||
| } | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this interresting for
getpeerinfo?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, but again separate pr