From 85aad4be5d1f8c7b422921b980fa762277a7cedf Mon Sep 17 00:00:00 2001 From: ca333 Date: Wed, 2 Oct 2019 21:53:47 +0200 Subject: [PATCH] fix PING attack [CVE-2019-17048] --- src/init.cpp | 20 +++++++++++++++++ src/main.cpp | 2 -- src/rpc/blockchain.cpp | 4 ++++ src/txmempool.cpp | 47 +++++++++++++++++++++++++++++++++++++++ src/txmempool.h | 7 ++++++ src/wallet/wallet.cpp | 16 ++------------ src/wallet/wallet.h | 2 +- src/zcash/Note.cpp | 50 +++++++++++++++++++++++++++--------------- 8 files changed, 113 insertions(+), 35 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 107e3e476..4b3b32ca2 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -728,6 +728,22 @@ void ThreadImport(std::vector vImportFiles) } } +void ThreadNotifyRecentlyAdded() +{ + while (true) { + // Run the notifier on an integer second in the steady clock. + auto now = std::chrono::steady_clock::now().time_since_epoch(); + auto nextFire = std::chrono::duration_cast( + now + std::chrono::seconds(1)); + std::this_thread::sleep_until( + std::chrono::time_point(nextFire)); + + boost::this_thread::interruption_point(); + + mempool.NotifyRecentlyAdded(); + } +} + /** Sanity checks * Ensure that Bitcoin is running in a usable environment with all * necessary library support. @@ -1935,6 +1951,10 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) LogPrintf("mapAddressBook.size() = %u\n", pwalletMain ? pwalletMain->mapAddressBook.size() : 0); #endif + // Start the thread that notifies listeners of transactions that have been + // recently added to the mempool. + threadGroup.create_thread(boost::bind(&TraceThread, "txnotify", &ThreadNotifyRecentlyAdded)); + if (GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION)) StartTorControl(threadGroup, scheduler); diff --git a/src/main.cpp b/src/main.cpp index 30783e2ab..2e2a34a1a 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -2047,8 +2047,6 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa } } - SyncWithWallets(tx, NULL); - return true; } diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 9c55c807b..813b92a77 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1899,6 +1899,10 @@ UniValue mempoolInfoToJSON() ret.push_back(Pair("bytes", (int64_t) mempool.GetTotalTxSize())); ret.push_back(Pair("usage", (int64_t) mempool.DynamicMemoryUsage())); + if (Params().NetworkIDString() == "regtest") { + ret.push_back(Pair("fullyNotified", mempool.IsFullyNotified())); + } + return ret; } diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 2b38d7153..037f71198 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -29,6 +29,7 @@ #include "timedata.h" #include "util.h" #include "utilmoneystr.h" +#include "validationinterface.h" #include "version.h" #define _COINBASE_MATURITY 100 @@ -119,6 +120,8 @@ bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, LOCK(cs); mapTx.insert(entry); const CTransaction& tx = mapTx.find(hash)->GetTx(); + mapRecentlyAddedTx[tx.GetHash()] = &tx; + nRecentlyAddedSequence += 1; if (!tx.IsCoinImport()) { for (unsigned int i = 0; i < tx.vin.size(); i++) mapNextTx[tx.vin[i].prevout] = CInPoint(&tx, i); @@ -360,6 +363,7 @@ void CTxMemPool::remove(const CTransaction &origTx, std::list& rem txToRemove.push_back(it->second.ptx->GetHash()); } } + mapRecentlyAddedTx.erase(hash); BOOST_FOREACH(const CTxIn& txin, tx.vin) mapNextTx.erase(txin.prevout); BOOST_FOREACH(const JSDescription& joinsplit, tx.vjoinsplit) { @@ -833,6 +837,49 @@ bool CTxMemPool::nullifierExists(const uint256& nullifier, ShieldedType type) co } } +void CTxMemPool::NotifyRecentlyAdded() +{ + uint64_t recentlyAddedSequence; + std::vector txs; + { + LOCK(cs); + recentlyAddedSequence = nRecentlyAddedSequence; + for (const auto& kv : mapRecentlyAddedTx) { + txs.push_back(*(kv.second)); + } + mapRecentlyAddedTx.clear(); + } + + // A race condition can occur here between these SyncWithWallets calls, and + // the ones triggered by block logic (in ConnectTip and DisconnectTip). It + // is harmless because calling SyncWithWallets(_, NULL) does not alter the + // wallet transaction's block information. + for (auto tx : txs) { + try { + SyncWithWallets(tx, NULL); + } catch (const boost::thread_interrupted&) { + throw; + } catch (const std::exception& e) { + PrintExceptionContinue(&e, "CTxMemPool::NotifyRecentlyAdded()"); + } catch (...) { + PrintExceptionContinue(NULL, "CTxMemPool::NotifyRecentlyAdded()"); + } + } + + // Update the notified sequence number. We only need this in regtest mode, + // and should not lock on cs after calling SyncWithWallets otherwise. + if (Params().NetworkIDString() == "regtest") { + LOCK(cs); + nNotifiedSequence = recentlyAddedSequence; + } +} + +bool CTxMemPool::IsFullyNotified() { + assert(Params().NetworkIDString() == "regtest"); + LOCK(cs); + return nRecentlyAddedSequence == nNotifiedSequence; +} + CCoinsViewMemPool::CCoinsViewMemPool(CCoinsView *baseIn, CTxMemPool &mempoolIn) : CCoinsViewBacked(baseIn), mempool(mempoolIn) { } bool CCoinsViewMemPool::GetNullifier(const uint256 &nf, ShieldedType type) const diff --git a/src/txmempool.h b/src/txmempool.h index d3e7f7b57..b73ff4b39 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -148,6 +148,10 @@ private: uint64_t totalTxSize = 0; //! sum of all mempool tx' byte sizes uint64_t cachedInnerUsage; //! sum of dynamic memory usage of all the map elements (NOT the maps themselves) + std::map mapRecentlyAddedTx; + uint64_t nRecentlyAddedSequence = 0; + uint64_t nNotifiedSequence = 0; + std::map mapSproutNullifiers; std::map mapSaplingNullifiers; @@ -234,6 +238,9 @@ public: bool nullifierExists(const uint256& nullifier, ShieldedType type) const; + void NotifyRecentlyAdded(); + bool IsFullyNotified(); + unsigned long size() { LOCK(cs); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 4d834ccee..77ea2e59d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1841,7 +1841,7 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlock* pbl void CWallet::SyncTransaction(const CTransaction& tx, const CBlock* pblock) { - LOCK2(cs_main, cs_wallet); + LOCK(cs_wallet); if (!AddToWalletIfInvolvingMe(tx, pblock, true)) return; // Not one of ours @@ -4830,9 +4830,8 @@ CWalletKey::CWalletKey(int64_t nExpires) nTimeExpires = nExpires; } -int CMerkleTx::SetMerkleBranch(const CBlock& block) +void CMerkleTx::SetMerkleBranch(const CBlock& block) { - AssertLockHeld(cs_main); CBlock blockTmp; // Update the tx's hashBlock @@ -4847,21 +4846,10 @@ int CMerkleTx::SetMerkleBranch(const CBlock& block) vMerkleBranch.clear(); nIndex = -1; LogPrintf("ERROR: SetMerkleBranch(): couldn't find tx in block\n"); - return 0; } // Fill in merkle branch vMerkleBranch = block.GetMerkleBranch(nIndex); - - // Is the tx in a block that's in the main chain - BlockMap::iterator mi = mapBlockIndex.find(hashBlock); - if (mi == mapBlockIndex.end()) - return 0; - const CBlockIndex* pindex = (*mi).second; - if (!pindex || !chainActive.Contains(pindex)) - return 0; - - return chainActive.Height() - pindex->GetHeight() + 1; } int CMerkleTx::GetDepthInMainChainINTERNAL(const CBlockIndex* &pindexRet) const diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 296e2fa57..60b5fb3bf 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -386,7 +386,7 @@ public: READWRITE(nIndex); } - int SetMerkleBranch(const CBlock& block); + void SetMerkleBranch(const CBlock& block); /** diff --git a/src/zcash/Note.cpp b/src/zcash/Note.cpp index ee8f7b641..23210c784 100644 --- a/src/zcash/Note.cpp +++ b/src/zcash/Note.cpp @@ -173,15 +173,21 @@ boost::optional SaplingOutgoingPlaintext::decrypt( } // Deserialize from the plaintext - CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); - ss << pt.get(); + try { + CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); + ss << pt.get(); - SaplingOutgoingPlaintext ret; - ss >> ret; + SaplingOutgoingPlaintext ret; + ss >> ret; - assert(ss.size() == 0); + assert(ss.size() == 0); - return ret; + return ret; + } catch (const boost::thread_interrupted&) { + throw; + } catch (...) { + return boost::none; + } } boost::optional SaplingNotePlaintext::decrypt( @@ -197,13 +203,17 @@ boost::optional SaplingNotePlaintext::decrypt( } // Deserialize from the plaintext - CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); - ss << pt.get(); - SaplingNotePlaintext ret; - ss >> ret; - - assert(ss.size() == 0); + try { + CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); + ss << pt.get(); + ss >> ret; + assert(ss.size() == 0); + } catch (const boost::thread_interrupted&) { + throw; + } catch (...) { + return boost::none; + } uint256 pk_d; if (!librustzcash_ivk_to_pkd(ivk.begin(), ret.d.data(), pk_d.begin())) { @@ -243,11 +253,17 @@ boost::optional SaplingNotePlaintext::decrypt( } // Deserialize from the plaintext - CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); - ss << pt.get(); - SaplingNotePlaintext ret; - ss >> ret; + try { + CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); + ss << pt.get(); + ss >> ret; + assert(ss.size() == 0); + } catch (const boost::thread_interrupted&) { + throw; + } catch (...) { + return boost::none; + } uint256 cmu_expected; if (!librustzcash_sapling_compute_cm( @@ -265,8 +281,6 @@ boost::optional SaplingNotePlaintext::decrypt( return boost::none; } - assert(ss.size() == 0); - return ret; }