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. |
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 |
glozow
left a comment
There was a problem hiding this comment.
concept ACK, I agree with the approach of catching and dropping these earlier rather than later
|
I would like to see this in v27, can we add it to the milestone? |
fjahr
left a comment
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. |
d028bb1 to
0f356b0
Compare
src/validation.cpp
Outdated
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...
There was a problem hiding this comment.
in e2d1eb2e2001c31b43154df47ab5be215a26774f
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 |
maflcko
left a comment
There was a problem hiding this comment.
left some initial nits/questions
0f356b0b2fb23aef96ed7396890aa36410aa1d59 💬
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==
src/validation.cpp
Outdated
There was a problem hiding this comment.
(reply to #29412 (comment))
note: unit tests may use the ASSERT_DEBUG_LOG, as an alternative.
0f356b0 to
8c18b70
Compare
src/validation.cpp
Outdated
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.
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 bitcoin#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#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@d8087ad Tree-SHA512: 618ff4ea7f168e10f07504d3651290efbb1bb2ab3b838ffff3527c028caf6c52dedad18d04d3dbc627977479710930e200f2dfae18a08f627efe7e64a57e535f
Since bitcoin#29412, we have not allowed mutated blocks to continue being processed immediately the block is received, but this is only done for the legacy BLOCK message. Extend these checks as belt-and-suspenders to not allow similar mutation strategies to affect relay by honest peers by applying the check inside PartiallyDownloadedBlock::FillBlock, immediately before returning READ_STATUS_OK. This also removes the extraneous CheckBlock call. Github-Pull: bitcoin#32646 Rebased-From: bac9ee4
Summary: Add a test to show that an attacker cannot clear the download state for other peers by send an invalid mutated block. This is a partial backport of [[bitcoin/bitcoin#29412 | core#29412]] bitcoin/bitcoin@5bf4f5b Test Plan: Run this test before and after D16879, to show that the test failed before and works after this diff ``` git checkout 2701a514b014122ad024b6d2cf9e0098cd7adc5b ninja && test/functional/test_runner.py p2p_mutated_blocks git checkout 5a3dd8183dfc2459aa976deccff2b4f03a737180 ninja && test/functional/test_runner.py p2p_mutated_blocks ``` A minor patch is required in blocktools.py to make this test work on those old commits: add the `txlist` argument to `create_coinbase` (see D17691) Reviewers: #bitcoin_abc, roqqit Reviewed By: #bitcoin_abc, roqqit Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D18005
Summary: > 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/bitcoin#27608). PR 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. #27608 for which a regression test is included in this PR). This concludes backport of [[bitcoin/bitcoin#29412 | core#29412]] bitcoin/bitcoin@95bddb9 bitcoin/bitcoin@e7669e1 bitcoin/bitcoin@66abce1 bitcoin/bitcoin@2d8495e bitcoin/bitcoin@49257c0 bitcoin/bitcoin@1ec6bbe bitcoin/bitcoin@d8087ad Depends on D18005 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D18006
|
Past reviewers may be interested in the cleanup followup #33805. |
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).