From 520e1e0edecff3853e5a0e0c8cf45c746080472f Mon Sep 17 00:00:00 2001 From: DanS Date: Mon, 31 Aug 2026 22:11:42 -0500 Subject: [PATCH] audit: fix ADDR send amplification and the IVK-only witness deref (UNTESTED) Two more findings from the general audit. Compile-verified only, like the rest of this branch. main.cpp SendMessages / ADDR: the per-address loop called pto->PushAddrMessage(msgMaker.Make(flags, msg_type, pto->vAddrToSend)) i.e. it pushed the ENTIRE vector once per accepted address. One 24-byte getaddr therefore produced N messages of N addresses each instead of one message of N -- on a live addrman (GetAddr returning ~494) that is ~494 x ~14.8 KB = ~7.3 MB of upstream for a 24-byte request, and up to ~30 MB with a full addrman. All of it serialized, with a double-SHA256 checksum per message, while cs_main is held, so each burst also stalls block validation and RPC. The locally built vAddr was accumulated and then discarded, and the `vAddr.resize(MAX_ADDR_TO_SEND)` sat exactly where upstream has `vAddr.clear()` -- it can never fire, because vAddr.size() already equals MAX_ADDR_TO_SEND there. This is a local regression, not inherited: 512da314a rewrote the correct upstream form into this one. Restores the upstream idiom (accumulate, flush in MAX_ADDR_TO_SEND batches, send the remainder after the loop) and hoists the msg_type/make_flags selection out of the loop, since neither varies per address. wallet.cpp VerifyAndSetInitialWitness: five sites dereferenced `*item.second.nullifier` on a boost::optional that can legitimately be unset. A Sapling note discovered through an imported INCOMING viewing key has no nullifier -- computing one requires the full viewing key, and z_importviewingkey calls only AddSaplingIncomingViewingKey. Dereferencing it aborts the daemon with a boost assertion rather than an RPC error, at the end of the import's own rescan; and because the IVK-only note data is already persisted in the wallet transaction and BuildWitnessCache is re-driven from ChainTip on every connected block, the node then fails the same way on every restart. The codebase already had the right pattern in the two neighbouring functions -- DecrementNoteWitnesses guards with `if (nd->nullifier && ...)` and BuildWitnessCache with `if (!nd->nullifier) continue;` -- so only VerifyAndSetInitialWitness was missing it. Those guards also establish the intended semantics: a note whose spend depth cannot be computed is treated as UNSPENT, keeping its witness rather than pruning a note we cannot prove spent. Adds a boost::optional overload of SaplingWitnessMinimumHeight doing exactly that, and routes all five sites through it. NOT fixed here, deliberately, because none can be done responsibly without tests: - the inline TLS handshake on ThreadSocketHandler (architectural: one slow unauthenticated connection can freeze all P2P I/O for up to 60s); - the getblocktemplate function-static leaking a CBlockTemplate per concurrent caller, assigned with cs_main released; - z_sendmany's pre-flight size estimate omitting Sapling spends, so a wallet with ~499+ notes builds an oversize, unbroadcastable transaction after minutes of proving; - z_shieldcoinbase's opt-in donation paying a hardcoded upstream-Hush z-address that no DragonX party can spend (a policy decision, not a code fix). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FU87LdsJZiZkfq1eXubpeo --- src/main.cpp | 38 ++++++++++++++++++++++---------------- src/wallet/wallet.cpp | 20 +++++++++++++++----- src/wallet/wallet.h | 6 ++++++ 3 files changed, 43 insertions(+), 21 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 6bc40c659..c8ee7cb3c 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -8336,6 +8336,24 @@ bool SendMessages(CNode* pto, bool fSendTrickle) // Message: addr if (fSendTrickle) { + // Accumulate into vAddr and send it ONCE (or in MAX_ADDR_TO_SEND-sized batches). + // This loop previously pushed the ENTIRE pto->vAddrToSend on every accepted address, + // so a single 24-byte getaddr produced N messages of N addresses each instead of one + // message of N -- ~500x the intended bandwidth on a typical addrman, all serialized + // (with per-message double-SHA256 checksums) while cs_main is held. The locally built + // vAddr was accumulated and then discarded, and the vAddr.resize(MAX_ADDR_TO_SEND) was + // a no-op standing where upstream has vAddr.clear(). + const char* msg_type; + int make_flags; + if (pto->m_wants_addrv2) { + msg_type = NetMsgType::ADDRV2; + make_flags = ADDRV2_FORMAT; + } else { + msg_type = NetMsgType::ADDR; + make_flags = 0; + } + const CNetMsgMaker msgMaker(std::min(pto->nVersion, PROTOCOL_VERSION)); + vector vAddr; vAddr.reserve(pto->vAddrToSend.size()); BOOST_FOREACH(const CAddress& addr, pto->vAddrToSend) @@ -8343,29 +8361,17 @@ bool SendMessages(CNode* pto, bool fSendTrickle) if (pto->AddAddressIfNotAlreadyKnown(addr)) { vAddr.push_back(addr); - if (vAddr.size() >= MAX_ADDR_TO_SEND) { - // Should be impossible since we always check size before adding to - // vAddrToSend. Recover by trimming the vector. - vAddr.resize(MAX_ADDR_TO_SEND); + pto->PushAddrMessage(msgMaker.Make(make_flags, msg_type, vAddr)); + vAddr.clear(); } - const char* msg_type; - int make_flags; - if (pto->m_wants_addrv2) { - msg_type = NetMsgType::ADDRV2; - make_flags = ADDRV2_FORMAT; - } else { - msg_type = NetMsgType::ADDR; - make_flags = 0; - } - pto->PushAddrMessage(CNetMsgMaker(std::min(pto->nVersion, PROTOCOL_VERSION)).Make(make_flags, msg_type, pto->vAddrToSend)); - } } pto->vAddrToSend.clear(); - vAddr.clear(); + if (!vAddr.empty()) + pto->PushAddrMessage(msgMaker.Make(make_flags, msg_type, vAddr)); } CNodeState &state = *State(pto->GetId()); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 443498cba..285c09f5d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1240,6 +1240,16 @@ int CWallet::SaplingWitnessMinimumHeight(const uint256& nullifier, int nWitnessH return nMinimumHeight; } +int CWallet::SaplingWitnessMinimumHeight(const boost::optional& nullifier, int nWitnessHeight, int nMinimumHeight) +{ + // No nullifier => an incoming-viewing-key-only note (z_importviewingkey without the full + // viewing key). Spend depth is unknowable, so treat it as unspent and keep its witness. + if (!nullifier) { + return min(nWitnessHeight, nMinimumHeight); + } + return SaplingWitnessMinimumHeight(*nullifier, nWitnessHeight, nMinimumHeight); +} + int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessOnly) { LOCK2(cs_main, cs_wallet); @@ -1277,7 +1287,7 @@ int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessO //Skip Validation when witness root has been validated if (nd->witnessRootValidated) { - nMinimumHeight = SaplingWitnessMinimumHeight(*item.second.nullifier, nd->witnessHeight, nMinimumHeight); + nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight); continue; } @@ -1289,12 +1299,12 @@ int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessO CBlockIndex* whIndex = chainActive[nd->witnessHeight]; if (whIndex == NULL) { //witnessHeight strictly above the active chain (transient catch-up): cannot validate yet - nMinimumHeight = SaplingWitnessMinimumHeight(*item.second.nullifier, nd->witnessHeight, nMinimumHeight); + nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight); continue; } if (nd->witnesses.front().root() == whIndex->hashFinalSaplingRoot) { nd->witnessRootValidated = true; - nMinimumHeight = SaplingWitnessMinimumHeight(*item.second.nullifier, nd->witnessHeight, nMinimumHeight); + nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight); continue; } //root mismatch on the active chain -> desynced; fall through to rebuild below @@ -1306,7 +1316,7 @@ int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessO blockRoot = pblockindex->hashFinalSaplingRoot; if (witnessRoot == blockRoot) { nd->witnessRootValidated = true; - nMinimumHeight = SaplingWitnessMinimumHeight(*item.second.nullifier, nd->witnessHeight, nMinimumHeight); + nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight); continue; } } @@ -1358,7 +1368,7 @@ int CWallet::VerifyAndSetInitialWitness(const CBlockIndex* pindex, bool witnessO } nd->witnessHeight = pblockindex->GetHeight(); UpdateSaplingNullifierNoteMapWithTx(wtxItem.second); - nMinimumHeight = SaplingWitnessMinimumHeight(*item.second.nullifier, nd->witnessHeight, nMinimumHeight); + nMinimumHeight = SaplingWitnessMinimumHeight(item.second.nullifier, nd->witnessHeight, nMinimumHeight); } } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 107814971..27754d8f3 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -896,6 +896,12 @@ public: protected: int SaplingWitnessMinimumHeight(const uint256& nullifier, int nWitnessHeight, int nMinimumHeight); + //! Overload for a note whose nullifier may be unset. A note discovered through an imported + //! INCOMING viewing key has no nullifier (computing one needs the full viewing key), so + //! dereferencing the optional aborts the daemon. Treats such a note as unspent, which is the + //! conservative direction: it keeps the witness alive rather than pruning a note we cannot + //! prove spent. + int SaplingWitnessMinimumHeight(const boost::optional& nullifier, int nWitnessHeight, int nMinimumHeight); /** * pindex is the new tip being connected.