From c45b9fb54c5ca068a5e276c3bd6ebf4ae720f6f7 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Tue, 7 Feb 2017 12:02:02 -0500 Subject: 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(). --- src/net_processing.cpp | 60 +++++++++++++++++++++++++++++++------------------- 1 file changed, 37 insertions(+), 23 deletions(-) (limited to 'src/net_processing.cpp') diff --git a/src/net_processing.cpp b/src/net_processing.cpp index bb14e69d83..587e857970 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2596,6 +2596,36 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr 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& interruptMsgProc) { const CChainParams& chainparams = Params(); @@ -2706,8 +2736,12 @@ bool ProcessMessages(CNode* pfrom, CConnman& connman, const std::atomic& i PrintExceptionContinue(NULL, "ProcessMessages()"); } - if (!fRet) + if (!fRet) { LogPrintf("%s(%s, %u bytes) FAILED peer=%d\n", __func__, SanitizeString(strCommand), nMessageSize, pfrom->id); + } + + LOCK(cs_main); + SendRejectsAndCheckIfBanned(pfrom, connman); return fMoreWork; } @@ -2773,30 +2807,10 @@ bool SendMessages(CNode* pto, CConnman& connman, const std::atomic& interr if (!lockMain) return true; + if (SendRejectsAndCheckIfBanned(pto, connman)) + return true; 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 int64_t nNow = GetTimeMicros(); if (!IsInitialBlockDownload() && pto->nNextLocalAddrSend < nNow) { -- cgit v1.2.3 From 8502e7acbe0f42fd6e6979681bc9c4610c4fb8cb Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Wed, 8 Feb 2017 01:02:49 -0500 Subject: net: parse reject earlier Prior to this change, all messages were ignored until a VERSION message was received, as well as possibly incurring a ban score. Since REJECT messages can be sent at any time (including as a response to a bad VERSION message), make sure to always parse them. Moving this parsing up keeps it from being caught in the if (pfrom->nVersion == 0) check below. --- src/net_processing.cpp | 50 ++++++++++++++++++++++++-------------------------- 1 file changed, 24 insertions(+), 26 deletions(-) (limited to 'src/net_processing.cpp') diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 587e857970..b304da76c2 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1190,8 +1190,31 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr } } + if (strCommand == NetMsgType::REJECT) + { + if (fDebug) { + try { + std::string strMsg; unsigned char ccode; std::string strReason; + vRecv >> LIMITED_STRING(strMsg, CMessageHeader::COMMAND_SIZE) >> ccode >> LIMITED_STRING(strReason, MAX_REJECT_MESSAGE_LENGTH); + + std::ostringstream ss; + ss << strMsg << " code " << itostr(ccode) << ": " << strReason; - if (strCommand == NetMsgType::VERSION) + if (strMsg == NetMsgType::BLOCK || strMsg == NetMsgType::TX) + { + uint256 hash; + vRecv >> hash; + ss << ": hash " << hash.ToString(); + } + LogPrint("net", "Reject %s\n", SanitizeString(ss.str())); + } catch (const std::ios_base::failure&) { + // Avoid feedback loops by preventing reject messages from triggering a new reject message. + LogPrint("net", "Unparseable reject message received\n"); + } + } + } + + else if (strCommand == NetMsgType::VERSION) { // Each connection can only send one version message if (pfrom->nVersion != 0) @@ -2544,31 +2567,6 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr pfrom->fRelayTxes = true; } - - else if (strCommand == NetMsgType::REJECT) - { - if (fDebug) { - try { - std::string strMsg; unsigned char ccode; std::string strReason; - vRecv >> LIMITED_STRING(strMsg, CMessageHeader::COMMAND_SIZE) >> ccode >> LIMITED_STRING(strReason, MAX_REJECT_MESSAGE_LENGTH); - - std::ostringstream ss; - ss << strMsg << " code " << itostr(ccode) << ": " << strReason; - - if (strMsg == NetMsgType::BLOCK || strMsg == NetMsgType::TX) - { - uint256 hash; - vRecv >> hash; - ss << ": hash " << hash.ToString(); - } - LogPrint("net", "Reject %s\n", SanitizeString(ss.str())); - } catch (const std::ios_base::failure&) { - // Avoid feedback loops by preventing reject messages from triggering a new reject message. - LogPrint("net", "Unparseable reject message received\n"); - } - } - } - else if (strCommand == NetMsgType::FEEFILTER) { CAmount newFeeFilter = 0; vRecv >> newFeeFilter; -- cgit v1.2.3 From cbfc5a6728d389fbb15e0555cdf50f1b04595106 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Wed, 8 Feb 2017 01:04:53 -0500 Subject: net: require a verack before responding to anything else 7a8c251901 made this logic hard to follow. After that change, messages would not be sent to a peer via SendMessages() before the handshake was complete, but messages could still be sent as a response to an incoming message. For example, if a peer had not yet sent a verack, we wouldn't notify it about new blocks, but we would respond to a PING with a PONG. This change makes the behavior straightforward: until we've received a verack, never send any message other than version/verack/reject. The behavior until a VERACK is received has always been undefined, this change just tightens our policy. This also makes testing much easier, because we can now connect but not send version/verack, and anything sent to us is an error. --- src/net_processing.cpp | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 'src/net_processing.cpp') diff --git a/src/net_processing.cpp b/src/net_processing.cpp index b304da76c2..f458a35256 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1420,6 +1420,13 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr pfrom->fSuccessfullyConnected = true; } + else if (!pfrom->fSuccessfullyConnected) + { + // Must have a verack message before anything else + LOCK(cs_main); + Misbehaving(pfrom->GetId(), 1); + return false; + } else if (strCommand == NetMsgType::ADDR) { -- cgit v1.2.3