From 7e9b2c66152c690af15292871b850f33291b3e4d Mon Sep 17 00:00:00 2001 From: DanS Date: Tue, 14 Jul 2026 11:21:28 -0500 Subject: [PATCH] fix(net): verify RandomX at correct height in AcceptBlockHeader + cap locator header-pow: AcceptBlockHeader passed the caller's reused *ppindex (and a height derived from it, ==0 for a new header) to CheckBlockHeader instead of the header's own local pindex + real height. Post-IBD this made RandomXValidationRequired(0) false, so CheckRandomXSolution returned true WITHOUT verifying (and the fRandomXVerified short-circuit could fire on an unverified header) - silently defeating the header-flood PoW gate from b9fdc7981. Resolve pindexPrev up-front, pass real height (parent+1) and the local (NULL) pindex so the post-IBD RandomX check actually runs; IBD stays fast (fCheckPOW=0). Stability-tested: 303 valid headers accepted across a 4-node RandomX net, 0 false rejects / bans. #9 (MEDIUM) GETBLOCKS/GETHEADERS deserialized an unbounded CBlockLocator.vHave (~130k hashes) and scanned it linearly under cs_main with no ban - a message-thread liveness DoS. Add MAX_LOCATOR_SZ=101 + Misbehaving, matching the adjacent vInv/headers caps. Co-Authored-By: Claude Opus 4.8 --- src/main.cpp | 37 ++++++++++++++++++++++++++++++++++++- src/main.h | 6 ++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/src/main.cpp b/src/main.cpp index d72d730cc..834746ad9 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -5457,7 +5457,24 @@ bool AcceptBlockHeader(int32_t *futureblockp,const CBlockHeader& block, CValidat // SECURITY (header-flood DoS): once synced, verify PoW at header-accept time so a peer cannot // flood unbounded PoW-less headers into mapBlockIndex (they now fail RandomX -> DoS-ban). During // IBD keep fCheckPOW=0 for fast header sync; the full RandomX/target check runs at block connect. - if (!CheckBlockHeader(futureblockp,*ppindex!=0?(*ppindex)->GetHeight():0,*ppindex, block, state, IsInitialBlockDownload() ? 0 : 1)) { + // Resolve the parent up-front so CheckBlockHeader receives THIS header's own (still-NULL) pindex + // and its CORRECT height (parent height + 1) — never the caller's reused *ppindex, which in a + // HEADERS batch aliases the PREVIOUS header. Passing *ppindex here made (a) the height a stale + // value (0 for a fresh header, or the prior header's height when aliased) so post-IBD + // RandomXValidationRequired() saw a below-activation height and CheckRandomXSolution returned true + // WITHOUT verifying, and (b) the (pindex && pindex->fRandomXVerified) short-circuit fire on an + // as-yet-unverified header — both silently defeating the post-IBD header-flood PoW gate. The + // authoritative parent validation (prev-not-found / prev-invalid) still runs unchanged below; this + // lookup is read-only and under cs_main, so it cannot disagree with it. IBD stays fast: fCheckPOW + // is still 0 during IBD, so no RandomX is computed here regardless of the height. + CBlockIndex* pindexPrevForHeight = NULL; + { + BlockMap::iterator miPrev = mapBlockIndex.find(block.hashPrevBlock); + if (miPrev != mapBlockIndex.end()) + pindexPrevForHeight = miPrev->second; + } + int32_t nHeaderHeight = (pindexPrevForHeight != NULL) ? pindexPrevForHeight->GetHeight() + 1 : 0; + if (!CheckBlockHeader(futureblockp,nHeaderHeight,pindex, block, state, IsInitialBlockDownload() ? 0 : 1)) { if ( *futureblockp == 0 ) { LogPrintf("%s: CheckBlockHeader futureblock=0\n", __func__); return false; @@ -7587,6 +7604,15 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, uint256 hashStop; vRecv >> locator >> hashStop; + // Bound the locator before FindForkInGlobalIndex() scans it linearly under cs_main. An honest + // GetLocator() never exceeds MAX_LOCATOR_SZ, so this cannot reject a valid peer; an oversized + // vHave (~130k hashes fit in one message) is a message-thread liveness DoS. Ban like the + // adjacent vInv > MAX_INV_SZ path. + if (locator.vHave.size() > MAX_LOCATOR_SZ) { + Misbehaving(pfrom->GetId(), 20); + return true; + } + LOCK(cs_main); // Find the last block the caller has in the main chain @@ -7619,6 +7645,15 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, uint256 hashStop; vRecv >> locator >> hashStop; + // Bound the locator before FindForkInGlobalIndex() scans it linearly under cs_main. An honest + // GetLocator() never exceeds MAX_LOCATOR_SZ, so this cannot reject a valid peer; an oversized + // vHave (~130k hashes fit in one message) is a message-thread liveness DoS. Ban like the + // adjacent vInv > MAX_INV_SZ path. + if (locator.vHave.size() > MAX_LOCATOR_SZ) { + Misbehaving(pfrom->GetId(), 20); + return true; + } + LOCK(cs_main); diff --git a/src/main.h b/src/main.h index ee57a18be..007e67891 100644 --- a/src/main.h +++ b/src/main.h @@ -130,6 +130,12 @@ static const unsigned int BLOCK_STALLING_TIMEOUT = 2; * peer's 160-header reply as "tip reached" and stall header sync. Raise only as a coordinated * network upgrade (with a protocol-version bump). */ static const unsigned int MAX_HEADERS_RESULTS = 160; +/** Maximum number of entries we accept in a CBlockLocator.vHave (GETBLOCKS / GETHEADERS). An honest + * CChain::GetLocator() emits ~10 linear hashes then exponentially-spaced ones, so even a chain of + * 2^91 blocks stays well under this bound (GetLocator reserves 32). Matches upstream Bitcoin Core's + * MAX_LOCATOR_SZ. A larger vHave is a peer trying to make FindForkInGlobalIndex() linearly scan a + * huge list under cs_main (message-thread liveness DoS). */ +static const unsigned int MAX_LOCATOR_SZ = 101; /** Size of the "block download window": how far ahead of our current height do we fetch? * Larger windows tolerate larger download speed differences between peer, but increase the potential * degree of disordering of blocks on disk (which make reindexing and in the future perhaps pruning