wallet2: validate fetched block height and parent hash #9344
Conversation
|
Does this have an impact on wallet scan speed? |
No, it doesn't affect performance at all. |
|
@valldrac some tests failed, can you take a look? https://github.com/monero-project/monero/actions/runs/9274388707/job/25517494248?pr=9344 |
valldrac
force-pushed
the
validate-block-parent-hash
branch
from
c2529cf
to
4d6f781
Compare
Fixed. I had assumed |
|
Hi. Just a friendly ping to request a review and merge for this PR. It addresses an important issue with the blockchain integrity during refresh in wallet2. No impact on wallet scan speed, and tests have passed. |
| @@ -3110,11 +3114,10 @@ void wallet2::pull_hashes(uint64_t start_height, uint64_t &blocks_start_height, | |||
| //---------------------------------------------------------------------------------------------------- | |||
| void wallet2::process_parsed_blocks(uint64_t start_height, const std::vector<cryptonote::block_complete_entry> &blocks, const std::vector<parsed_block> &parsed_blocks, uint64_t& blocks_added, std::map<std::pair<uint64_t, uint64_t>, size_t> *output_tracker_cache) | |||
| { | |||
| size_t current_index = start_height; | |||
There was a problem hiding this comment.
I understand why you moved current_index to where it's used the first time, but this is an unrelated change for this PR. As a result, half of the changes in this function are unnecessary.
Patches should be self contained. A good rule of thumb is to have one patch per separate issue, feature, or logical change. Also, no other changes, such as random whitespace changes, reindentation, or fixing typos, spelling, or wording, unless user visible. Following the code style of the particular chunk of code you're modifying is encouraged. Proper squashing should be done (eg, if you're making a buggy patch, then a later patch to fix the bug, both patches should be merged).
There was a problem hiding this comment.
I believe this change is necessary to prevent future issues. I had to choose between two variables, neither of which was correct for my PR needs. is_in_bounds(start_height) must be called before start_height is used, but it was accessed via current_index, which is mutated later in the function. This mix-up could lead to future issues.
Such a trivial change is easy to review and quick to fix in any merge conflict. Leaving it as is would be incorrect, because the current code already mixes mutable and immutable variables. Probably reviewing the commit history would show that this mix-up comes from previous merges that made minimal changes without considering the context.
I can split the commit in two to clarify the change if necessary, but creating another PR only for this seems excessive.
There was a problem hiding this comment.
Splitting into two commits is fine. And while you're at it, start_height should then be declared as const uint64_t in function definition and declaration, if it's not supposed to be changed in the function.
There was a problem hiding this comment.
Sure, I've split into two commits and declared start_height as const.
What prevents a malicious node from forging |
Yes, wallet2 already hashes the block internally when it calls |
valldrac
force-pushed
the
validate-block-parent-hash
branch
from
4d6f781
to
377445c
Compare
| @@ -3125,8 +3128,20 @@ void wallet2::process_parsed_blocks(uint64_t start_height, const std::vector<cry | |||
| num_txes += 1 + parsed_blocks[i].txes.size(); | |||
| tx_cache_data.resize(num_txes); | |||
| size_t txidx = 0; | |||
| crypto::hash prev_block_id; | |||
| if (start_height > 0) { | |||
| prev_block_id = m_blockchain[start_height - 1]; | |||
There was a problem hiding this comment.
There is a check above that start_height is in bounds, but you never check that start_height - 1 is in bounds. It must be checked too.
There was a problem hiding this comment.
I guess you just shouldn't init prev_block_id if it's not in bounds, instead of throwing an exception like above.
There was a problem hiding this comment.
You're right. It could fail if the blockchain is trimmed to one block. I just fixed it.
valldrac
force-pushed
the
validate-block-parent-hash
branch
from
377445c
to
a026d5a
Compare
|
@tevador any opinion on this? |
This PR adds integrity checks in wallet2 to ensure that blocks fetched from remote nodes during the refresh process align correctly with the wallet's local view of the blockchain.
It mitigates some of the risks described in the security advisory (link) regarding the trust in remote nodes for blockchain synchronization, without needing to verify the PoW.
Currently, wallet2 allows malicious nodes to provide a forged (non-mined) block at a specific height when requested by the wallet, while providing genuine blocks thereafter. All this without triggering a blockchain split, maintaining the appearance of the real blockchain, and avoiding later detection. This is possible because wallet2 fails to verify whether each block's parent hash (field
prev_idin the block header) matches the hash of the previous block in the chain.With these new basic checks, any fake block injected will be detected upon connection to an honest node. This detection will trigger either a reorganization (within the
max-reorg-depthlimit) or a fatal error to prevent further compromise.Note that any injection attack that occurred prior to this PR can only be detected by issuing a rescan of the wallet.