Harden snapshot deserialization against crafted snapshots#275
Open
Harden snapshot deserialization against crafted snapshots#275
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens pre-attestation snapshot loading by adding structural invariant validation, while also migrating contract storage/state-history plumbing from legacy db_* tables to the new KV database intrinsics (and updating tests/contracts accordingly).
Changes:
- Add snapshot-time invariant validation (resource limits, finality core, finalizer policies, permissions, snapshot reader bounds).
- Introduce KV database intrinsics + iterator/index support; remove legacy db/table deltas and emit KV-compatible SHiP deltas.
- Update system/test contracts, ABIs, plugins, and unit/integration tests to use KV storage and new delta types/APIs.
Reviewed changes
Copilot reviewed 142 out of 206 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| unittests/test-contracts/test_kv_api/CMakeLists.txt | Adds build/copy rules for new KV test contract |
| unittests/test-contracts/test_api_multi_index/test_api_multi_index.hpp | Removes legacy test contract header |
| unittests/test-contracts/test_api_multi_index/test_api_multi_index.abi | Removes legacy ABI artifact |
| unittests/test-contracts/test_api_multi_index/CMakeLists.txt | Removes legacy build rules |
| unittests/test-contracts/test_api_db/test_api_db.hpp | Removes legacy test contract header |
| unittests/test-contracts/test_api_db/test_api_db.abi | Removes legacy ABI artifact |
| unittests/test-contracts/test_api_db/CMakeLists.txt | Removes legacy build rules |
| unittests/test-contracts/test_api/test_transaction.cpp | Switches a test to KV upsert storage |
| unittests/test-contracts/test_api/test_permission.cpp | Switches permission test storage to KV upsert |
| unittests/test-contracts/test_api/test_chain.cpp | Updates RAM billing test to KV object billing model |
| unittests/test-contracts/test_api/test_api.hpp | Replaces legacy db intrinsics with KV intrinsics + key helper |
| unittests/test-contracts/test_api/test_action.cpp | Updates CFA forbidden-calls tests to KV intrinsics |
| unittests/test-contracts/snapshot_test/snapshot_test.abi | Adds ABI key metadata for KV-compatible table keys |
| unittests/test-contracts/savanna/ibc/ibc.cpp | Fixes pre-decrement formatting/portability issue |
| unittests/test-contracts/savanna/ibc/ibc.abi | Adds ABI key metadata for KV-compatible table keys |
| unittests/test-contracts/ram_restrictions_test/ram_restrictions_test.abi | Adds ABI key metadata for KV-compatible table keys |
| unittests/test-contracts/no_auth_table/no_auth_table.abi | Adds ABI key metadata for KV-compatible table keys |
| unittests/test-contracts/integration_test/integration_test.abi | Adds ABI key metadata for KV-compatible table keys |
| unittests/test-contracts/get_table_test/get_table_test.abi | Adds ABI key metadata for KV-compatible table keys |
| unittests/test-contracts/get_table_seckey_test/get_table_seckey_test.hpp | Removes legacy secondary-key test contract |
| unittests/test-contracts/get_table_seckey_test/get_table_seckey_test.cpp | Removes legacy secondary-key test contract impl |
| unittests/test-contracts/get_table_seckey_test/get_table_seckey_test.abi | Removes legacy ABI artifact |
| unittests/test-contracts/get_table_seckey_test/CMakeLists.txt | Removes legacy build/copy rules |
| unittests/test-contracts/db_find_secondary_test/db_find_secondary_test.abi | Removes legacy ABI artifact |
| unittests/test-contracts/db_find_secondary_test/CMakeLists.txt | Removes legacy build/copy rules |
| unittests/test-contracts/dancer/dancer.abi | Adds ABI key metadata for KV-compatible table keys |
| unittests/test-contracts/bench_kv_token/bench_kv_token.cpp | Adds raw-KV benchmark token contract |
| unittests/test-contracts/bench_kv_token/bench_kv_token.abi | Adds ABI for raw-KV benchmark token |
| unittests/test-contracts/bench_kv_token/CMakeLists.txt | Adds build/copy rules for benchmark |
| unittests/test-contracts/bench_kv_shim_token/bench_kv_shim_token.cpp | Adds KV-shim (kv_multi_index) benchmark token |
| unittests/test-contracts/bench_kv_shim_token/bench_kv_shim_token.abi | Adds ABI for KV-shim token benchmark |
| unittests/test-contracts/bench_kv_shim_token/CMakeLists.txt | Adds build/copy rules for benchmark |
| unittests/test-contracts/bench_kv_shim/bench_kv_shim.cpp | Adds kv_multi_index benchmark contract |
| unittests/test-contracts/bench_kv_shim/bench_kv_shim.abi | Adds ABI for kv_multi_index benchmark contract |
| unittests/test-contracts/bench_kv_shim/CMakeLists.txt | Adds build/copy rules for benchmark |
| unittests/test-contracts/bench_kv_fast_token/bench_kv_fast_token.cpp | Adds sysio::kv::table “fast” token benchmark |
| unittests/test-contracts/bench_kv_fast_token/bench_kv_fast_token.abi | Adds ABI for fast token benchmark |
| unittests/test-contracts/bench_kv_fast_token/CMakeLists.txt | Adds build/copy rules for benchmark |
| unittests/test-contracts/bench_kv_db/bench_kv_db.cpp | Adds direct-intrinsics KV benchmark contract |
| unittests/test-contracts/bench_kv_db/bench_kv_db.abi | Adds ABI for KV benchmark contract |
| unittests/test-contracts/bench_kv_db/CMakeLists.txt | Adds build/copy rules for benchmark |
| unittests/test-contracts/CMakeLists.txt | Replaces removed legacy contracts with KV benchmarks/tests |
| unittests/test-contracts/.gitignore | Ignores CDT-generated contract artifacts |
| unittests/system-test-contracts/test_wasts.hpp | Updates WAST import test to use kv_set |
| unittests/system-test-contracts/sysio.mechanics/sysmechanics.abi | Adds ABI key metadata for KV-compatible table keys |
| unittests/system-test-contracts/jumborow/jumborow.wast | Migrates jumborow to kv_set and bulk-store logic |
| unittests/system-test-contracts/jumborow/CMakeLists.txt | Documents rebuild steps for precompiled WAT/WASM |
| unittests/state_history_tests.cpp | Updates delta expectations + adds KV delta tests |
| unittests/payer_choice_test.cpp | Updates payer-billing tests for KV + secondary index billing |
| unittests/native_overlay_tests.cpp | Adds dlopen/dlsym test for native contract modules |
| unittests/kv_token_tests.cpp | Adds unit tests for KV-backed sysio.token behavior |
| unittests/kv_tests.cpp | Adds chainbase-level tests for KV objects and iterator pool |
| tests/test_chain_plugin.cpp | Removes legacy table objects include |
| tests/sysio_util_snapshot_info_test.py | Updates expected head_block_id output |
| tests/nodeop_under_min_avail_ram.py | Adjusts RAM purchase amount in test harness |
| tests/get_table_seckey_tests.cpp | Removes legacy get_table secondary-key tests |
| tests/CMakeLists.txt | Adds new python integration tests for KV endpoints/deltas |
| plugins/producer_plugin/src/trx_priority_db.cpp | Migrates trx priority table reads from legacy DB to KV |
| plugins/chain_api_plugin/src/chain_api_plugin.cpp | Adds get_kv_rows API route |
| libraries/testing/tester.cpp | Migrates helper reads to KV object index |
| libraries/testing/include/sysio/testing/tester.hpp | Migrates generic table entry helper to KV lookup |
| libraries/state_history/include/sysio/state_history/serialization.hpp | Serializes kv_object/kv_index_object into SHiP deltas |
| libraries/state_history/create_deltas.cpp | Emits contract_row/contract_row_kv + contract_index_kv deltas |
| libraries/state_history/abi.cpp | Updates SHiP ABI to remove legacy contract_* tables and add KV deltas |
| libraries/chain/webassembly/runtimes/sys-vm.cpp | Replaces legacy db_* host functions with KV host functions |
| libraries/chain/webassembly/kv_database.cpp | Adds WASM interface bindings for KV intrinsics |
| libraries/chain/sysio_contract.cpp | Removes legacy contract_table include dependency |
| libraries/chain/snapshot.cpp | Adds JSON row bounds check + stronger section index validation |
| libraries/chain/resource_limits.cpp | Adds snapshot validation for resource limits invariants |
| libraries/chain/peer_keys_db.cpp | Removes legacy contract_table include dependency |
| libraries/chain/include/sysio/chain/webassembly/sys-vm-oc/intrinsic_mapping.hpp | Replaces intrinsic table entries for db_* with KV intrinsics |
| libraries/chain/include/sysio/chain/types.hpp | Adds KV key helpers + changes chainbase object_type enum |
| libraries/chain/include/sysio/chain/resource_limits.hpp | Declares validate_snapshot_state() |
| libraries/chain/include/sysio/chain/kv_context.hpp | Adds kv_iterator_pool and slot types |
| libraries/chain/include/sysio/chain/incremental_merkle.hpp | Adds snapshot validation helper for merkle tree |
| libraries/chain/include/sysio/chain/global_property_object.hpp | Validates configs during snapshot load |
| libraries/chain/include/sysio/chain/finalizer_policy.hpp | Adds snapshot-time threshold/weight validation |
| libraries/chain/include/sysio/chain/finality_core.hpp | Declares validate_snapshot() |
| libraries/chain/include/sysio/chain/exceptions.hpp | Adds KV exceptions; comments out legacy table exceptions |
| libraries/chain/include/sysio/chain/deep_mind.hpp | Replaces db_* hooks with KV hooks |
| libraries/chain/include/sysio/chain/database_utils.hpp | Adds BE key codec utilities for KV row API |
| libraries/chain/include/sysio/chain/config.hpp | Adds KV size/format/iterator limits constants |
| libraries/chain/include/sysio/chain/authority.hpp | Avoids memcmp on empty BLS key buffers |
| libraries/chain/genesis_intrinsics.cpp | Replaces legacy db_* intrinsics list with KV intrinsics |
| libraries/chain/finality_core.cpp | Implements snapshot-time finality invariants check |
| libraries/chain/deep_mind.cpp | Implements KV deep mind logging (DB_OP parity for format=1) |
| libraries/chain/block_state.cpp | Runs finality/policy/merkle validations at snapshot construction |
| libraries/chain/authorization_manager.cpp | Adds snapshot validations for authority threshold/keys/cycles |
| libraries/chain/CMakeLists.txt | Registers kv_database.cpp for build |
| docs/kv-ram-billing.md | Documents KV vs legacy RAM billing differences |
| contracts/tests/sysio.system_tests.cpp | Removes legacy table objects include |
| contracts/tests/sysio.system_blockinfo_tests.cpp | Migrates table scan to KV index iteration |
| contracts/tests/sysio.roa_tests.cpp | Migrates ROA test reads to KV object lookup |
| contracts/tests/sysio.msig_tests.cpp | Migrates token balance read to KV + adjusts wasm reference |
| contracts/tests/sysio.limitauth_tests.cpp | Removes legacy table objects include |
| contracts/tests/sysio.finalizer_key_tests.cpp | Migrates lastpropfins reads to KV lookup |
| contracts/tests/main.cpp | Removes legacy table objects include |
| contracts/tests/contracts.hpp.in | Removes exchange_wasm reference |
| contracts/test_contracts/blockinfo_tester/blockinfo_tester.cpp | Adjusts wasm_entry attribute placement |
| contracts/test_contracts/blockinfo_tester/blockinfo_tester.abi | Adds minimal ABI file |
| contracts/sysio.wrap/sysio.wrap.abi | Clears ricardian_contract field |
| contracts/sysio.token/sysio.token.hpp | Replaces multi_index tables with sysio::kv::table |
| contracts/sysio.token/sysio.token.abi | Adds ABI key metadata for KV-compatible table keys |
| contracts/sysio.system/sysio.system.abi | Adds/reshuffles structs and table key metadata for KV |
| contracts/sysio.system/src/exchange_state.cpp | Removes exchange_state implementation |
| contracts/sysio.system/include/sysio.system/sysio.system.hpp | Adds missing sysio includes |
| contracts/sysio.roa/sysio.roa.hpp | Adds multi_index include |
| contracts/sysio.roa/sysio.roa.abi | Adds ABI key metadata for KV-compatible table keys |
| contracts/sysio.msig/sysio.msig.hpp | Replaces multi_index tables with sysio::kv::table |
| contracts/sysio.msig/sysio.msig.cpp | Fixes kv::table get() binding to const refs |
| contracts/sysio.msig/sysio.msig.abi | Clears ricardian_contract + adds ABI key metadata |
| contracts/sysio.bios/sysio.bios.hpp | Replaces multi_index table with sysio::kv::table |
| contracts/sysio.bios/sysio.bios.abi | Adds ABI key metadata for KV-compatible table keys |
| contracts/sysio.authex/sysio.authex.abi | Adds ABI key metadata for KV-compatible table keys |
| contracts/.gitignore | Ignores CDT-generated contract artifacts |
| cmake/SysioTester.cmake.in | Improves build-tree lib/include discovery + adds fmt/keccak linking |
| CLAUDE.md | Adds contributor/dev workflow notes for tests/contracts/snapshots |
Comments suppressed due to low confidence (1)
libraries/chain/include/sysio/chain/types.hpp:1
- This PR description is focused on snapshot hardening, but the diff includes a major shared-memory schema change:
object_typeentries have been removed/reordered. Per the comment directly above, reordering/removing is a shared-memory breaking change and will invalidate existing chainbase DBs/snapshots. If the intent is not to break compatibility, keep all existing enum ordinals (rename toUNUSED_*placeholders) and only append new*_object_typevalues beforeOBJECT_TYPE_COUNT. If breaking is intended, the PR description should explicitly call out the compatibility break and the required migration/DB reset strategy.
#pragma once
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const auto& kv_idx = db.get_index<chain::kv_index, chain::by_code_key>(); | ||
| auto itr = kv_idx.lower_bound(boost::make_tuple(config::system_account_name, config::kv_format_standard, prefix.to_string_view())); | ||
|
|
||
| while (itr != kv_idx.end() && itr->code == config::system_account_name) { |
There was a problem hiding this comment.
The loop does not validate itr->key_format before attempting to interpret keys as standard-format rows. A crafted/accidental non-standard entry with code==sysio, key_size==24, and the same 16-byte prefix could be incorrectly parsed and fc::raw::unpack’d as trx_prio. Fix by requiring itr->key_format == config::kv_format_standard (and ideally also requiring kv.size() == chain::kv_key_size as you already do) before decoding/unpacking.
Suggested change
| while (itr != kv_idx.end() && itr->code == config::system_account_name) { | |
| while (itr != kv_idx.end() | |
| && itr->code == config::system_account_name | |
| && itr->key_format == config::kv_format_standard) { |
Comment on lines
+183
to
+198
| // NUL-escape decoding: 0x00,0x01 = literal NUL byte, 0x00,0x00 = end of string. | ||
| std::string read_nul_escaped_string() { | ||
| std::string s; | ||
| while (pos < end) { | ||
| char c = *pos++; | ||
| if (c == '\0') { | ||
| FC_ASSERT(pos < end, "BE key underflow reading string terminator"); | ||
| char next = *pos++; | ||
| if (next == '\0') break; // 0x00,0x00 = end | ||
| s.push_back('\0'); // 0x00,0x01 = literal NUL | ||
| } else { | ||
| s.push_back(c); | ||
| } | ||
| } | ||
| return s; | ||
| } |
There was a problem hiding this comment.
The NUL-escaped string decoder accepts any byte after 0x00 as a literal NUL (line 192) and also allows the string to run to end without ever seeing the required terminator 0x00,0x00. For hostile input (e.g., via get_kv_rows key decoding), this makes decoding ambiguous and can hide malformed keys. Consider enforcing: (1) after 0x00, the next byte must be either 0x00 (end) or 0x01 (literal NUL), otherwise throw; and (2) require a terminator to be present rather than silently accepting unterminated strings.
Comment on lines
+65
to
+80
| uint32_t allocate_secondary(account_name code, name table, uint8_t index_id) { | ||
| uint32_t idx = find_free(); | ||
| auto& s = _slots[idx]; | ||
| s.in_use = true; | ||
| s.is_primary = false; | ||
| s.status = kv_it_stat::iterator_end; | ||
| s.code = code; | ||
| s.prefix.clear(); | ||
| s.table = table; | ||
| s.index_id = index_id; | ||
| s.current_key.clear(); | ||
| s.current_sec_key.clear(); | ||
| s.current_pri_key.clear(); | ||
| s.cached_id = -1; | ||
| return idx; | ||
| } |
There was a problem hiding this comment.
allocate_secondary never initializes s.key_format. Since iterator slots are reused, this can leave a stale key_format from a previous primary iterator allocation, which can cause incorrect behavior if any secondary-iterator logic consults key_format. Set s.key_format explicitly (e.g., to config::kv_format_standard or 0 as a sentinel) when allocating a secondary iterator.
Comment on lines
+99
to
+107
| uint32_t count = 0; | ||
| int32_t status = 0; // 0 = OK | ||
| while (status == 0) { | ||
| ++count; | ||
| status = kv_it_next(handle); | ||
| } | ||
|
|
||
| kv_it_destroy(handle); | ||
| check(count > 0, "no rows"); |
There was a problem hiding this comment.
This iterator loop increments count before advancing, and initializes status to OK unconditionally. As written, count will be at least 1 even when there are no rows, so check(count > 0, \"no rows\") can never fail. Consider driving the loop from kv_it_status(handle) and only incrementing when the iterator is positioned on a valid element (or call kv_it_next once before incrementing).
| // format=0 rows should appear in "contract_row_kv" | ||
| auto kv_result = chain.find_table_delta("contract_row_kv"); | ||
| BOOST_REQUIRE(kv_result.first); | ||
| BOOST_REQUIRE_GT(kv_result.second->rows.obj.size(), 0u); |
There was a problem hiding this comment.
The test case comment says format=0 rows appear in contract_row_kv and not in contract_row, but the test does not assert the absence of the contract_row delta. Adding a BOOST_CHECK(!chain.find_table_delta(\"contract_row\").first) here would make the test actually enforce the intended separation and prevent regressions where format=0 rows leak into the legacy delta.
Suggested change
| BOOST_REQUIRE_GT(kv_result.second->rows.obj.size(), 0u); | |
| BOOST_REQUIRE_GT(kv_result.second->rows.obj.size(), 0u); | |
| // and MUST NOT appear in the legacy "contract_row" delta | |
| auto legacy_result = chain.find_table_delta("contract_row"); | |
| BOOST_CHECK(!legacy_result.first); |
Snapshots from untrusted sources (peer-served, hash-attested) can contain structurally valid but semantically poisoned state that crashes or hangs a node before attestation is verified on-chain. This adds validation for all structural invariants that hash attestation alone cannot protect. Config validation: - Call chain_config::validate() and wasm_config::validate() during GPO deserialization to catch zero denominators and invalid WASM limits - Validate elastic_limit_parameters denominators, max, and max_multiplier are positive; account usage average windows are positive; virtual cpu/net limits are positive Count and singleton enforcement: - Assert exactly 1 row for GPO, DGPO, protocol_state, resource_limits config, and resource_limits state objects - Assert exactly 65536 block_summary rows - Assert preactivated_protocol_features is empty in finalized snapshots Finality state validation: - Add finality_core::validate_snapshot() checking all 9 documented invariants (links non-empty, sorted, contiguous; refs consistent) - Validate all finalizer policies (active, pending, proposed, latest_qc_claim_block) have threshold > 0, weights > 0, threshold <= sum(weights) with overflow protection - Assert active_finalizer_policy and active_proposer_policy are non-null - Validate incremental_merkle_tree mask/trees consistency - Validate valid_t.validation_mroots size matches block range Permission validation: - Assert authority threshold > 0 for non-reserved permissions - Validate key types (k1/r1/wa/em/ed only) - Detect circular parent references inline during load via depth-bounded parent chain walk (capped at max_auth_depth) Snapshot reader hardening: - Fix integer overflow in section bounds check (data_offset + data_size) - Cap num_sections at 256 to prevent unbounded allocation - Detect duplicate section names and overlapping data ranges - Validate JSON snapshot row array size matches num_rows metadata
- Add key_format check to trx_priority_db iterator loop - Enforce NUL-escaped string terminator in BE key decoder - Initialize key_format in allocate_secondary to prevent stale slot reuse - Fix bench_kv_db iterall() off-by-one iterator count - Assert format=0 rows absent from legacy contract_row SHiP delta - Validate KV key/value sizes against absolute limits on snapshot load - Refactor kv_format constants to unscoped enum with KV_FORMAT_COUNT sentinel - Replace bare key_format literals with named constants
heifner
force-pushed
the
feature/snapshot-hardening
branch
from
3e33e17 to
e57e48d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Motivation
Peer-served snapshots are hash-attested, but a node must process blocks from the snapshot state before it syncs far enough to verify the on-chain attestation record. A crafted snapshot with structurally valid but semantically poisoned state (e.g. zero denominators, circular permission parents, empty finality links) could crash or hang the node in this pre-attestation window.
Data consistency issues (code_hash mismatches, cross-object referential integrity) are covered by hash attestation and excluded from scope.
Changes
Config validation —
chain_config::validate(),wasm_config::validate()during GPO deserialization; elastic limit parameter max/max_multiplier/denominator checks; usage window and virtual limit positivity checksCount enforcement — singleton assertions for GPO, DGPO, protocol_state, resource_limits config/state; block_summary == 65536; preactivated_protocol_features must be empty
Finality state —
finality_core::validate_snapshot()checking all 9 documented invariants; finalizer policy threshold/weight validation with overflow protection on all policy types; null checks on active_finalizer_policy and active_proposer_policy; incremental_merkle_tree mask/trees consistency; valid_t.validation_mroots sizePermissions — authority threshold > 0, key type validation (k1/r1/wa/em/ed), inline circular parent detection via depth-bounded walk (capped at max_auth_depth=6)
Snapshot reader — integer overflow fix in section bounds check, num_sections cap at 256, duplicate section name detection, overlapping data range detection, JSON row array bounds check