p2p: Don't process mutated blocks #29412
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
dergoegge
force-pushed
the
2024-01-mut-blocks
branch
from
56ed5bd
to
d028bb1
Compare
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
|
How do you define a mutated block? What are the known forms of mutated blocks? |
Looking at
|
|
Concept ACK |
|
Concept ACK. |
|
concept ACK might be good to recap why it was only added in BLOCK processing but not other |
We call |
|
I would like to see this in v27, can we add it to the milestone? |
There was a problem hiding this comment.
Concept ACK
"CBlock::GetHash() is a foot-gun without a prior mutation check", I hadn't felt this strongly about but I get it. I think it makes sense to rename it. I have drafted it here: fjahr@8e11e9c If it sounds valuable I will open a PR.
|
Concept ACK. Looking forward to addressing already pending comments, then i review. |
dergoegge
force-pushed
the
2024-01-mut-blocks
branch
from
d028bb1
to
0f356b0
Compare
| @@ -3758,6 +3814,40 @@ bool HasValidProofOfWork(const std::vector<CBlockHeader>& headers, const Consens | |||
| [&](const auto& header) { return CheckProofOfWork(header.GetHash(), header.nBits, consensusParams);}); | |||
| } | |||
|
|
|||
| bool IsBlockMutated(const CBlock& block, bool check_witness_root) | |||
There was a problem hiding this comment.
have you thought about having this function return an optional error string so unit tests can check expected failure reason?
There was a problem hiding this comment.
(reply to #29412 (comment))
note: unit tests may use the ASSERT_DEBUG_LOG, as an alternative.
There was a problem hiding this comment.
have you thought about having this function return an optional error string so unit tests can check expected failure reason?
I'm considering returning the mutation type but I will not be asserting logs...
| get_block_txn = honest_relayer.last_message['getblocktxn'] | ||
| return get_block_txn.block_txn_request.blockhash == block.sha256 and \ | ||
| get_block_txn.block_txn_request.indexes == [1] | ||
| honest_relayer.wait_until(self_transfer_requested, timeout=5) |
There was a problem hiding this comment.
in e2d1eb2
does this need a with p2p_lock?
There was a problem hiding this comment.
Does it? None of the other wait_for_* helpers require it.
|
I'm running this PR on my mainnet monitoring infrastructure as I was looking at the currently broadcast mutated blocks ( |
|
Received two |
There was a problem hiding this comment.
left some initial nits/questions
0f356b0 💬
Show signature
Signature:
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: 0f356b0b2fb23aef96ed7396890aa36410aa1d59 💬
kBfK9UKECARIswfAVfqnH6nAuO9mA7Tcad0J1J6aCTeTkHT4VK7z3GpY/0Wz6H0SeVaxH2Thgu3Qvz87+QF6CA==
| @@ -3758,6 +3814,40 @@ bool HasValidProofOfWork(const std::vector<CBlockHeader>& headers, const Consens | |||
| [&](const auto& header) { return CheckProofOfWork(header.GetHash(), header.nBits, consensusParams);}); | |||
| } | |||
|
|
|||
| bool IsBlockMutated(const CBlock& block, bool check_witness_root) | |||
There was a problem hiding this comment.
(reply to #29412 (comment))
note: unit tests may use the ASSERT_DEBUG_LOG, as an alternative.
dergoegge
force-pushed
the
2024-01-mut-blocks
branch
from
0f356b0
to
8c18b70
Compare
|
|
||
| // The malleation check is ignored; as the transaction tree itself | ||
| // already does not permit it, it is impossible to trigger in the | ||
| // witness tree. |
There was a problem hiding this comment.
future work nit: the mutated arg is never non-nullptr and has no test coverage it seems.
There was a problem hiding this comment.
future work nit: the mutated arg is never non-nullptr and has no test coverage it seems.
I presume the reason is that it can't be mutated and all callers are expected to pass nullptr? Seems fine to remove the arg, but should be fine either way.
…t to known prev block a1fbde0 p2p: Don't consider blocks mutated if they don't connect to known prev block (Greg Sanders) Pull request description: Followup to #29412 to revert some of the behavior change that was likely unintentional. Based on comments from #29412 (comment) ACKs for top commit: 0xB10C: utACK a1fbde0 dergoegge: Code review ACK a1fbde0 Sjors: ACK a1fbde0 sr-gi: tACK a1fbde0 Tree-SHA512: be6204c8cc57b271d55c1d02a5c77d03a37738d91cb5ac164f483b0bab3991c24679c5ff02fbaa52bf37ee625874b63f4c9f7b39ad6fd5f3a25386567a0942e4
Github-Pull: bitcoin#29412 Rebased-From: 95bddb9
Github-Pull: bitcoin#29412 Rebased-From: e7669e1
Github-Pull: bitcoin#29412 Rebased-From: 66abce1
Github-Pull: bitcoin#29412 Rebased-From: 2d8495e
We preemptively perform a block mutation check before further processing a block message (similar to early sanity checks on other messsage types). The main reasons for this change are as follows: - `CBlock::GetHash()` is a foot-gun without a prior mutation check, as the hash returned only commits to the header but not to the actual transactions (`CBlock::vtx`) contained in the block. - 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#27608). Github-Pull: bitcoin#29412 Rebased-From: 49257c0
Github-Pull: bitcoin#29412 Rebased-From: 5bf4f5b
Slight performance improvement by avoiding duplicate work. Github-Pull: bitcoin#29412 Rebased-From: 1ec6bbe
Github-Pull: bitcoin#29412 Rebased-From: d8087ad
|
Backported for 26.x in #29509 |
Github-Pull: bitcoin#29412 Rebased-From: 95bddb9
Github-Pull: bitcoin#29412 Rebased-From: 66abce1
Github-Pull: bitcoin#29412 Rebased-From: 2d8495e
We preemptively perform a block mutation check before further processing a block message (similar to early sanity checks on other messsage types). The main reasons for this change are as follows: - `CBlock::GetHash()` is a foot-gun without a prior mutation check, as the hash returned only commits to the header but not to the actual transactions (`CBlock::vtx`) contained in the block. - 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#27608). Github-Pull: bitcoin#29412 Rebased-From: 49257c0
Github-Pull: bitcoin#29412 Rebased-From: 5bf4f5b
Slight performance improvement by avoiding duplicate work. Github-Pull: bitcoin#29412 Rebased-From: 1ec6bbe
Github-Pull: bitcoin#29412 Rebased-From: d8087ad
|
Backported to 25.x in #29531 |
c68d4d0 [doc] update manual pages for v26.1rc2 (glozow) bd715bf [build] bump version to v26.1rc2 (glozow) b6d006d update release notes 26.1 (glozow) fce992b fuzz: restrict fopencookie usage to Linux & FreeBSD (fanquake) 40c56a4 test: make sure keypool sizes do not change on `getrawchangeaddress`/`getnewaddress` failures (UdjinM6) 7c82b27 wallet: Avoid updating `ReserveDestination::nIndex` when `GetReservedDestination` fails (UdjinM6) b5419ce p2p: Don't consider blocks mutated if they don't connect to known prev block (Greg Sanders) 0535c25 [test] IsBlockMutated unit tests (dergoegge) 8141498 [validation] Cache merkle root and witness commitment checks (dergoegge) 0c5c596 [test] Add regression test for #27608 (dergoegge) 2473635 [net processing] Don't process mutated blocks (dergoegge) 50c0b61 [validation] Merkle root malleation should be caught by IsBlockMutated (dergoegge) aff368f [validation] Introduce IsBlockMutated (dergoegge) 076c67c [refactor] Cleanup merkle root checks (dergoegge) 97a1d0a [validation] Isolate merkle root checks (dergoegge) 4ac0eb5 test: Drop `x` modifier in `fsbridge::fopen` call for mingw builds (Hennadii Stepanov) Pull request description: Includes: - #29357 - #29412 - #29524 - #29510 - #29529 Also does: - update to release notes - bump to rc2 - manpages - (no changes to bitcoin.conf) ACKs for top commit: achow101: ACK c68d4d0 Tree-SHA512: 2f8c3dd705e3f9f33403b3cc17e8006510ff827d7dbd609b09732a1669964e9b001cfecdc63d8d8daeb8f39c652e1e4ad0aac873d44d259c21803de85688ed2b
An unnecessary check was added to the block mutation tests in bitcoin#29412 where IsBlockMutated is returning true for the invalid reasons: we try to check mutation via transaction duplication, but the merkle root is not updated before the check, therefore the check fails because the provided root and the computed root differ, but not because the block contains the same transaction twice. The check is meaningless so it can be removed.
An unnecessary check was added to the block mutation tests in bitcoin#29412 where IsBlockMutated is returning true for the invalid reasons: we try to check mutation via transaction duplication, but the merkle root is not updated before the check, therefore the check fails because the provided root and the computed root differ, but not because the block contains the same transaction twice. The check is meaningless so it can be removed.
27cfda1 doc: Update release notes for 25.2rc2 (Ava Chow) daba5e2 doc: Update manpages for 25.2rc2 (Ava Chow) 8a0c980 build: Bump to 25.2rc2 (Ava Chow) cf7d3a8 p2p: Don't consider blocks mutated if they don't connect to known prev block (Greg Sanders) 3eaaafa [test] IsBlockMutated unit tests (dergoegge) 0667441 [validation] Cache merkle root and witness commitment checks (dergoegge) de97ecf [test] Add regression test for #27608 (dergoegge) 8cc4b24 [net processing] Don't process mutated blocks (dergoegge) 098f07d [validation] Merkle root malleation should be caught by IsBlockMutated (dergoegge) 8804c36 [validation] Introduce IsBlockMutated (dergoegge) 4f5baac [validation] Isolate merkle root checks (dergoegge) f93be01 test: make sure keypool sizes do not change on `getrawchangeaddress`/`getnewaddress` failures (UdjinM6) 7c08ccf wallet: Avoid updating `ReserveDestination::nIndex` when `GetReservedDestination` fails (UdjinM6) Pull request description: Backport: * #29510 * #29412 * #29524 ACKs for top commit: glozow: utACK 27cfda1 Tree-SHA512: 37feadd65d9ea55c0a92c9d2a6f74f87cafed3bc67f8deeaaafc5b7042f954e55ea34816612e1a49088f4f1906f104e00c7c3bec7affd1c1f48220b57a8769c5
This PR proposes to check for mutated blocks early as a defense-in-depth mitigation against attacks leveraging mutated blocks.
We introduce
IsBlockMutatedwhich catches all known forms of block malleation and use it to do an early mutation check whenever we receive ablockmessage.We have observed attacks that abused mutated blocks in the past, which could have been prevented by simply not processing mutated blocks (e.g. #27608 for which a regression test is included in this PR).