From 916af52c0a9f29428feaf5ce76e1595a1352a2a4 Mon Sep 17 00:00:00 2001 From: Ilya Savinov Date: Thu, 13 Jul 2017 12:38:00 +0300 Subject: [PATCH] Fix vulnerability with mapMasternodeOrphanObjects (#1512) * fix vulnerability with mapMasternodeOrphanObjects The vulnerability is that a malicious node can send a lot of NetMsgType::MNGOVERNANCEOBJECT messages which refer to many arbitrary MN's. In this case, mapMasternodeOrphanObjects will grow unrestrictedly. * MN collateral moved to governance-object.cpp; ban score applied to misbehaving nodes * recursive locks removed * check for the mn collateral code segregated to a separate function * CheckCollateral implementation moved to cpp --- src/governance-object.cpp | 16 +++++++++--- src/governance.cpp | 52 +++++++++++++++++++++++++-------------- src/governance.h | 20 +++++++++++---- src/masternode.cpp | 44 +++++++++++++++++++++++++-------- src/masternode.h | 8 ++++++ src/rpc/governance.cpp | 18 +++++++++----- 6 files changed, 115 insertions(+), 43 deletions(-) diff --git a/src/governance-object.cpp b/src/governance-object.cpp index 4224df4925..3331e3a2ee 100644 --- a/src/governance-object.cpp +++ b/src/governance-object.cpp @@ -427,6 +427,7 @@ std::string CGovernanceObject::GetDataAsString() void CGovernanceObject::UpdateLocalValidity() { + LOCK(cs_main); // THIS DOES NOT CHECK COLLATERAL, THIS IS CHECKED UPON ORIGINAL ARRIVAL fCachedLocalValidity = IsValidLocally(strLocalValidityError, false); }; @@ -469,8 +470,17 @@ bool CGovernanceObject::IsValidLocally(std::string& strError, bool& fMissingMast std::string strOutpoint = vinMasternode.prevout.ToStringShort(); masternode_info_t infoMn = mnodeman.GetMasternodeInfo(vinMasternode); if(!infoMn.fInfoValid) { - fMissingMasternode = true; - strError = "Masternode not found: " + strOutpoint; + + CMasternode::CollateralStatus err = CMasternode::CheckCollateral(GetMasternodeVin()); + if (err == CMasternode::COLLATERAL_OK) { + fMissingMasternode = true; + strError = "Masternode not found: " + strOutpoint; + } else if (err == CMasternode::COLLATERAL_UTXO_NOT_FOUND) { + strError = "Failed to find Masternode UTXO, missing masternode=" + GetMasternodeVin().prevout.ToStringShort() + "\n"; + } else if (err == CMasternode::COLLATERAL_INVALID_AMOUNT) { + strError = "Masternode UTXO should have 1000 DASH, missing masternode=" + GetMasternodeVin().prevout.ToStringShort() + "\n"; + } + return false; } @@ -579,7 +589,7 @@ bool CGovernanceObject::IsCollateralValid(std::string& strError, bool& fMissingC // GET CONFIRMATIONS FOR TRANSACTION - LOCK(cs_main); + AssertLockHeld(cs_main); int nConfirmationsIn = GetIXConfirmations(nCollateralHash); if (nBlockHash != uint256()) { BlockMap::iterator mi = mapBlockIndex.find(nBlockHash); diff --git a/src/governance.cpp b/src/governance.cpp index d212dbe46c..9659d6722a 100644 --- a/src/governance.cpp +++ b/src/governance.cpp @@ -206,14 +206,27 @@ void CGovernanceManager::ProcessMessage(CNode* pfrom, std::string& strCommand, C if(!fIsValid) { if(fMasternodeMissing) { - mapMasternodeOrphanObjects.insert(std::make_pair(nHash, object_time_pair_t(govobj, GetAdjustedTime() + GOVERNANCE_ORPHAN_EXPIRATION_TIME))); + + int& count = mapMasternodeOrphanCounter[govobj.GetMasternodeVin().prevout]; + if (count >= 10) { + LogPrint("gobject", "MNGOVERNANCEOBJECT -- Too many orphan objects, missing masternode=%s\n", govobj.GetMasternodeVin().prevout.ToStringShort()); + // ask for this object again in 2 minutes + CInv inv(MSG_GOVERNANCE_OBJECT, govobj.GetHash()); + pfrom->AskFor(inv); + return; + } + + count++; + ExpirationInfo info(pfrom->GetId(), GetAdjustedTime() + GOVERNANCE_ORPHAN_EXPIRATION_TIME); + mapMasternodeOrphanObjects.insert(std::make_pair(nHash, object_info_pair_t(govobj, info))); LogPrintf("MNGOVERNANCEOBJECT -- Missing masternode for: %s, strError = %s\n", strHash, strError); } else if(fMissingConfirmations) { AddPostponedObject(govobj); LogPrintf("MNGOVERNANCEOBJECT -- Not enough fee confirmations for: %s, strError = %s\n", strHash, strError); } else { LogPrintf("MNGOVERNANCEOBJECT -- Governance object is invalid - %s\n", strError); - // TODO: apply node's ban score if object is invalid + // apply node's ban score + Misbehaving(pfrom->GetId(), 20); } return; @@ -1023,31 +1036,32 @@ void CGovernanceManager::CheckMasternodeOrphanObjects() LOCK2(cs_main, cs); int64_t nNow = GetAdjustedTime(); CRateChecksGuard guard(false, *this); - object_time_m_it it = mapMasternodeOrphanObjects.begin(); + object_info_m_it it = mapMasternodeOrphanObjects.begin(); while(it != mapMasternodeOrphanObjects.end()) { - object_time_pair_t& pair = it->second; + object_info_pair_t& pair = it->second; CGovernanceObject& govobj = pair.first; - if(pair.second < nNow) { - mapMasternodeOrphanObjects.erase(it++); - continue; - } + if(pair.second.nExpirationTime >= nNow) { + string strError; + bool fMasternodeMissing = false; + bool fConfirmationsMissing = false; + bool fIsValid = govobj.IsValidLocally(strError, fMasternodeMissing, fConfirmationsMissing, true); - string strError; - bool fMasternodeMissing = false; - bool fConfirmationsMissing = false; - bool fIsValid = govobj.IsValidLocally(strError, fMasternodeMissing, fConfirmationsMissing, true); - if(!fIsValid) { - if(!fMasternodeMissing) { - mapMasternodeOrphanObjects.erase(it++); - } - else { + if(fIsValid) { + AddGovernanceObject(govobj); + } else if(fMasternodeMissing) { ++it; + continue; } - continue; + } else { + // apply node's ban score + Misbehaving(pair.second.idFrom, 20); } - AddGovernanceObject(govobj); + auto it_count = mapMasternodeOrphanCounter.find(govobj.GetMasternodeVin().prevout); + if(--it_count->second == 0) + mapMasternodeOrphanCounter.erase(it_count); + mapMasternodeOrphanObjects.erase(it++); } } diff --git a/src/governance.h b/src/governance.h index 344d125eb9..d06010b4e6 100644 --- a/src/governance.h +++ b/src/governance.h @@ -26,7 +26,14 @@ class CGovernanceVote; extern CGovernanceManager governance; -typedef std::pair object_time_pair_t; +struct ExpirationInfo { + ExpirationInfo(int64_t _nExpirationTime, int _idFrom) : nExpirationTime(_nExpirationTime), idFrom(_idFrom) {} + + int64_t nExpirationTime; + NodeId idFrom; +}; + +typedef std::pair object_info_pair_t; static const int RATE_BUFFER_SIZE = 5; @@ -197,17 +204,19 @@ public: // Types typedef txout_m_t::const_iterator txout_m_cit; + typedef std::map txout_int_m_t; + typedef std::set hash_s_t; typedef hash_s_t::iterator hash_s_it; typedef hash_s_t::const_iterator hash_s_cit; - typedef std::map object_time_m_t; + typedef std::map object_info_m_t; - typedef object_time_m_t::iterator object_time_m_it; + typedef object_info_m_t::iterator object_info_m_it; - typedef object_time_m_t::const_iterator object_time_m_cit; + typedef object_info_m_t::const_iterator object_info_m_cit; typedef std::map hash_time_m_t; @@ -237,7 +246,8 @@ private: // value - expiration time for deleted objects hash_time_m_t mapErasedGovernanceObjects; - object_time_m_t mapMasternodeOrphanObjects; + object_info_m_t mapMasternodeOrphanObjects; + txout_int_m_t mapMasternodeOrphanCounter; object_m_t mapPostponedObjects; hash_s_t setAdditionalRelayObjects; diff --git a/src/masternode.cpp b/src/masternode.cpp index d3611f9ca6..50c899c4ba 100644 --- a/src/masternode.cpp +++ b/src/masternode.cpp @@ -161,6 +161,31 @@ arith_uint256 CMasternode::CalculateScore(const uint256& blockHash) return (hash3 > hash2 ? hash3 - hash2 : hash2 - hash3); } +CMasternode::CollateralStatus CMasternode::CheckCollateral(CTxIn vin) +{ + int nHeight; + return CheckCollateral(vin, nHeight); +} + +CMasternode::CollateralStatus CMasternode::CheckCollateral(CTxIn vin, int& nHeight) +{ + AssertLockHeld(cs_main); + + CCoins coins; + if(!pcoinsTip->GetCoins(vin.prevout.hash, coins) || + (unsigned int)vin.prevout.n>=coins.vout.size() || + coins.vout[vin.prevout.n].IsNull()) { + return COLLATERAL_UTXO_NOT_FOUND; + } + + if(coins.vout[vin.prevout.n].nValue != 1000 * COIN) { + return COLLATERAL_INVALID_AMOUNT; + } + + nHeight = coins.nHeight; + return COLLATERAL_OK; +} + void CMasternode::Check(bool fForce) { LOCK(cs); @@ -180,10 +205,8 @@ void CMasternode::Check(bool fForce) TRY_LOCK(cs_main, lockMain); if(!lockMain) return; - CCoins coins; - if(!pcoinsTip->GetCoins(vin.prevout.hash, coins) || - (unsigned int)vin.prevout.n>=coins.vout.size() || - coins.vout[vin.prevout.n].IsNull()) { + CollateralStatus err = CheckCollateral(vin); + if (err == COLLATERAL_UTXO_NOT_FOUND) { nActiveState = MASTERNODE_OUTPOINT_SPENT; LogPrint("masternode", "CMasternode::Check -- Failed to find Masternode UTXO, masternode=%s\n", vin.prevout.ToStringShort()); return; @@ -632,18 +655,19 @@ bool CMasternodeBroadcast::CheckOutpoint(int& nDos) return false; } - CCoins coins; - if(!pcoinsTip->GetCoins(vin.prevout.hash, coins) || - (unsigned int)vin.prevout.n>=coins.vout.size() || - coins.vout[vin.prevout.n].IsNull()) { + int nHeight; + CollateralStatus err = CheckCollateral(vin, nHeight); + if (err == COLLATERAL_UTXO_NOT_FOUND) { LogPrint("masternode", "CMasternodeBroadcast::CheckOutpoint -- Failed to find Masternode UTXO, masternode=%s\n", vin.prevout.ToStringShort()); return false; } - if(coins.vout[vin.prevout.n].nValue != 1000 * COIN) { + + if (err == COLLATERAL_INVALID_AMOUNT) { LogPrint("masternode", "CMasternodeBroadcast::CheckOutpoint -- Masternode UTXO should have 1000 DASH, masternode=%s\n", vin.prevout.ToStringShort()); return false; } - if(chainActive.Height() - coins.nHeight + 1 < Params().GetConsensus().nMasternodeMinimumConfirmations) { + + if(chainActive.Height() - nHeight + 1 < Params().GetConsensus().nMasternodeMinimumConfirmations) { LogPrintf("CMasternodeBroadcast::CheckOutpoint -- Masternode UTXO must have at least %d confirmations, masternode=%s\n", Params().GetConsensus().nMasternodeMinimumConfirmations, vin.prevout.ToStringShort()); // maybe we miss few blocks, let this mnb to be checked again later diff --git a/src/masternode.h b/src/masternode.h index 81887f8400..08e198c07f 100644 --- a/src/masternode.h +++ b/src/masternode.h @@ -173,6 +173,12 @@ public: MASTERNODE_POSE_BAN }; + enum CollateralStatus { + COLLATERAL_OK, + COLLATERAL_UTXO_NOT_FOUND, + COLLATERAL_INVALID_AMOUNT + }; + CTxIn vin; CService addr; CPubKey pubKeyCollateralAddress; @@ -262,6 +268,8 @@ public: bool UpdateFromNewBroadcast(CMasternodeBroadcast& mnb); + static CollateralStatus CheckCollateral(CTxIn vin); + static CollateralStatus CheckCollateral(CTxIn vin, int& nHeight); void Check(bool fForce = false); bool IsBroadcastedWithin(int nSeconds) { return GetAdjustedTime() - sigTime < nSeconds; } diff --git a/src/rpc/governance.cpp b/src/rpc/governance.cpp index 9de4720533..6698550621 100644 --- a/src/rpc/governance.cpp +++ b/src/rpc/governance.cpp @@ -153,9 +153,12 @@ UniValue gobject(const UniValue& params, bool fHelp) throw JSONRPCError(RPC_INVALID_PARAMETER, "Trigger and watchdog objects need not be prepared (however only masternodes can create them)"); } - std::string strError = ""; - if(!govobj.IsValidLocally(strError, false)) - throw JSONRPCError(RPC_INTERNAL_ERROR, "Governance object is not valid - " + govobj.GetHash().ToString() + " - " + strError); + { + LOCK(cs_main); + std::string strError = ""; + if(!govobj.IsValidLocally(strError, false)) + throw JSONRPCError(RPC_INTERNAL_ERROR, "Governance object is not valid - " + govobj.GetHash().ToString() + " - " + strError); + } CWalletTx wtx; if(!pwalletMain->GetBudgetSystemCollateralTX(wtx, govobj.GetHash(), govobj.GetMinCollateralFee(), false)) { @@ -256,9 +259,12 @@ UniValue gobject(const UniValue& params, bool fHelp) std::string strError = ""; bool fMissingMasternode; bool fMissingConfirmations; - if(!govobj.IsValidLocally(strError, fMissingMasternode, fMissingConfirmations, true) && !fMissingConfirmations) { - LogPrintf("gobject(submit) -- Object submission rejected because object is not valid - hash = %s, strError = %s\n", strHash, strError); - throw JSONRPCError(RPC_INTERNAL_ERROR, "Governance object is not valid - " + strHash + " - " + strError); + { + LOCK(cs_main); + if(!govobj.IsValidLocally(strError, fMissingMasternode, fMissingConfirmations, true) && !fMissingConfirmations) { + LogPrintf("gobject(submit) -- Object submission rejected because object is not valid - hash = %s, strError = %s\n", strHash, strError); + throw JSONRPCError(RPC_INTERNAL_ERROR, "Governance object is not valid - " + strHash + " - " + strError); + } } // RELAY THIS OBJECT