More comments on the design of AttemptToEvictConnection.
Some developers clearly don't get this and have been posting "improvements" that create clear vulnerabilities. It should have been better explained in the code, since the design is somewhat subtle and getting it right is important.
This commit is contained in:
parent
20f9ecd343
commit
d90351f050
1 changed files with 11 additions and 2 deletions
13
src/net.cpp
13
src/net.cpp
|
@ -877,6 +877,14 @@ public:
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
|
/** Try to find a connection to evict when the node is full.
|
||||||
|
* Extreme care must be taken to avoid opening the node to attacker
|
||||||
|
* triggered network partitioning.
|
||||||
|
* The strategy used here is to protect a small number of peers
|
||||||
|
* for each of several distinct characteristics which are difficult
|
||||||
|
* to forge. In order to partition a node the attacker must be
|
||||||
|
* simultaneously better at all of them than honest peers.
|
||||||
|
*/
|
||||||
static bool AttemptToEvictConnection(bool fPreferNewConnection) {
|
static bool AttemptToEvictConnection(bool fPreferNewConnection) {
|
||||||
std::vector<CNodeRef> vEvictionCandidates;
|
std::vector<CNodeRef> vEvictionCandidates;
|
||||||
{
|
{
|
||||||
|
@ -905,7 +913,7 @@ static bool AttemptToEvictConnection(bool fPreferNewConnection) {
|
||||||
|
|
||||||
if (vEvictionCandidates.empty()) return false;
|
if (vEvictionCandidates.empty()) return false;
|
||||||
|
|
||||||
// Protect the 8 nodes with the best ping times.
|
// Protect the 8 nodes with the lowest minimum ping time.
|
||||||
// An attacker cannot manipulate this metric without physically moving nodes closer to the target.
|
// An attacker cannot manipulate this metric without physically moving nodes closer to the target.
|
||||||
std::sort(vEvictionCandidates.begin(), vEvictionCandidates.end(), ReverseCompareNodeMinPingTime);
|
std::sort(vEvictionCandidates.begin(), vEvictionCandidates.end(), ReverseCompareNodeMinPingTime);
|
||||||
vEvictionCandidates.erase(vEvictionCandidates.end() - std::min(8, static_cast<int>(vEvictionCandidates.size())), vEvictionCandidates.end());
|
vEvictionCandidates.erase(vEvictionCandidates.end() - std::min(8, static_cast<int>(vEvictionCandidates.size())), vEvictionCandidates.end());
|
||||||
|
@ -913,7 +921,7 @@ static bool AttemptToEvictConnection(bool fPreferNewConnection) {
|
||||||
if (vEvictionCandidates.empty()) return false;
|
if (vEvictionCandidates.empty()) return false;
|
||||||
|
|
||||||
// Protect the half of the remaining nodes which have been connected the longest.
|
// Protect the half of the remaining nodes which have been connected the longest.
|
||||||
// This replicates the existing implicit behavior.
|
// This replicates the non-eviction implicit behavior, and precludes attacks that start later.
|
||||||
std::sort(vEvictionCandidates.begin(), vEvictionCandidates.end(), ReverseCompareNodeTimeConnected);
|
std::sort(vEvictionCandidates.begin(), vEvictionCandidates.end(), ReverseCompareNodeTimeConnected);
|
||||||
vEvictionCandidates.erase(vEvictionCandidates.end() - static_cast<int>(vEvictionCandidates.size() / 2), vEvictionCandidates.end());
|
vEvictionCandidates.erase(vEvictionCandidates.end() - static_cast<int>(vEvictionCandidates.size() / 2), vEvictionCandidates.end());
|
||||||
|
|
||||||
|
@ -941,6 +949,7 @@ static bool AttemptToEvictConnection(bool fPreferNewConnection) {
|
||||||
vEvictionCandidates = mapAddrCounts[naMostConnections];
|
vEvictionCandidates = mapAddrCounts[naMostConnections];
|
||||||
|
|
||||||
// Do not disconnect peers if there is only one unprotected connection from their network group.
|
// Do not disconnect peers if there is only one unprotected connection from their network group.
|
||||||
|
// This step excessively favors netgroup diversity, and should be removed once more protective criteria are established.
|
||||||
if (vEvictionCandidates.size() <= 1)
|
if (vEvictionCandidates.size() <= 1)
|
||||||
// unless we prefer the new connection (for whitelisted peers)
|
// unless we prefer the new connection (for whitelisted peers)
|
||||||
if (!fPreferNewConnection)
|
if (!fPreferNewConnection)
|
||||||
|
|
Loading…
Reference in a new issue