From 9f3020f117dec258f93def05cbdb27d86917a015 Mon Sep 17 00:00:00 2001 From: alex Date: Thu, 5 Sep 2013 00:12:02 +0400 Subject: [PATCH] Always reserialize transactions before relaying This is more proper way to fix CVE-2013-4627. See https://bitslog.wordpress.com/2013/07/18/buggy-cve-2013-4627-patch-open-new-vectors-of-attack/ for details. --- src/main.cpp | 72 +++++++++++++++------------------------ src/net.cpp | 28 +++++++++++++++ src/net.h | 40 ++-------------------- src/rpcrawtransaction.cpp | 2 +- src/wallet.cpp | 4 +-- 5 files changed, 61 insertions(+), 85 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index eb2dbc6f9a..85640d75c7 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -64,8 +64,8 @@ multimap mapOrphanBlocksByPrev; set > setStakeSeenOrphan; map mapProofOfStake; -map mapOrphanTransactions; -map > mapOrphanTransactionsByPrev; +map mapOrphanTransactions; +map > mapOrphanTransactionsByPrev; // Constant stuff for coinbase transactions we create: CScript COINBASE_FLAGS; @@ -191,16 +191,12 @@ void ResendWalletTransactions() // mapOrphanTransactions // -bool AddOrphanTx(const CDataStream& vMsg) +bool AddOrphanTx(const CTransaction& tx) { - CTransaction tx; - CDataStream(vMsg) >> tx; uint256 hash = tx.GetHash(); if (mapOrphanTransactions.count(hash)) return false; - CDataStream* pvMsg = new CDataStream(vMsg); - // Ignore big transactions, to avoid a // send-big-orphans memory exhaustion attack. If a peer has a legitimate // large transaction with a missing parent then we assume @@ -208,16 +204,18 @@ bool AddOrphanTx(const CDataStream& vMsg) // have been mined or received. // 10,000 orphans, each of which is at most 5,000 bytes big is // at most 500 megabytes of orphans: - if (pvMsg->size() > 5000) + + size_t nSize = tx.GetSerializeSize(SER_NETWORK, CTransaction::CURRENT_VERSION); + + if (nSize > 5000) { - printf("ignoring large orphan tx (size: %"PRIszu", hash: %s)\n", pvMsg->size(), hash.ToString().substr(0,10).c_str()); - delete pvMsg; + printf("ignoring large orphan tx (size: %"PRIszu", hash: %s)\n", nSize, hash.ToString().substr(0,10).c_str()); return false; } - mapOrphanTransactions[hash] = pvMsg; + mapOrphanTransactions[hash] = tx; BOOST_FOREACH(const CTxIn& txin, tx.vin) - mapOrphanTransactionsByPrev[txin.prevout.hash].insert(make_pair(hash, pvMsg)); + mapOrphanTransactionsByPrev[txin.prevout.hash].insert(hash); printf("stored orphan tx %s (mapsz %"PRIszu")\n", hash.ToString().substr(0,10).c_str(), mapOrphanTransactions.size()); @@ -228,16 +226,13 @@ void static EraseOrphanTx(uint256 hash) { if (!mapOrphanTransactions.count(hash)) return; - const CDataStream* pvMsg = mapOrphanTransactions[hash]; - CTransaction tx; - CDataStream(*pvMsg) >> tx; + const CTransaction& tx = mapOrphanTransactions[hash]; BOOST_FOREACH(const CTxIn& txin, tx.vin) { mapOrphanTransactionsByPrev[txin.prevout.hash].erase(hash); if (mapOrphanTransactionsByPrev[txin.prevout.hash].empty()) mapOrphanTransactionsByPrev.erase(txin.prevout.hash); } - delete pvMsg; mapOrphanTransactions.erase(hash); } @@ -248,7 +243,7 @@ unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans) { // Evict a random orphan: uint256 randomhash = GetRandHash(); - map::iterator it = mapOrphanTransactions.lower_bound(randomhash); + map::iterator it = mapOrphanTransactions.lower_bound(randomhash); if (it == mapOrphanTransactions.end()) it = mapOrphanTransactions.begin(); EraseOrphanTx(it->first); @@ -3069,10 +3064,6 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv) return true; } - - - - if (strCommand == "version") { // Each connection can only send one version message @@ -3119,7 +3110,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv) return true; } - // ppcoin: record my external IP reported by peer + // record my external IP reported by peer if (addrFrom.IsRoutable() && addrMe.IsRoutable()) addrSeenByPeer = addrMe; @@ -3179,7 +3170,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv) item.second.RelayTo(pfrom); } - // ppcoin: relay sync-checkpoint + // Relay sync-checkpoint { LOCK(Checkpoints::cs_hashSyncCheckpoint); if (!Checkpoints::checkpointMessage.IsNull()) @@ -3278,7 +3269,6 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv) pfrom->fDisconnect = true; } - else if (strCommand == "inv") { vector vInv; @@ -3501,17 +3491,11 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv) CInv inv(MSG_TX, tx.GetHash()); pfrom->AddInventoryKnown(inv); - // Truncate messages to the size of the tx in them - unsigned int nSize = ::GetSerializeSize(tx,SER_NETWORK, PROTOCOL_VERSION); - if (nSize < vMsg.size()){ - vMsg.resize(nSize); - } - bool fMissingInputs = false; if (tx.AcceptToMemoryPool(txdb, true, &fMissingInputs)) { SyncWithWallets(tx, NULL, true); - RelayMessage(inv, vMsg); + RelayTransaction(tx, inv.hash); mapAlreadyAskedFor.erase(inv); vWorkQueue.push_back(inv.hash); vEraseQueue.push_back(inv.hash); @@ -3520,30 +3504,28 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv) for (unsigned int i = 0; i < vWorkQueue.size(); i++) { uint256 hashPrev = vWorkQueue[i]; - for (map::iterator mi = mapOrphanTransactionsByPrev[hashPrev].begin(); + for (set::iterator mi = mapOrphanTransactionsByPrev[hashPrev].begin(); mi != mapOrphanTransactionsByPrev[hashPrev].end(); ++mi) { - const CDataStream& vMsg = *((*mi).second); - CTransaction tx; - CDataStream(vMsg) >> tx; - CInv inv(MSG_TX, tx.GetHash()); + const uint256& orphanTxHash = *mi; + CTransaction& orphanTx = mapOrphanTransactions[orphanTxHash]; bool fMissingInputs2 = false; - if (tx.AcceptToMemoryPool(txdb, true, &fMissingInputs2)) + if (orphanTx.AcceptToMemoryPool(txdb, true, &fMissingInputs2)) { - printf(" accepted orphan tx %s\n", inv.hash.ToString().substr(0,10).c_str()); + printf(" accepted orphan tx %s\n", orphanTxHash.ToString().substr(0,10).c_str()); SyncWithWallets(tx, NULL, true); - RelayMessage(inv, vMsg); - mapAlreadyAskedFor.erase(inv); - vWorkQueue.push_back(inv.hash); - vEraseQueue.push_back(inv.hash); + RelayTransaction(orphanTx, orphanTxHash); + mapAlreadyAskedFor.erase(CInv(MSG_TX, orphanTxHash)); + vWorkQueue.push_back(orphanTxHash); + vEraseQueue.push_back(orphanTxHash); } else if (!fMissingInputs2) { // invalid orphan - vEraseQueue.push_back(inv.hash); - printf(" removed invalid orphan tx %s\n", inv.hash.ToString().substr(0,10).c_str()); + vEraseQueue.push_back(orphanTxHash); + printf(" removed invalid orphan tx %s\n", orphanTxHash.ToString().substr(0,10).c_str()); } } } @@ -3553,7 +3535,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv) } else if (fMissingInputs) { - AddOrphanTx(vMsg); + AddOrphanTx(tx); // DoS prevention: do not allow mapOrphanTransactions to grow unbounded unsigned int nEvicted = LimitOrphanTxSize(MAX_ORPHAN_TRANSACTIONS); diff --git a/src/net.cpp b/src/net.cpp index 72ec614e0f..5ae5ed5efe 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1950,3 +1950,31 @@ class CNetCleanup } } instance_of_cnetcleanup; + +void RelayTransaction(const CTransaction& tx, const uint256& hash) +{ + CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); + ss.reserve(10000); + ss << tx; + RelayTransaction(tx, hash, ss); +} + +void RelayTransaction(const CTransaction& tx, const uint256& hash, const CDataStream& ss) +{ + CInv inv(MSG_TX, hash); + { + LOCK(cs_mapRelay); + // Expire old relay messages + while (!vRelayExpiration.empty() && vRelayExpiration.front().first < GetTime()) + { + mapRelay.erase(vRelayExpiration.front().second); + vRelayExpiration.pop_front(); + } + + // Save original serialized message so newer versions are preserved + mapRelay.insert(std::make_pair(inv, ss)); + vRelayExpiration.push_back(std::make_pair(GetTime() + 15 * 60, inv)); + } + + RelayInventory(inv); +} diff --git a/src/net.h b/src/net.h index a7d9c62161..8a8916a1d2 100644 --- a/src/net.h +++ b/src/net.h @@ -641,15 +641,6 @@ class CNode void copyStats(CNodeStats &stats); }; - - - - - - - - - inline void RelayInventory(const CInv& inv) { // Put on lists to offer to the other nodes @@ -660,34 +651,9 @@ inline void RelayInventory(const CInv& inv) } } -template -void RelayMessage(const CInv& inv, const T& a) -{ - CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); - ss.reserve(10000); - ss << a; - RelayMessage(inv, ss); -} - -template<> -inline void RelayMessage<>(const CInv& inv, const CDataStream& ss) -{ - { - LOCK(cs_mapRelay); - // Expire old relay messages - while (!vRelayExpiration.empty() && vRelayExpiration.front().first < GetTime()) - { - mapRelay.erase(vRelayExpiration.front().second); - vRelayExpiration.pop_front(); - } - - // Save original serialized message so newer versions are preserved - mapRelay.insert(std::make_pair(inv, ss)); - vRelayExpiration.push_back(std::make_pair(GetTime() + 15 * 60, inv)); - } - - RelayInventory(inv); -} +class CTransaction; +void RelayTransaction(const CTransaction& tx, const uint256& hash); +void RelayTransaction(const CTransaction& tx, const uint256& hash, const CDataStream& ss); #endif diff --git a/src/rpcrawtransaction.cpp b/src/rpcrawtransaction.cpp index f683db9d01..e5ea5c8051 100644 --- a/src/rpcrawtransaction.cpp +++ b/src/rpcrawtransaction.cpp @@ -558,7 +558,7 @@ Value sendrawtransaction(const Array& params, bool fHelp) SyncWithWallets(tx, NULL, true); } - RelayMessage(CInv(MSG_TX, hashTx), tx); + RelayTransaction(tx, hashTx); return hashTx.GetHex(); } diff --git a/src/wallet.cpp b/src/wallet.cpp index 33ece6a890..3036f23d7d 100644 --- a/src/wallet.cpp +++ b/src/wallet.cpp @@ -904,7 +904,7 @@ void CWalletTx::RelayWalletTransaction(CTxDB& txdb) { uint256 hash = tx.GetHash(); if (!txdb.ContainsTx(hash)) - RelayMessage(CInv(MSG_TX, hash), (CTransaction)tx); + RelayTransaction((CTransaction)tx, hash); } } if (!(IsCoinBase() || IsCoinStake())) @@ -913,7 +913,7 @@ void CWalletTx::RelayWalletTransaction(CTxDB& txdb) if (!txdb.ContainsTx(hash)) { printf("Relaying wtx %s\n", hash.ToString().substr(0,10).c_str()); - RelayMessage(CInv(MSG_TX, hash), (CTransaction)*this); + RelayTransaction((CTransaction)*this, hash); } } }