net: correctly ban before the handshake is complete

7a8c251901 made a change to avoid getting into SendMessages() until the
version handshake (VERSION + VERACK) is complete. That was done to avoid
leaking out messages to nodes who could connect, but never bothered sending
us their version/verack.

Unfortunately, the ban tally and possible disconnect are done as part of
SendMessages(). So after 7a8c251901, if a peer managed to do something
bannable before completing the handshake (say send 100 non-version messages
before their version), they wouldn't actually end up getting
disconnected/banned. That's fixed here by checking the banscore as part of
ProcessMessages() in addition to SendMessages().
This commit is contained in:
Cory Fields 2017-02-07 12:02:02 -05:00
parent d304fef374
commit c45b9fb54c

View file

@ -2596,6 +2596,36 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
return true; return true;
} }
static bool SendRejectsAndCheckIfBanned(CNode* pnode, CConnman& connman)
{
AssertLockHeld(cs_main);
CNodeState &state = *State(pnode->GetId());
BOOST_FOREACH(const CBlockReject& reject, state.rejects) {
connman.PushMessage(pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::REJECT, (std::string)NetMsgType::BLOCK, reject.chRejectCode, reject.strRejectReason, reject.hashBlock));
}
state.rejects.clear();
if (state.fShouldBan) {
state.fShouldBan = false;
if (pnode->fWhitelisted)
LogPrintf("Warning: not punishing whitelisted peer %s!\n", pnode->addr.ToString());
else if (pnode->fAddnode)
LogPrintf("Warning: not punishing addnoded peer %s!\n", pnode->addr.ToString());
else {
pnode->fDisconnect = true;
if (pnode->addr.IsLocal())
LogPrintf("Warning: not banning local peer %s!\n", pnode->addr.ToString());
else
{
connman.Ban(pnode->addr, BanReasonNodeMisbehaving);
}
}
return true;
}
return false;
}
bool ProcessMessages(CNode* pfrom, CConnman& connman, const std::atomic<bool>& interruptMsgProc) bool ProcessMessages(CNode* pfrom, CConnman& connman, const std::atomic<bool>& interruptMsgProc)
{ {
const CChainParams& chainparams = Params(); const CChainParams& chainparams = Params();
@ -2706,8 +2736,12 @@ bool ProcessMessages(CNode* pfrom, CConnman& connman, const std::atomic<bool>& i
PrintExceptionContinue(NULL, "ProcessMessages()"); PrintExceptionContinue(NULL, "ProcessMessages()");
} }
if (!fRet) if (!fRet) {
LogPrintf("%s(%s, %u bytes) FAILED peer=%d\n", __func__, SanitizeString(strCommand), nMessageSize, pfrom->id); LogPrintf("%s(%s, %u bytes) FAILED peer=%d\n", __func__, SanitizeString(strCommand), nMessageSize, pfrom->id);
}
LOCK(cs_main);
SendRejectsAndCheckIfBanned(pfrom, connman);
return fMoreWork; return fMoreWork;
} }
@ -2773,30 +2807,10 @@ bool SendMessages(CNode* pto, CConnman& connman, const std::atomic<bool>& interr
if (!lockMain) if (!lockMain)
return true; return true;
if (SendRejectsAndCheckIfBanned(pto, connman))
return true;
CNodeState &state = *State(pto->GetId()); CNodeState &state = *State(pto->GetId());
BOOST_FOREACH(const CBlockReject& reject, state.rejects)
connman.PushMessage(pto, msgMaker.Make(NetMsgType::REJECT, (std::string)NetMsgType::BLOCK, reject.chRejectCode, reject.strRejectReason, reject.hashBlock));
state.rejects.clear();
if (state.fShouldBan) {
state.fShouldBan = false;
if (pto->fWhitelisted)
LogPrintf("Warning: not punishing whitelisted peer %s!\n", pto->addr.ToString());
else if (pto->fAddnode)
LogPrintf("Warning: not punishing addnoded peer %s!\n", pto->addr.ToString());
else {
pto->fDisconnect = true;
if (pto->addr.IsLocal())
LogPrintf("Warning: not banning local peer %s!\n", pto->addr.ToString());
else
{
connman.Ban(pto->addr, BanReasonNodeMisbehaving);
}
return true;
}
}
// Address refresh broadcast // Address refresh broadcast
int64_t nNow = GetTimeMicros(); int64_t nNow = GetTimeMicros();
if (!IsInitialBlockDownload() && pto->nNextLocalAddrSend < nNow) { if (!IsInitialBlockDownload() && pto->nNextLocalAddrSend < nNow) {