From 95bddb930aa72edd40fdff52cf447202995b0dce Mon Sep 17 00:00:00 2001 From: dergoegge Date: Fri, 19 Jan 2024 11:49:48 +0000 Subject: [validation] Isolate merkle root checks --- src/validation.cpp | 90 +++++++++++++++++++++++++++++++----------------------- 1 file changed, 52 insertions(+), 38 deletions(-) (limited to 'src/validation.cpp') diff --git a/src/validation.cpp b/src/validation.cpp index caa4ff3115..98383cd133 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3639,6 +3639,54 @@ static bool CheckBlockHeader(const CBlockHeader& block, BlockValidationState& st return true; } +static bool CheckMerkleRoot(const CBlock& block, BlockValidationState& state) +{ + bool mutated; + uint256 hashMerkleRoot2 = BlockMerkleRoot(block, &mutated); + if (block.hashMerkleRoot != hashMerkleRoot2) + return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-txnmrklroot", "hashMerkleRoot mismatch"); + + // Check for merkle tree malleability (CVE-2012-2459): repeating sequences + // of transactions in a block without affecting the merkle root of a block, + // while still invalidating it. + if (mutated) + return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-txns-duplicate", "duplicate transaction"); + + return true; +} + +static bool CheckWitnessMalleation(const CBlock& block, bool expect_witness_commitment, BlockValidationState& state) +{ + if (expect_witness_commitment) { + int commitpos = GetWitnessCommitmentIndex(block); + if (commitpos != NO_WITNESS_COMMITMENT) { + bool malleated = false; + uint256 hashWitness = BlockWitnessMerkleRoot(block, &malleated); + // The malleation check is ignored; as the transaction tree itself + // already does not permit it, it is impossible to trigger in the + // witness tree. + if (block.vtx[0]->vin[0].scriptWitness.stack.size() != 1 || block.vtx[0]->vin[0].scriptWitness.stack[0].size() != 32) { + return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-witness-nonce-size", strprintf("%s : invalid witness reserved value size", __func__)); + } + CHash256().Write(hashWitness).Write(block.vtx[0]->vin[0].scriptWitness.stack[0]).Finalize(hashWitness); + if (memcmp(hashWitness.begin(), &block.vtx[0]->vout[commitpos].scriptPubKey[6], 32)) { + return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-witness-merkle-match", strprintf("%s : witness merkle commitment mismatch", __func__)); + } + + return true; + } + } + + // No witness data is allowed in blocks that don't commit to witness data, as this would otherwise leave room for spam + for (const auto& tx : block.vtx) { + if (tx->HasWitness()) { + return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "unexpected-witness", strprintf("%s : unexpected witness data found", __func__)); + } + } + + return true; +} + bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW, bool fCheckMerkleRoot) { // These are checks that are independent of context. @@ -3657,17 +3705,8 @@ bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensu } // Check the merkle root. - if (fCheckMerkleRoot) { - bool mutated; - uint256 hashMerkleRoot2 = BlockMerkleRoot(block, &mutated); - if (block.hashMerkleRoot != hashMerkleRoot2) - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-txnmrklroot", "hashMerkleRoot mismatch"); - - // Check for merkle tree malleability (CVE-2012-2459): repeating sequences - // of transactions in a block without affecting the merkle root of a block, - // while still invalidating it. - if (mutated) - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-txns-duplicate", "duplicate transaction"); + if (fCheckMerkleRoot && !CheckMerkleRoot(block, state)) { + return false; } // All potential-corruption validation must be done before we do any @@ -3866,33 +3905,8 @@ static bool ContextualCheckBlock(const CBlock& block, BlockValidationState& stat // * There must be at least one output whose scriptPubKey is a single 36-byte push, the first 4 bytes of which are // {0xaa, 0x21, 0xa9, 0xed}, and the following 32 bytes are SHA256^2(witness root, witness reserved value). In case there are // multiple, the last one is used. - bool fHaveWitness = false; - if (DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_SEGWIT)) { - int commitpos = GetWitnessCommitmentIndex(block); - if (commitpos != NO_WITNESS_COMMITMENT) { - bool malleated = false; - uint256 hashWitness = BlockWitnessMerkleRoot(block, &malleated); - // The malleation check is ignored; as the transaction tree itself - // already does not permit it, it is impossible to trigger in the - // witness tree. - if (block.vtx[0]->vin[0].scriptWitness.stack.size() != 1 || block.vtx[0]->vin[0].scriptWitness.stack[0].size() != 32) { - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-witness-nonce-size", strprintf("%s : invalid witness reserved value size", __func__)); - } - CHash256().Write(hashWitness).Write(block.vtx[0]->vin[0].scriptWitness.stack[0]).Finalize(hashWitness); - if (memcmp(hashWitness.begin(), &block.vtx[0]->vout[commitpos].scriptPubKey[6], 32)) { - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-witness-merkle-match", strprintf("%s : witness merkle commitment mismatch", __func__)); - } - fHaveWitness = true; - } - } - - // No witness data is allowed in blocks that don't commit to witness data, as this would otherwise leave room for spam - if (!fHaveWitness) { - for (const auto& tx : block.vtx) { - if (tx->HasWitness()) { - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "unexpected-witness", strprintf("%s : unexpected witness data found", __func__)); - } - } + if (!CheckWitnessMalleation(block, DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_SEGWIT), state)) { + return false; } // After the coinbase witness reserved value and commitment are verified, -- cgit v1.2.3 From e7669e1343aec4298fd30d539847963e6fa5619c Mon Sep 17 00:00:00 2001 From: dergoegge Date: Thu, 22 Feb 2024 11:27:49 +0000 Subject: [refactor] Cleanup merkle root checks --- src/validation.cpp | 55 ++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 14 deletions(-) (limited to 'src/validation.cpp') diff --git a/src/validation.cpp b/src/validation.cpp index 98383cd133..e02ce2ad1d 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3642,35 +3642,59 @@ static bool CheckBlockHeader(const CBlockHeader& block, BlockValidationState& st static bool CheckMerkleRoot(const CBlock& block, BlockValidationState& state) { bool mutated; - uint256 hashMerkleRoot2 = BlockMerkleRoot(block, &mutated); - if (block.hashMerkleRoot != hashMerkleRoot2) - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-txnmrklroot", "hashMerkleRoot mismatch"); + uint256 merkle_root = BlockMerkleRoot(block, &mutated); + if (block.hashMerkleRoot != merkle_root) { + return state.Invalid( + /*result=*/BlockValidationResult::BLOCK_MUTATED, + /*reject_reason=*/"bad-txnmrklroot", + /*debug_message=*/"hashMerkleRoot mismatch"); + } // Check for merkle tree malleability (CVE-2012-2459): repeating sequences // of transactions in a block without affecting the merkle root of a block, // while still invalidating it. - if (mutated) - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-txns-duplicate", "duplicate transaction"); + if (mutated) { + return state.Invalid( + /*result=*/BlockValidationResult::BLOCK_MUTATED, + /*reject_reason=*/"bad-txns-duplicate", + /*debug_message=*/"duplicate transaction"); + } return true; } +/** CheckWitnessMalleation performs checks for block malleation with regard to + * its witnesses. + * + * Note: If the witness commitment is expected (i.e. `expect_witness_commitment + * = true`), then the block is required to have at least one transaction and the + * first transaction needs to have at least one input. */ static bool CheckWitnessMalleation(const CBlock& block, bool expect_witness_commitment, BlockValidationState& state) { if (expect_witness_commitment) { int commitpos = GetWitnessCommitmentIndex(block); if (commitpos != NO_WITNESS_COMMITMENT) { - bool malleated = false; - uint256 hashWitness = BlockWitnessMerkleRoot(block, &malleated); + assert(!block.vtx.empty() && !block.vtx[0]->vin.empty()); + const auto& witness_stack{block.vtx[0]->vin[0].scriptWitness.stack}; + + if (witness_stack.size() != 1 || witness_stack[0].size() != 32) { + return state.Invalid( + /*result=*/BlockValidationResult::BLOCK_MUTATED, + /*reject_reason=*/"bad-witness-nonce-size", + /*debug_message=*/strprintf("%s : invalid witness reserved value size", __func__)); + } + // The malleation check is ignored; as the transaction tree itself // already does not permit it, it is impossible to trigger in the // witness tree. - if (block.vtx[0]->vin[0].scriptWitness.stack.size() != 1 || block.vtx[0]->vin[0].scriptWitness.stack[0].size() != 32) { - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-witness-nonce-size", strprintf("%s : invalid witness reserved value size", __func__)); - } - CHash256().Write(hashWitness).Write(block.vtx[0]->vin[0].scriptWitness.stack[0]).Finalize(hashWitness); - if (memcmp(hashWitness.begin(), &block.vtx[0]->vout[commitpos].scriptPubKey[6], 32)) { - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-witness-merkle-match", strprintf("%s : witness merkle commitment mismatch", __func__)); + uint256 hash_witness = BlockWitnessMerkleRoot(block, /*mutated=*/nullptr); + + CHash256().Write(hash_witness).Write(witness_stack[0]).Finalize(hash_witness); + if (memcmp(hash_witness.begin(), &block.vtx[0]->vout[commitpos].scriptPubKey[6], 32)) { + return state.Invalid( + /*result=*/BlockValidationResult::BLOCK_MUTATED, + /*reject_reason=*/"bad-witness-merkle-match", + /*debug_message=*/strprintf("%s : witness merkle commitment mismatch", __func__)); } return true; @@ -3680,7 +3704,10 @@ static bool CheckWitnessMalleation(const CBlock& block, bool expect_witness_comm // No witness data is allowed in blocks that don't commit to witness data, as this would otherwise leave room for spam for (const auto& tx : block.vtx) { if (tx->HasWitness()) { - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "unexpected-witness", strprintf("%s : unexpected witness data found", __func__)); + return state.Invalid( + /*result=*/BlockValidationResult::BLOCK_MUTATED, + /*reject_reason=*/"unexpected-witness", + /*debug_message=*/strprintf("%s : unexpected witness data found", __func__)); } } -- cgit v1.2.3 From 66abce1d98115e41f394bc4f4f52341960ddc839 Mon Sep 17 00:00:00 2001 From: dergoegge Date: Fri, 19 Jan 2024 15:09:28 +0000 Subject: [validation] Introduce IsBlockMutated --- src/validation.cpp | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) (limited to 'src/validation.cpp') diff --git a/src/validation.cpp b/src/validation.cpp index e02ce2ad1d..e1133138df 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3824,6 +3824,26 @@ bool HasValidProofOfWork(const std::vector& headers, const Consens [&](const auto& header) { return CheckProofOfWork(header.GetHash(), header.nBits, consensusParams);}); } +bool IsBlockMutated(const CBlock& block, bool check_witness_root) +{ + BlockValidationState state; + if (!CheckMerkleRoot(block, state)) { + LogDebug(BCLog::VALIDATION, "Block mutated: %s\n", state.ToString()); + return true; + } + + if (block.vtx.empty() || !block.vtx[0]->IsCoinBase()) { + return false; + } + + if (!CheckWitnessMalleation(block, check_witness_root, state)) { + LogDebug(BCLog::VALIDATION, "Block mutated: %s\n", state.ToString()); + return true; + } + + return false; +} + arith_uint256 CalculateHeadersWork(const std::vector& headers) { arith_uint256 total_work{0}; -- cgit v1.2.3 From 2d8495e0800f5332748cd50eaad801ff77671bba Mon Sep 17 00:00:00 2001 From: dergoegge Date: Tue, 6 Feb 2024 17:07:48 +0000 Subject: [validation] Merkle root malleation should be caught by IsBlockMutated --- src/validation.cpp | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) (limited to 'src/validation.cpp') diff --git a/src/validation.cpp b/src/validation.cpp index e1133138df..120663bfc0 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3833,7 +3833,18 @@ bool IsBlockMutated(const CBlock& block, bool check_witness_root) } if (block.vtx.empty() || !block.vtx[0]->IsCoinBase()) { - return false; + // Consider the block mutated if any transaction is 64 bytes in size (see 3.1 + // in "Weaknesses in Bitcoin’s Merkle Root Construction": + // https://lists.linuxfoundation.org/pipermail/bitcoin-dev/attachments/20190225/a27d8837/attachment-0001.pdf). + // + // Note: This is not a consensus change as this only applies to blocks that + // don't have a coinbase transaction and would therefore already be invalid. + return std::any_of(block.vtx.begin(), block.vtx.end(), + [](auto& tx) { return GetSerializeSize(TX_NO_WITNESS(tx)) == 64; }); + } else { + // Theoretically it is still possible for a block with a 64 byte + // coinbase transaction to be mutated but we neglect that possibility + // here as it requires at least 224 bits of work. } if (!CheckWitnessMalleation(block, check_witness_root, state)) { -- cgit v1.2.3 From 1ec6bbeb8d27d31647d1433ccb87b362f6d81f90 Mon Sep 17 00:00:00 2001 From: dergoegge Date: Tue, 6 Feb 2024 10:07:18 +0000 Subject: [validation] Cache merkle root and witness commitment checks Slight performance improvement by avoiding duplicate work. --- src/validation.cpp | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'src/validation.cpp') diff --git a/src/validation.cpp b/src/validation.cpp index 120663bfc0..b03cc7f78a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3641,6 +3641,8 @@ static bool CheckBlockHeader(const CBlockHeader& block, BlockValidationState& st static bool CheckMerkleRoot(const CBlock& block, BlockValidationState& state) { + if (block.m_checked_merkle_root) return true; + bool mutated; uint256 merkle_root = BlockMerkleRoot(block, &mutated); if (block.hashMerkleRoot != merkle_root) { @@ -3660,6 +3662,7 @@ static bool CheckMerkleRoot(const CBlock& block, BlockValidationState& state) /*debug_message=*/"duplicate transaction"); } + block.m_checked_merkle_root = true; return true; } @@ -3672,6 +3675,8 @@ static bool CheckMerkleRoot(const CBlock& block, BlockValidationState& state) static bool CheckWitnessMalleation(const CBlock& block, bool expect_witness_commitment, BlockValidationState& state) { if (expect_witness_commitment) { + if (block.m_checked_witness_commitment) return true; + int commitpos = GetWitnessCommitmentIndex(block); if (commitpos != NO_WITNESS_COMMITMENT) { assert(!block.vtx.empty() && !block.vtx[0]->vin.empty()); @@ -3697,6 +3702,7 @@ static bool CheckWitnessMalleation(const CBlock& block, bool expect_witness_comm /*debug_message=*/strprintf("%s : witness merkle commitment mismatch", __func__)); } + block.m_checked_witness_commitment = true; return true; } } -- cgit v1.2.3