Merge bitcoin/bitcoin#29412: p2p: Don't process mutated blocks

d8087ad [test] IsBlockMutated unit tests (dergoegge)
1ed2c98 Add transaction_identifier::size to allow Span conversion (dergoegge)
1ec6bbe [validation] Cache merkle root and witness commitment checks (dergoegge)
5bf4f5b [test] Add regression test for #27608 (dergoegge)
49257c0 [net processing] Don't process mutated blocks (dergoegge)
2d8495e [validation] Merkle root malleation should be caught by IsBlockMutated (dergoegge)
66abce1 [validation] Introduce IsBlockMutated (dergoegge)
e7669e1 [refactor] Cleanup merkle root checks (dergoegge)
95bddb9 [validation] Isolate merkle root checks (dergoegge)

Pull request description:

  This PR proposes to check for mutated blocks early as a defense-in-depth mitigation against attacks leveraging mutated blocks.

  We introduce `IsBlockMutated` which catches all known forms of block malleation and use it to do an early mutation check whenever we receive a `block` message.

  We have observed attacks that abused mutated blocks in the past, which could have been prevented by simply not processing mutated blocks (e.g. bitcoin/bitcoin#27608 for which a regression test is included in this PR).

ACKs for top commit:
  achow101:
    ACK d8087ad
  maflcko:
    ACK d8087ad 🏄
  fjahr:
    Code review ACK d8087ad
  sr-gi:
    Code review ACK bitcoin/bitcoin@d8087ad

Tree-SHA512: 618ff4ea7f168e10f07504d3651290efbb1bb2ab3b838ffff3527c028caf6c52dedad18d04d3dbc627977479710930e200f2dfae18a08f627efe7e64a57e535f

Author: achow101

src/net_processing.cpp

@@ -4719,6 +4719,16 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
 
         LogPrint(BCLog::NET, "received block %s peer=%d\n", pblock->GetHash().ToString(), pfrom.GetId());
 
+        const CBlockIndex* prev_block{WITH_LOCK(m_chainman.GetMutex(), return m_chainman.m_blockman.LookupBlockIndex(pblock->hashPrevBlock))};
+
+        if (IsBlockMutated(/*block=*/*pblock,
+                           /*check_witness_root=*/DeploymentActiveAfter(prev_block, m_chainman, Consensus::DEPLOYMENT_SEGWIT))) {
+            LogDebug(BCLog::NET, "Received mutated block from peer=%d\n", peer->m_id);
+            Misbehaving(*peer, 100, "mutated block");
+            WITH_LOCK(cs_main, RemoveBlockRequest(pblock->GetHash(), peer->m_id));
+            return;
+        }
+
         bool forceProcessing = false;
         const uint256 hash(pblock->GetHash());
         bool min_pow_checked = false;
@@ -4734,7 +4744,6 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
             mapBlockSource.emplace(hash, std::make_pair(pfrom.GetId(), true));
 
             // Check work on this block against our anti-dos thresholds.
-            const CBlockIndex* prev_block = m_chainman.m_blockman.LookupBlockIndex(pblock->hashPrevBlock);
             if (prev_block && prev_block->nChainWork + CalculateHeadersWork({pblock->GetBlockHeader()}) >= GetAntiDoSWorkThreshold()) {
                 min_pow_checked = true;
             }

src/primitives/block.h

@@ -71,8 +71,10 @@ class CBlock : public CBlockHeader
     // network and disk
     std::vector<CTransactionRef> vtx;
 
-    // memory only
-    mutable bool fChecked;
+    // Memory-only flags for caching expensive checks
+    mutable bool fChecked;                            // CheckBlock()
+    mutable bool m_checked_witness_commitment{false}; // CheckWitnessCommitment()
+    mutable bool m_checked_merkle_root{false};        // CheckMerkleRoot()
 
     CBlock()
     {
@@ -95,6 +97,8 @@ class CBlock : public CBlockHeader
         CBlockHeader::SetNull();
         vtx.clear();
         fChecked = false;
+        m_checked_witness_commitment = false;
+        m_checked_merkle_root = false;
     }
 
     CBlockHeader GetBlockHeader() const

src/test/validation_tests.cpp

@@ -4,12 +4,17 @@
 
 #include <chainparams.h>
 #include <consensus/amount.h>
+#include <consensus/merkle.h>
+#include <core_io.h>
+#include <hash.h>
 #include <net.h>
 #include <signet.h>
 #include <uint256.h>
 #include <util/chaintype.h>
 #include <validation.h>
 
+#include <string>
+
 #include <test/util/setup_common.h>
 
 #include <boost/test/unit_test.hpp>
@@ -145,4 +150,215 @@ BOOST_AUTO_TEST_CASE(test_assumeutxo)
     BOOST_CHECK_EQUAL(out110_2.nChainTx, 111U);
 }
 
+BOOST_AUTO_TEST_CASE(block_malleation)
+{
+    // Test utilities that calls `IsBlockMutated` and then clears the validity
+    // cache flags on `CBlock`.
+    auto is_mutated = [](CBlock& block, bool check_witness_root) {
+        bool mutated{IsBlockMutated(block, check_witness_root)};
+        block.fChecked = false;
+        block.m_checked_witness_commitment = false;
+        block.m_checked_merkle_root = false;
+        return mutated;
+    };
+    auto is_not_mutated = [&is_mutated](CBlock& block, bool check_witness_root) {
+        return !is_mutated(block, check_witness_root);
+    };
+
+    // Test utilities to create coinbase transactions and insert witness
+    // commitments.
+    //
+    // Note: this will not include the witness stack by default to avoid
+    // triggering the "no witnesses allowed for blocks that don't commit to
+    // witnesses" rule when testing other malleation vectors.
+    auto create_coinbase_tx = [](bool include_witness = false) {
+        CMutableTransaction coinbase;
+        coinbase.vin.resize(1);
+        if (include_witness) {
+            coinbase.vin[0].scriptWitness.stack.resize(1);
+            coinbase.vin[0].scriptWitness.stack[0] = std::vector<unsigned char>(32, 0x00);
+        }
+
+        coinbase.vout.resize(1);
+        coinbase.vout[0].scriptPubKey.resize(MINIMUM_WITNESS_COMMITMENT);
+        coinbase.vout[0].scriptPubKey[0] = OP_RETURN;
+        coinbase.vout[0].scriptPubKey[1] = 0x24;
+        coinbase.vout[0].scriptPubKey[2] = 0xaa;
+        coinbase.vout[0].scriptPubKey[3] = 0x21;
+        coinbase.vout[0].scriptPubKey[4] = 0xa9;
+        coinbase.vout[0].scriptPubKey[5] = 0xed;
+
+        auto tx = MakeTransactionRef(coinbase);
+        assert(tx->IsCoinBase());
+        return tx;
+    };
+    auto insert_witness_commitment = [](CBlock& block, uint256 commitment) {
+        assert(!block.vtx.empty() && block.vtx[0]->IsCoinBase() && !block.vtx[0]->vout.empty());
+
+        CMutableTransaction mtx{*block.vtx[0]};
+        CHash256().Write(commitment).Write(std::vector<unsigned char>(32, 0x00)).Finalize(commitment);
+        memcpy(&mtx.vout[0].scriptPubKey[6], commitment.begin(), 32);
+        block.vtx[0] = MakeTransactionRef(mtx);
+    };
+
+    {
+        CBlock block;
+
+        // Empty block is expected to have merkle root of 0x0.
+        BOOST_CHECK(block.vtx.empty());
+        block.hashMerkleRoot = uint256{1};
+        BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false));
+        block.hashMerkleRoot = uint256{};
+        BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/false));
+
+        // Block with a single coinbase tx is mutated if the merkle root is not
+        // equal to the coinbase tx's hash.
+        block.vtx.push_back(create_coinbase_tx());
+        BOOST_CHECK(block.vtx[0]->GetHash() != block.hashMerkleRoot);
+        BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false));
+        block.hashMerkleRoot = block.vtx[0]->GetHash();
+        BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/false));
+
+        // Block with two transactions is mutated if the merkle root does not
+        // match the double sha256 of the concatenation of the two transaction
+        // hashes.
+        block.vtx.push_back(MakeTransactionRef(CMutableTransaction{}));
+        BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false));
+        HashWriter hasher;
+        hasher.write(block.vtx[0]->GetHash());
+        hasher.write(block.vtx[1]->GetHash());
+        block.hashMerkleRoot = hasher.GetHash();
+        BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/false));
+
+        // Block with two transactions is mutated if any node is duplicate.
+        {
+            block.vtx[1] = block.vtx[0];
+            BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false));
+            HashWriter hasher;
+            hasher.write(block.vtx[0]->GetHash());
+            hasher.write(block.vtx[1]->GetHash());
+            block.hashMerkleRoot = hasher.GetHash();
+            BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false));
+        }
+
+        // Blocks with 64-byte coinbase transactions are not considered mutated
+        block.vtx.clear();
+        {
+            CMutableTransaction mtx;
+            mtx.vin.resize(1);
+            mtx.vout.resize(1);
+            mtx.vout[0].scriptPubKey.resize(4);
+            block.vtx.push_back(MakeTransactionRef(mtx));
+            block.hashMerkleRoot = block.vtx.back()->GetHash();
+            assert(block.vtx.back()->IsCoinBase());
+            assert(GetSerializeSize(TX_NO_WITNESS(block.vtx.back())) == 64);
+        }
+        BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/false));
+    }
+
+    {
+        // Test merkle root malleation
+
+        // Pseudo code to mine transactions tx{1,2,3}:
+        //
+        // ```
+        // loop {
+        //   tx1 = random_tx()
+        //   tx2 = random_tx()
+        //   tx3 = deserialize_tx(txid(tx1) || txid(tx2));
+        //   if serialized_size_without_witness(tx3) == 64 {
+        //     print(hex(tx3))
+        //     break
+        //   }
+        // }
+        // ```
+        //
+        // The `random_tx` function used to mine the txs below simply created
+        // empty transactions with a random version field.
+        CMutableTransaction tx1;
+        BOOST_CHECK(DecodeHexTx(tx1, "ff204bd0000000000000", /*try_no_witness=*/true, /*try_witness=*/false));
+        CMutableTransaction tx2;
+        BOOST_CHECK(DecodeHexTx(tx2, "8ae53c92000000000000", /*try_no_witness=*/true, /*try_witness=*/false));
+        CMutableTransaction tx3;
+        BOOST_CHECK(DecodeHexTx(tx3, "cdaf22d00002c6a7f848f8ae4d30054e61dcf3303d6fe01d282163341f06feecc10032b3160fcab87bdfe3ecfb769206ef2d991b92f8a268e423a6ef4d485f06", /*try_no_witness=*/true, /*try_witness=*/false));
+        {
+            // Verify that double_sha256(txid1||txid2) == txid3
+            HashWriter hasher;
+            hasher.write(tx1.GetHash());
+            hasher.write(tx2.GetHash());
+            assert(hasher.GetHash() == tx3.GetHash());
+            // Verify that tx3 is 64 bytes in size (without witness).
+            assert(GetSerializeSize(TX_NO_WITNESS(tx3)) == 64);
+        }
+
+        CBlock block;
+        block.vtx.push_back(MakeTransactionRef(tx1));
+        block.vtx.push_back(MakeTransactionRef(tx2));
+        uint256 merkle_root = block.hashMerkleRoot = BlockMerkleRoot(block);
+        BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/false));
+
+        // Mutate the block by replacing the two transactions with one 64-byte
+        // transaction that serializes into the concatenation of the txids of
+        // the transactions in the unmutated block.
+        block.vtx.clear();
+        block.vtx.push_back(MakeTransactionRef(tx3));
+        BOOST_CHECK(!block.vtx.back()->IsCoinBase());
+        BOOST_CHECK(BlockMerkleRoot(block) == merkle_root);
+        BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false));
+    }
+
+    {
+        CBlock block;
+        block.vtx.push_back(create_coinbase_tx(/*include_witness=*/true));
+        {
+            CMutableTransaction mtx;
+            mtx.vin.resize(1);
+            mtx.vin[0].scriptWitness.stack.resize(1);
+            mtx.vin[0].scriptWitness.stack[0] = {0};
+            block.vtx.push_back(MakeTransactionRef(mtx));
+        }
+        block.hashMerkleRoot = BlockMerkleRoot(block);
+        // Block with witnesses is considered mutated if the witness commitment
+        // is not validated.
+        BOOST_CHECK(is_mutated(block, /*check_witness_root=*/false));
+        // Block with invalid witness commitment is considered mutated.
+        BOOST_CHECK(is_mutated(block, /*check_witness_root=*/true));
+
+        // Block with valid commitment is not mutated
+        {
+            auto commitment{BlockWitnessMerkleRoot(block)};
+            insert_witness_commitment(block, commitment);
+            block.hashMerkleRoot = BlockMerkleRoot(block);
+        }
+        BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/true));
+
+        // Malleating witnesses should be caught by `IsBlockMutated`.
+        {
+            CMutableTransaction mtx{*block.vtx[1]};
+            assert(!mtx.vin[0].scriptWitness.stack[0].empty());
+            ++mtx.vin[0].scriptWitness.stack[0][0];
+            block.vtx[1] = MakeTransactionRef(mtx);
+        }
+        // Without also updating the witness commitment, the merkle root should
+        // not change when changing one of the witnesses.
+        BOOST_CHECK(block.hashMerkleRoot == BlockMerkleRoot(block));
+        BOOST_CHECK(is_mutated(block, /*check_witness_root=*/true));
+        {
+            auto commitment{BlockWitnessMerkleRoot(block)};
+            insert_witness_commitment(block, commitment);
+            block.hashMerkleRoot = BlockMerkleRoot(block);
+        }
+        BOOST_CHECK(is_not_mutated(block, /*check_witness_root=*/true));
+
+        // Test malleating the coinbase witness reserved value
+        {
+            CMutableTransaction mtx{*block.vtx[0]};
+            mtx.vin[0].scriptWitness.stack.resize(0);
+            block.vtx[0] = MakeTransactionRef(mtx);
+            block.hashMerkleRoot = BlockMerkleRoot(block);
+        }
+        BOOST_CHECK(is_mutated(block, /*check_witness_root=*/true));
+    }
+}
+
 BOOST_AUTO_TEST_SUITE_END()

src/util/transaction_identifier.h

@@ -44,6 +44,7 @@ class transaction_identifier
     constexpr void SetNull() { m_wrapped.SetNull(); }
     std::string GetHex() const { return m_wrapped.GetHex(); }
     std::string ToString() const { return m_wrapped.ToString(); }
+    static constexpr auto size() { return decltype(m_wrapped)::size(); }
     constexpr const std::byte* data() const { return reinterpret_cast<const std::byte*>(m_wrapped.data()); }
     constexpr const std::byte* begin() const { return reinterpret_cast<const std::byte*>(m_wrapped.begin()); }
     constexpr const std::byte* end() const { return reinterpret_cast<const std::byte*>(m_wrapped.end()); }