Harden block validation and snapshot load paths#299
Conversation
- Remove the early-return gate in verify_basic_proper_block_invariants
that skipped all QC-claim and block_extensions checks for block 2;
switch the QC-claim reference from prev.header.qc_claim to the
authoritative prev.core.latest_qc_claim(), with a new snapshot
invariant asserting their equality for non-genesis blocks.
- Factor finalizer_policy::validate() and proposer_policy::validate()
as shared structural-validation methods (uniqueness, bounds, BFT
threshold) reused by both the set_finalizers / set_proposed_producers
intrinsics and snapshot loading. Closes drift risk between the two
paths. Snapshot null-pointer guards now run before member-init-list
code that dereferences active_finalizer_policy.
- Extract validate_s_root_extensions_match() from the inline
nested-lambda loop in apply_block into a named helper with 10
dedicated unit tests.
- Add duplicate-detection in check_protocol_features for the
within-block activation list.
- Reorder qc_t::verify_basic so pending verify_vote_format runs
before verify_dual_finalizers_votes.
- Swap += to |= in block_header::calculate_id for clarity.
- Switch std::accumulate init to uint64_t{0} in
max_weak_sum_before_weak_final.
- 40+ new unit tests across finalizer intrinsic negatives, producer
schedule negatives, merkle async-determinism, signature-recovery
dedup regression, s_root_extension matching, and snapshot invariant
checks.
| } | ||
|
|
||
| // Now safe to compute state that depends on active_finalizer_policy. | ||
| strong_digest = compute_finality_digest(); |
There was a problem hiding this comment.
This computes strong_digest from snapshot state before validating core at block_state.cpp (line 224), and before checking activated_protocol_features. compute_finality_digest() can reach core.latest_qc_claim() and compute_base_digest(), which asserts then dereferences activated_protocol_features at block_header_state.cpp (line 32). A malformed snapshot with a null activated_protocol_features pointer or invalid/empty finality core can still assert/segfault before the intended snapshot_exception. Move core.validate_snapshot() and an explicit activated_protocol_features null check before digest computation, and add regression coverage for both cases.
…atch_rejected controller::head() returns block_handle by value; binding the get_bsp() reference directly to the temporary leaves live_bsp pointing at a destroyed shared_ptr. gcc happened to leave the stack intact; clang/ASan/UBSan/asserton reused it and the BOOST_REQUIRE on core.latest_qc_claim() == header.qc_claim read garbage. Bind the head to a named local first (same pattern as finality_proof.hpp:315).
…ing digests compute_finality_digest() reaches core.latest_qc_claim() and core.get_block_reference() (both assert non-empty / in-bounds), then calls compute_base_digest() which dereferences activated_protocol_features. A tampered snapshot planting nulls or a malformed core would assert/segfault before the ctor could throw snapshot_exception. Add an explicit activated_protocol_features null-check to the null-guard group, and move core.validate_snapshot() ahead of the digest computation. Cover both with regression tests.
| validate_prop_pol(*latest_pending_proposer_policy, "latest_pending"); | ||
| } | ||
|
|
||
| // Snapshot hardening: validate valid_t |
There was a problem hiding this comment.
This only validates valid_t when valid is present, but it never rejects valid == std::nullopt for a non-genesis snapshot. The new comment in block_state.cpp (line 365) says null valid is legitimate only for the Savanna genesis core, yet the snapshot constructor accepts a tampered non-genesis snapshot with sbs.valid.reset(). In release builds, get_validation_mroot() then returns an empty digest after an assert(core.is_genesis_core()), so the malformed snapshot is detected too late or only indirectly. Add a constructor check like SYS_ASSERT(valid || core.is_genesis_core(), snapshot_exception, ...), plus a regression test.
The snapshot block_state ctor validated valid_t only when it was present, so a tampered or truncated snapshot carrying a null valid passed construction. get_validation_mroot() would then return an empty digest after a debug-only assert(core.is_genesis_core()), so in release builds the corruption surfaced late and indirectly as a finality_mroot mismatch in apply_block. A snapshot is never taken at genesis, so every snapshot block_state must carry a valid_t finality structure. Promote the check to an unconditional SYS_ASSERT that throws snapshot_exception, and add a regression test.
| // (see verify_basic_proper_block_invariants). For every non-genesis block it equals | ||
| // header.qc_claim by construction (finality_core::next sets latest_qc_claim from the | ||
| // header's claim). Genesis intentionally differs: core is {1, false}, header is {0, false}. | ||
| SYS_ASSERT(core.is_genesis_core() || core.latest_qc_claim() == header.qc_claim, snapshot_exception, |
There was a problem hiding this comment.
This lets any refs.empty() core bypass the core.latest_qc_claim() == header.qc_claim check. A tampered non-genesis snapshot can replace the head’s core with a one-link “genesis core” for that block number, trim valid.validation_mroots to the expected size, and avoid the new QC/header consistency check. The exemption should be tied to the actual genesis block, not just core.is_genesis_core().
There was a problem hiding this comment.
Fixed: 324435f612 -- dropped the is_genesis_core() exemption entirely and added a guard that rejects block_num() == 1. Snapshots of the genesis block aren't a legitimate input (genesis state is reconstructed from genesis_state via initialize_blockchain_state), so the underlying refs.empty() bypass surface is gone rather than narrowed.
|
|
||
| block_state::block_state(snapshot_detail::snapshot_block_state_v1&& sbs) | ||
| : block_header_state { | ||
| .block_id = sbs.block_id, |
There was a problem hiding this comment.
This still trusts sbs.block_id without checking it equals header.calculate_id(). id() and fork-db indexing then use that stored value as authoritative, so a malformed snapshot can load a block_state whose identity does not match its header. The constructor should reject block_id != header.calculate_id() before exposing the state as chain_head.
block_state(snapshot_block_state_v1) now requires sbs.block_id == header.calculate_id() and rejects block_num() == 1. The first check closes a forge-the-identity attack where a tampered snapshot can pin a chosen block_id to a real header -- fork_db indexing and id() consumers trust the stored value as authoritative. The second check drops the prior is_genesis_core() exemption on the qc_claim consistency check. That exemption fired on refs.empty() alone, which an attacker can synthesize on a non-genesis block: a single src==tgt link satisfies validate_snapshot's invariant 3, so the qc_claim check was bypassed. Genesis state is reconstructed from genesis_state via initialize_blockchain_state -- it is not a legitimate snapshot input. The qc_claim-mismatch regression is updated to re-sync block_id after tampering header so the new block_id check does not fire first. The prior "genesis is allowed" test is replaced with one that asserts genesis is rejected. Adds: snapshot_synthetic_genesis_core_rejected (refs.empty() bypass blocked) and snapshot_block_id_header_mismatch_rejected (forged block_id blocked).
2 participants
Summary
verify_basic_proper_block_invariants, usecore.latest_qc_claim()as authoritative QC-claim referencefinalizer_policy::validate()/proposer_policy::validate()as shared structural-validation methods reused by intrinsics and snapshot loadingvalidate_s_root_extensions_match()helper from inline 39-line nested-lambda loop inapply_blockcheck_protocol_featuresfor within-block activation listqc_t::verify_basicso pendingverify_vote_formatruns beforeverify_dual_finalizers_votes