Skip to content

Commit f43c2b8

Browse files
Merge bitcoin#29412: p2p: Don't process mutated blocks
Backport of bitcoin#29412, slimmed for Dash. Introduces IsBlockMutated() and isolates the merkle-root check into CheckMerkleRoot() (with an m_checked_merkle_root cache on CBlock), and rejects/discourages mutated blocks early in the BLOCK message handler as a defence-in-depth DoS mitigation. Dash adaptations: Dash is not a segwit chain and has none of the witness-commitment primitives (GetWitnessCommitmentIndex, BlockWitnessMerkleRoot, HasWitness, m_checked_witness_commitment). IsBlockMutated therefore drops the witness/check_witness_root machinery entirely and reduces to the merkle-root and CVE-2012-2459 / 64-byte-transaction malleation checks; it uses ::GetSerializeSize(*tx, PROTOCOL_VERSION) instead of TX_NO_WITNESS. The early net_processing check is unconditional (no prev_block/DEPLOYMENT_SEGWIT gating) and uses Misbehaving(pfrom.GetId(), 100, ...).
1 parent 348bb55 commit f43c2b8

7 files changed

Lines changed: 234 additions & 12 deletions

File tree

src/net_processing.cpp

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5200,6 +5200,17 @@ void PeerManagerImpl::ProcessMessage(
52005200

52015201
LogPrint(BCLog::NET, "received block %s peer=%d\n", pblock->GetHash().ToString(), pfrom.GetId());
52025202

5203+
// Check for possible mutation as a defence-in-depth mitigation against
5204+
// attacks that leverage mutated blocks. Dash has no witness commitment,
5205+
// so this reduces to the merkle-root / 64-byte-transaction malleation
5206+
// checks and can be performed unconditionally.
5207+
if (IsBlockMutated(/*block=*/*pblock)) {
5208+
LogPrint(BCLog::NET, "Received mutated block from peer=%d\n", pfrom.GetId());
5209+
Misbehaving(*peer, 100, "mutated block");
5210+
WITH_LOCK(cs_main, RemoveBlockRequest(pblock->GetHash(), pfrom.GetId()));
5211+
return;
5212+
}
5213+
52035214
bool forceProcessing = false;
52045215
const uint256 hash(pblock->GetHash());
52055216
{

src/primitives/block.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,8 @@ class CBlock : public CBlockHeader
190190
std::vector<CTransactionRef> vtx;
191191

192192
// memory only
193-
mutable bool fChecked;
193+
mutable bool fChecked; // CheckBlock()
194+
mutable bool m_checked_merkle_root{false}; // CheckMerkleRoot()
194195

195196
CBlock()
196197
{
@@ -214,6 +215,7 @@ class CBlock : public CBlockHeader
214215
CBlockHeader::SetNull();
215216
vtx.clear();
216217
fChecked = false;
218+
m_checked_merkle_root = false;
217219
}
218220

219221
CBlockHeader GetBlockHeader() const

src/test/validation_tests.cpp

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,10 @@
33
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
44

55
#include <consensus/amount.h>
6+
#include <consensus/merkle.h>
67
#include <net.h>
8+
#include <primitives/block.h>
9+
#include <primitives/transaction.h>
710
#include <uint256.h>
811
#include <validation.h>
912

@@ -36,4 +39,62 @@ BOOST_AUTO_TEST_CASE(test_assumeutxo)
3639
BOOST_CHECK_EQUAL(out210.nChainTx, 200U);
3740
}
3841

42+
//! Test the Dash (non-witness) IsBlockMutated() predicate directly.
43+
BOOST_AUTO_TEST_CASE(block_malleation)
44+
{
45+
// Calls IsBlockMutated and clears the CBlock validity-cache flags so the
46+
// same block object can be re-tested.
47+
auto is_mutated = [](CBlock& block) {
48+
bool mutated{IsBlockMutated(block)};
49+
block.fChecked = false;
50+
block.m_checked_merkle_root = false;
51+
return mutated;
52+
};
53+
auto is_not_mutated = [&is_mutated](CBlock& block) {
54+
return !is_mutated(block);
55+
};
56+
57+
auto create_coinbase_tx = []() {
58+
CMutableTransaction coinbase;
59+
coinbase.vin.resize(1);
60+
coinbase.vout.resize(1);
61+
coinbase.vout[0].scriptPubKey.resize(4);
62+
auto tx = MakeTransactionRef(coinbase);
63+
assert(tx->IsCoinBase());
64+
return tx;
65+
};
66+
67+
CBlock block;
68+
69+
// An empty block is expected to have a merkle root of 0x0.
70+
BOOST_CHECK(block.vtx.empty());
71+
block.hashMerkleRoot = uint256::ONE;
72+
BOOST_CHECK(is_mutated(block));
73+
block.hashMerkleRoot = uint256();
74+
BOOST_CHECK(is_not_mutated(block));
75+
76+
// A block with a single coinbase tx is mutated if the merkle root does not
77+
// equal the coinbase tx's hash.
78+
block.vtx.push_back(create_coinbase_tx());
79+
BOOST_CHECK(block.vtx[0]->GetHash() != block.hashMerkleRoot);
80+
BOOST_CHECK(is_mutated(block));
81+
block.hashMerkleRoot = BlockMerkleRoot(block);
82+
BOOST_CHECK(is_not_mutated(block));
83+
84+
// A block with two transactions is mutated if the merkle root does not
85+
// match the transactions.
86+
block.vtx.push_back(MakeTransactionRef(CMutableTransaction{}));
87+
BOOST_CHECK(is_mutated(block));
88+
block.hashMerkleRoot = BlockMerkleRoot(block);
89+
BOOST_CHECK(is_not_mutated(block));
90+
91+
// A block with a duplicated transaction (CVE-2012-2459) is mutated even
92+
// when hashMerkleRoot is set to the value the malleable tree computes.
93+
block.vtx[1] = block.vtx[0];
94+
bool mutated{false};
95+
block.hashMerkleRoot = BlockMerkleRoot(block, &mutated);
96+
BOOST_CHECK(mutated);
97+
BOOST_CHECK(is_mutated(block));
98+
}
99+
39100
BOOST_AUTO_TEST_SUITE_END()

src/validation.cpp

Lines changed: 55 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3788,6 +3788,33 @@ static bool CheckBlockHeader(const CBlockHeader& block, const uint256& hash, Blo
37883788
return true;
37893789
}
37903790

3791+
static bool CheckMerkleRoot(const CBlock& block, BlockValidationState& state)
3792+
{
3793+
if (block.m_checked_merkle_root) return true;
3794+
3795+
bool mutated;
3796+
uint256 merkle_root = BlockMerkleRoot(block, &mutated);
3797+
if (block.hashMerkleRoot != merkle_root) {
3798+
return state.Invalid(
3799+
/*result=*/BlockValidationResult::BLOCK_MUTATED,
3800+
/*reject_reason=*/"bad-txnmrklroot",
3801+
/*debug_message=*/"hashMerkleRoot mismatch");
3802+
}
3803+
3804+
// Check for merkle tree malleability (CVE-2012-2459): repeating sequences
3805+
// of transactions in a block without affecting the merkle root of a block,
3806+
// while still invalidating it.
3807+
if (mutated) {
3808+
return state.Invalid(
3809+
/*result=*/BlockValidationResult::BLOCK_MUTATED,
3810+
/*reject_reason=*/"bad-txns-duplicate",
3811+
/*debug_message=*/"duplicate transaction");
3812+
}
3813+
3814+
block.m_checked_merkle_root = true;
3815+
return true;
3816+
}
3817+
37913818
bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW, bool fCheckMerkleRoot, const uint256* known_hash)
37923819
{
37933820
// These are checks that are independent of context.
@@ -3806,17 +3833,8 @@ bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensu
38063833
return false;
38073834

38083835
// Check the merkle root.
3809-
if (fCheckMerkleRoot) {
3810-
bool mutated;
3811-
uint256 hashMerkleRoot2 = BlockMerkleRoot(block, &mutated);
3812-
if (block.hashMerkleRoot != hashMerkleRoot2)
3813-
return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-txnmrklroot", "hashMerkleRoot mismatch");
3814-
3815-
// Check for merkle tree malleability (CVE-2012-2459): repeating sequences
3816-
// of transactions in a block without affecting the merkle root of a block,
3817-
// while still invalidating it.
3818-
if (mutated)
3819-
return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-txns-duplicate", "duplicate transaction");
3836+
if (fCheckMerkleRoot && !CheckMerkleRoot(block, state)) {
3837+
return false;
38203838
}
38213839

38223840
// All potential-corruption validation must be done before we do any
@@ -3865,6 +3883,32 @@ bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensu
38653883
return true;
38663884
}
38673885

3886+
bool IsBlockMutated(const CBlock& block)
3887+
{
3888+
BlockValidationState state;
3889+
if (!CheckMerkleRoot(block, state)) {
3890+
LogPrint(BCLog::VALIDATION, "Block mutated: %s\n", state.ToString());
3891+
return true;
3892+
}
3893+
3894+
if (block.vtx.empty() || !block.vtx[0]->IsCoinBase()) {
3895+
// Consider the block mutated if any transaction is 64 bytes in size (see 3.1
3896+
// in "Weaknesses in Bitcoin's Merkle Root Construction":
3897+
// https://lists.linuxfoundation.org/pipermail/bitcoin-dev/attachments/20190225/a27d8837/attachment-0001.pdf).
3898+
//
3899+
// Note: This is not a consensus change as this only applies to blocks that
3900+
// don't have a coinbase transaction and would therefore already be invalid.
3901+
return std::any_of(block.vtx.begin(), block.vtx.end(),
3902+
[](auto& tx) { return ::GetSerializeSize(*tx, PROTOCOL_VERSION) == 64; });
3903+
} else {
3904+
// Theoretically it is still possible for a block with a 64 byte
3905+
// coinbase transaction to be mutated but we neglect that possibility
3906+
// here as it requires at least 224 bits of work.
3907+
}
3908+
3909+
return false;
3910+
}
3911+
38683912
/** Context-dependent validity checks.
38693913
* By "context", we mean only the previous block headers, but not the UTXO
38703914
* set; UTXO-related validity checks are done in ConnectBlock().

src/validation.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -374,6 +374,10 @@ void InitScriptExecutionCache();
374374
/** Context-independent validity checks */
375375
bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW = true, bool fCheckMerkleRoot = true, const uint256* known_hash = nullptr);
376376

377+
/** Check if a block has been mutated (with respect to its merkle root and,
378+
* for defence-in-depth, CVE-2012-2459 / 64-byte-transaction malleation). */
379+
bool IsBlockMutated(const CBlock& block);
380+
377381
/** Check a block is completely valid from start to finish (only works on top of our current best block) */
378382
bool TestBlockValidity(BlockValidationState& state,
379383
const chainlock::Chainlocks& chainlocks,
Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
#!/usr/bin/env python3
2+
# Copyright (c) The Bitcoin Core developers
3+
# Distributed under the MIT software license, see the accompanying
4+
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+
6+
"""
7+
Test that an attacker can't degrade compact block relay by sending unsolicited
8+
mutated blocks to clear in-flight blocktxn requests from other honest peers.
9+
"""
10+
11+
from test_framework.p2p import P2PInterface
12+
from test_framework.messages import (
13+
BlockTransactions,
14+
msg_cmpctblock,
15+
msg_block,
16+
msg_blocktxn,
17+
HeaderAndShortIDs,
18+
)
19+
from test_framework.test_framework import BitcoinTestFramework
20+
from test_framework.blocktools import (
21+
COINBASE_MATURITY,
22+
create_block,
23+
NORMAL_GBT_REQUEST_PARAMS,
24+
)
25+
from test_framework.util import assert_equal
26+
from test_framework.wallet import MiniWallet
27+
import copy
28+
29+
class MutatedBlocksTest(BitcoinTestFramework):
30+
def set_test_params(self):
31+
self.setup_clean_chain = True
32+
self.num_nodes = 1
33+
34+
def run_test(self):
35+
self.wallet = MiniWallet(self.nodes[0])
36+
self.generate(self.wallet, COINBASE_MATURITY)
37+
38+
honest_relayer = self.nodes[0].add_outbound_p2p_connection(P2PInterface(), p2p_idx=0, connection_type="outbound-full-relay")
39+
attacker = self.nodes[0].add_p2p_connection(P2PInterface())
40+
41+
# Create new block with two transactions (coinbase + 1 self-transfer).
42+
# The self-transfer transaction is needed to trigger a compact block
43+
# `getblocktxn` roundtrip.
44+
tx = self.wallet.create_self_transfer()["tx"]
45+
block = create_block(tmpl=self.nodes[0].getblocktemplate(NORMAL_GBT_REQUEST_PARAMS), txlist=[tx])
46+
block.solve()
47+
48+
# Create mutated version of the block by changing the transaction
49+
# version on the self-transfer. Dash has no witness data, so mutating a
50+
# transaction (which changes its hash and hence the block's merkle root)
51+
# is what trips the mutation check.
52+
mutated_block = copy.deepcopy(block)
53+
mutated_block.vtx[1].nVersion = 4
54+
55+
# Announce the new block via a compact block through the honest relayer
56+
cmpctblock = HeaderAndShortIDs()
57+
cmpctblock.initialize_from_block(block)
58+
honest_relayer.send_message(msg_cmpctblock(cmpctblock.to_p2p()))
59+
60+
# Wait for a `getblocktxn` that attempts to fetch the self-transfer
61+
def self_transfer_requested():
62+
if not honest_relayer.last_message.get('getblocktxn'):
63+
return False
64+
65+
get_block_txn = honest_relayer.last_message['getblocktxn']
66+
return (
67+
get_block_txn.block_txn_request.blockhash == block.sha256
68+
and get_block_txn.block_txn_request.indexes == [1]
69+
)
70+
honest_relayer.wait_until(self_transfer_requested, timeout=5)
71+
72+
# Block at height 101 should be the only one in flight from peer 0
73+
peer_info_prior_to_attack = self.nodes[0].getpeerinfo()
74+
assert_equal(peer_info_prior_to_attack[0]['id'], 0)
75+
assert_equal([101], peer_info_prior_to_attack[0]["inflight"])
76+
77+
# Attempt to clear the honest relayer's download request by sending the
78+
# mutated block (as the attacker).
79+
with self.nodes[0].assert_debug_log(expected_msgs=["Block mutated: bad-txnmrklroot, hashMerkleRoot mismatch"]):
80+
attacker.send_message(msg_block(mutated_block))
81+
# Attacker should get disconnected for sending a mutated block
82+
attacker.wait_for_disconnect(timeout=5)
83+
84+
# Block at height 101 should *still* be the only block in-flight from
85+
# peer 0
86+
peer_info_after_attack = self.nodes[0].getpeerinfo()
87+
assert_equal(peer_info_after_attack[0]['id'], 0)
88+
assert_equal([101], peer_info_after_attack[0]["inflight"])
89+
90+
# The honest relayer should be able to complete relaying the block by
91+
# sending the blocktxn that was requested.
92+
block_txn = msg_blocktxn()
93+
block_txn.block_transactions = BlockTransactions(blockhash=block.sha256, transactions=[tx])
94+
honest_relayer.send_and_ping(block_txn)
95+
assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.sha256)
96+
97+
98+
if __name__ == '__main__':
99+
MutatedBlocksTest().main()

test/functional/test_runner.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -331,6 +331,7 @@
331331
'p2p_leak.py',
332332
'p2p_compactblocks.py',
333333
'p2p_compactblocks_blocksonly.py',
334+
'p2p_mutated_blocks.py', # NOTE: needs dash_hash to pass
334335
'p2p_connect_to_devnet.py',
335336
'feature_sporks.py',
336337
'rpc_getblockstats.py',

0 commit comments

Comments
 (0)