From 36201d2795b7b49beb1a837b0252134096beb64d Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Sat, 5 Sep 2026 03:16:12 +0000 Subject: compactblock: fail closed on RAND_bytes error, bound reconstruction allocation Two fixes to the file imported in the previous commit. Kept separate so the deviation from upstream's bytes is reviewable on its own. BuildCompactBlock ignored the RAND_bytes return value. On failure nonceBytes is left uninitialised and an indeterminate shortIdNonce is memcpy'd out and sent on the wire -- and that nonce is exactly what the file relies on being unpredictable so short IDs cannot be precomputed before a block exists or ground across blocks. Every other RAND_bytes call site in this tree (bip32.cpp:203, db.cpp:2462, db.cpp:2469) checks for != 1 and fails closed; this one now does too. The caller falls back to full-block relay. TryReconstructBlock sized a vector directly from the wire. nTotal is the sum of two deserialised vector lengths, and serialize.h bounds a vector only by MAX_SIZE (0x02000000), so once the P2P handlers are wired a single small message could ask this node to default-construct ~33M CTransactions. The smallest a transaction can serialise to is 10 bytes, so no block within MAX_BLOCK_SIZE holds more than MAX_BLOCK_SIZE/10 of them; anything above that is not a block this node could accept under any circumstance. Rejecting falls back to full-block relay, which is always correct. Neither is reachable today -- nothing feeds these functions from the network yet -- but both are latent the moment the message handlers land, which is the wrong time to be finding them. test_compactblock (17 checks) and all 14 selftests pass with both in place. --- src/compactblock.cpp | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) (limited to 'src/compactblock.cpp') diff --git a/src/compactblock.cpp b/src/compactblock.cpp index 3999ab7..1ca6118 100644 --- a/src/compactblock.cpp +++ b/src/compactblock.cpp @@ -48,8 +48,16 @@ bool BuildCompactBlock(const CBlock& block, CCmpctBlock& out) out.nBits = block.nBits; out.nNonce = block.nNonce; + // Upstream ignored the RAND_bytes return value here. On failure nonceBytes + // is left uninitialised and an indeterminate shortIdNonce goes out on the + // wire -- which is precisely the value this file relies on being + // unpredictable to stop short IDs being precomputed or ground against. + // Fail closed instead, matching every other RAND_bytes call site in this + // tree (bip32.cpp, db.cpp): the caller falls back to full-block relay, + // which is always correct. unsigned char nonceBytes[sizeof(uint64)]; - RAND_bytes(nonceBytes, sizeof(nonceBytes)); + if (RAND_bytes(nonceBytes, sizeof(nonceBytes)) != 1) + return false; memcpy(&out.shortIdNonce, nonceBytes, sizeof(uint64)); out.vPrefilledIndex.clear(); @@ -75,6 +83,18 @@ bool TryReconstructBlock(const CCmpctBlock& cmpct, CBlock& outBlock, std::vector if (cmpct.vPrefilledIndex.size() != cmpct.vPrefilledTx.size() || nTotal == 0) return false; // malformed + // nTotal comes straight off the wire once this is wired into the P2P + // layer, and serialize.h only bounds a vector length by MAX_SIZE + // (0x02000000), so an unbounded assign() below would let one 60-odd-byte + // message ask this node for ~33M default-constructed CTransactions. The + // smallest a transaction can serialise to is 10 bytes (nVersion, empty + // vin, empty vout, nLockTime), so no block within MAX_BLOCK_SIZE can hold + // more than MAX_BLOCK_SIZE/10 of them; anything above that is not a block + // we could ever accept. Rejecting here just falls back to full-block + // relay, which is always correct. + if (nTotal > MAX_BLOCK_SIZE / 10) + return false; // implausible tx count -- refuse to size a vector from it + outBlock.SetNull(); outBlock.nVersion = cmpct.nVersion; outBlock.hashPrevBlock = cmpct.hashPrevBlock; -- cgit v1.2.3