feat(platform-wallet): manifest integrity checksum (Risk-6/R12.5 follow-up)#3992
feat(platform-wallet): manifest integrity checksum (Risk-6/R12.5 follow-up)#3992Claudius-Maginificent wants to merge 4 commits into
Conversation
…_registrations
Add a per-row SHA-256(wallet_id ‖ account_xpub_bytes) integrity checksum to
the account_registrations manifest, verified at load() as a per-wallet SKIP
rather than a batch abort. Catches accidental corruption, migration errors,
and the Risk-6 "manifest row bound to the wrong wallet_id" class without
false-positiving on a backup restore (checksum ignores the store generation).
- V003 additive migration: ALTER TABLE account_registrations ADD COLUMN
checksum BLOB (nullable; SQLite has no SHA-256 builtin). V001/V002 stay
byte-identical; fingerprint goldens re-pinned for V003.
- Writer: apply_registrations computes + binds the checksum in the existing
single flush transaction (INSERT + DO UPDATE SET checksum = excluded).
- open(): one-time idempotent backfill fills any NULL checksum before load()
ever verifies (own short tx).
- load(): verify_manifest_checksums per wallet; a mismatch/NULL pushes
SkipReason::CorruptPersistedRow{ ManifestIntegrityMismatch } and continues.
The bulk platform_payment oracle also drops checksum-failing rows so it
never fail-hard decodes a tampered blob (G6 belt-and-suspenders).
- Typed WalletStorageError::ManifestIntegrityMismatch and
CorruptKind::ManifestIntegrityMismatch (+Display); FFI reason code 103.
- Tests TC-C-001..011: write, valid load, tampered/wrong-wallet/NULL skips,
combined-batch isolation, backfill exactness/idempotency, restore
no-false-positive, re-persist consistency, FFI mapping, fingerprint goldens.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
# Conflicts: # packages/rs-platform-wallet-ffi/src/manager.rs
…hot-redirect merge Integrate against origin/feat/3968-snapshot-redirect (WS-A #3980/#3984): - FFI: renumber LOAD_SKIP_REASON_MANIFEST_INTEGRITY_MISMATCH 103 -> 104; 103 is now WS-A's LOAD_SKIP_REASON_SNAPSHOT_IDENTITY_MISMATCH. Both CorruptKind variants and their mappings coexist. - load(): populate the new ClientWalletStartState.core_wallet_info field as None — the SQLite persister rehydrates via the manifest skeleton + core projection; the full snapshot path is the FFI/iOS persister's. - Update the V002 migration-execution tests for the V003 bump: migrated version 2 -> 3, pre-migration backup name 1-to-2 -> 1-to-3, and move the forward-version rejection probe from version 3 to 4. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…egrity-checksum Fold the four snapshot-redirect follow-ups (core_address_pool account_type PK widening, deterministic account_index_for_script ORDER BY, blob-size gate on core_pool/core_state load_used_addresses, restore_from generation rotation folded into the atomic swap) onto the V003 manifest-checksum work. Reconciliations: - persister.rs: kept the concise upstream core_wallet_info: None comment. - sqlite_schema_pinning.rs: recomputed EXPECTED_SQL_FINGERPRINT for the combined V001 + V002(widened account_type PK) + V003(checksum column) migration set -> e9c1679ba10d60b03923f20825cd41a6caaee2e186c239e138d8f6799f5659a7. EXPECTED_ID_FINGERPRINT unchanged (no migration file added/removed/renamed by the widening; V003 already accounted for on this branch). account_registrations checksum reader/writer is confined to that table and does not touch the widened core_address_pool PK. Full platform-wallet-storage, platform-wallet, platform-wallet-ffi suites green; fmt + clippy clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lklimek
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
lklimek
changed the title
|
✅ Review complete (commit 579b620) |
There was a problem hiding this comment.
Code Review
PR #3992 adds a well-scoped, additive SHA-256 integrity checksum over SHA-256(wallet_id ‖ account_xpub_bytes) to account_registrations, with an idempotent V003 backfill and per-wallet skip semantics on mismatch. Design and test coverage are solid (Risk-6 wrong-wallet-row, NULL-checksum skip, batch isolation, restore non-false-positive). Three in-scope suggestions worth addressing: (1) verify_manifest_checksums still calls blob::check_size before the checksum compare, so an oversized tampered blob returns BlobTooLarge and aborts the whole batch instead of falling into the per-wallet skip path the PR advertises; (2) backfill_missing_checksums does not size-gate account_xpub_bytes before materializing it in memory, unlike every other reader touching this column in this PR; (3) the Swift-side reasonDescription wire-code mirror was not extended for the new code 104.
Source: reviewers opus/opus, sonnet5/claude-sonnet-5, codex/gpt-5.5[high] for general + security-auditor + rust-quality + ffi-engineer; verifier opus/opus. Failed lanes: codex/gpt-5.5[high] general, codex/gpt-5.5[high] security-auditor, codex/gpt-5.5[high] rust-quality, codex/gpt-5.5[high] ffi-engineer.
🟡 3 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:259-275: verify_manifest_checksums fails hard on oversize blob, breaking the per-wallet skip contract for exactly the tamper class it defends against
`verify_manifest_checksums` calls `blob::check_size(row.get::<_, i64>(0)?)?` at line 265 *before* the SHA-256 comparison. If a tampered `account_xpub_bytes` also happens to exceed `BLOB_SIZE_LIMIT_BYTES`, this returns `WalletStorageError::BlobTooLarge` — which the caller in `persister.rs::load()` (line 952) routes through `Err(other) => return Err(PersistenceError::from(other))`, aborting the entire load batch. That contradicts the PR-headline invariant that a tampered / mis-bound row skips one wallet and never aborts a multi-wallet load, and it defeats it in exactly the corruption class this checksum exists to catch (in-place blob mutation, cross-wallet row copy). Because the writer's own size gate keeps this unreachable on legitimate rows, any hit here is by construction a manifest-integrity event. Either treat `BlobTooLarge` from this function as `ManifestIntegrityMismatch`, or drop the size gate here and let the checksum recompute be the sole verdict (a mismatched-length blob will fail the SHA-256 compare anyway). The pre-existing fail-hard in `load_state` predates this PR and is out of scope; this function is new and is where the skip contract lives.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:285-317: backfill_missing_checksums reads account_xpub_bytes without the blob-size gate used everywhere else in this PR
`backfill_missing_checksums` runs on every `SqlitePersister::open()` and materializes `(rowid, wallet_id, account_xpub_bytes)` for every NULL-checksum row into a single `Vec` via `.collect()`. Unlike every other reader of this same column in this PR — `verify_manifest_checksums` (line 265: `blob::check_size(row.get::<_, i64>(0)?)?`), `load_state` (line 212), and `all_platform_payment_registrations` (lines 107-113) — this new function never calls `blob::check_size` or checks `length(account_xpub_bytes)` before reading. This matters because the backfill's own threat model is a legacy or corrupted store: a row with `checksum = NULL` and an oversized `account_xpub_bytes` blob (SQLite BLOBs can be ~2GB) would be read in full into memory on every `open()`, before any checksum verification runs. Exploitation requires an already-corrupted DB on disk (matching this PR's stated Risk-6 threat model), so it is defense-in-depth rather than remotely triggerable, but the inconsistency with the size gate that appears on every other reader of the same column in this same change is worth closing. Select `length(account_xpub_bytes)` alongside the payload and reject via `blob::check_size` before decoding, matching the pattern in the sibling functions.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:344-354: Swift-side wire-code mirror not updated for new LOAD_SKIP_REASON_MANIFEST_INTEGRITY_MISMATCH (104)
This PR adds `LOAD_SKIP_REASON_MANIFEST_INTEGRITY_MISMATCH = 104` (rs-platform-wallet-ffi/src/manager.rs:193) and routes `CorruptKind::ManifestIntegrityMismatch` to it (manager.rs:249). The Swift binding's `SkippedWalletOnLoad.reasonDescription` (this file, lines 344-354) is the documented mirror of that wire contract — its own doc comment says the cases are matched by value against `rs-platform-wallet-ffi/src/manager.rs`. That switch was not extended with `case 104`, so any wallet skipped for the new manifest-integrity reason — the exact case this PR exists to surface — falls through to `default: return "unknown skip reason (104)"` instead of a meaningful description. The raw `reasonCode` is still delivered via `SkippedWalletOnLoad.reasonCode`, so no data is lost, but the SDK's own decoding helper silently degrades the PR's headline new signal. The struct-level doc comment at lines 329-334 also enumerates codes only up to 103 and should be updated in the same change.
| let mut stmt = conn.prepare( | ||
| "SELECT length(account_xpub_bytes), account_xpub_bytes, checksum \ | ||
| FROM account_registrations WHERE wallet_id = ?1", | ||
| )?; | ||
| let mut rows = stmt.query(params![wallet_id.as_slice()])?; | ||
| while let Some(row) = rows.next()? { | ||
| blob::check_size(row.get::<_, i64>(0)?)?; | ||
| let payload: Vec<u8> = row.get(1)?; | ||
| let stored: Option<Vec<u8>> = row.get(2)?; | ||
| let expected = account_registration_checksum(wallet_id, &payload); | ||
| match stored { | ||
| Some(c) if c.as_slice() == expected => {} | ||
| _ => return Err(WalletStorageError::ManifestIntegrityMismatch), | ||
| } | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: verify_manifest_checksums fails hard on oversize blob, breaking the per-wallet skip contract for exactly the tamper class it defends against
verify_manifest_checksums calls blob::check_size(row.get::<_, i64>(0)?)? at line 265 before the SHA-256 comparison. If a tampered account_xpub_bytes also happens to exceed BLOB_SIZE_LIMIT_BYTES, this returns WalletStorageError::BlobTooLarge — which the caller in persister.rs::load() (line 952) routes through Err(other) => return Err(PersistenceError::from(other)), aborting the entire load batch. That contradicts the PR-headline invariant that a tampered / mis-bound row skips one wallet and never aborts a multi-wallet load, and it defeats it in exactly the corruption class this checksum exists to catch (in-place blob mutation, cross-wallet row copy). Because the writer's own size gate keeps this unreachable on legitimate rows, any hit here is by construction a manifest-integrity event. Either treat BlobTooLarge from this function as ManifestIntegrityMismatch, or drop the size gate here and let the checksum recompute be the sole verdict (a mismatched-length blob will fail the SHA-256 compare anyway). The pre-existing fail-hard in load_state predates this PR and is out of scope; this function is new and is where the skip contract lives.
source: ['claude']
| pub fn backfill_missing_checksums(conn: &mut Connection) -> Result<usize, WalletStorageError> { | ||
| let tx = conn.transaction()?; | ||
| let pending: Vec<(i64, Vec<u8>, Vec<u8>)> = { | ||
| let mut stmt = tx.prepare( | ||
| "SELECT rowid, wallet_id, account_xpub_bytes \ | ||
| FROM account_registrations WHERE checksum IS NULL", | ||
| )?; | ||
| let mapped = stmt.query_map([], |row| { | ||
| let rowid: i64 = row.get(0)?; | ||
| let wid_bytes: Vec<u8> = row.get(1)?; | ||
| let payload: Vec<u8> = row.get(2)?; | ||
| Ok((rowid, wid_bytes, payload)) | ||
| })?; | ||
| mapped.collect::<Result<Vec<_>, _>>()? | ||
| }; | ||
| let mut filled = 0usize; | ||
| { | ||
| let mut upd = | ||
| tx.prepare_cached("UPDATE account_registrations SET checksum = ?1 WHERE rowid = ?2")?; | ||
| for (rowid, wid_bytes, payload) in pending { | ||
| let wallet_id = <[u8; 32]>::try_from(wid_bytes.as_slice()).map_err(|_| { | ||
| WalletStorageError::InvalidWalletIdLength { | ||
| actual: wid_bytes.len(), | ||
| } | ||
| })?; | ||
| let checksum = account_registration_checksum(&wallet_id, &payload); | ||
| upd.execute(params![&checksum[..], rowid])?; | ||
| filled += 1; | ||
| } | ||
| } | ||
| tx.commit()?; | ||
| Ok(filled) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: backfill_missing_checksums reads account_xpub_bytes without the blob-size gate used everywhere else in this PR
backfill_missing_checksums runs on every SqlitePersister::open() and materializes (rowid, wallet_id, account_xpub_bytes) for every NULL-checksum row into a single Vec via .collect(). Unlike every other reader of this same column in this PR — verify_manifest_checksums (line 265: blob::check_size(row.get::<_, i64>(0)?)?), load_state (line 212), and all_platform_payment_registrations (lines 107-113) — this new function never calls blob::check_size or checks length(account_xpub_bytes) before reading. This matters because the backfill's own threat model is a legacy or corrupted store: a row with checksum = NULL and an oversized account_xpub_bytes blob (SQLite BLOBs can be ~2GB) would be read in full into memory on every open(), before any checksum verification runs. Exploitation requires an already-corrupted DB on disk (matching this PR's stated Risk-6 threat model), so it is defense-in-depth rather than remotely triggerable, but the inconsistency with the size gate that appears on every other reader of the same column in this same change is worth closing. Select length(account_xpub_bytes) alongside the payload and reject via blob::check_size before decoding, matching the pattern in the sibling functions.
source: ['claude']
3 participants
Issue being fixed or feature implemented
account_registrationsrows currently have no binding to thewallet_idthey're supposed to belong to beyond "whichever row the query happened to read." A data bug, a migration error, or disk corruption could silently associate a manifest row with the wrong wallet, causing platform-wallet to derive addresses under the wrong key while believing it's still the original wallet — the manifest trust-boundary gap tracked as Risk-6/R12.5 in the Model-A adoption risk assessment, deferred out of #3986's scope for a focused follow-up.What was done?
An unkeyed SHA-256 integrity checksum, deliberately not a keyed MAC — a local-write-access adversary can recompute any value stored in the same file, so keying with an in-file secret (e.g. the store-generation token) buys no real security. Honest framing: this closes the accidental-corruption / cross-wallet-row class, not an authentication guarantee.
ALTER TABLE account_registrations ADD COLUMN checksum BLOB. V001/V002 stay byte-identical.SHA-256(wallet_id ‖ account_xpub_bytes)— exactly these two fixed identity fields, nothing else. Deliberately excludesmeta_store_generation/meta_data_versions.seq, both of which rotate on legitimate restore/migrate — including either would make every restore false-positive as tampered.account_registrationsis immutable after account creation; address-pool growth lives in the separatecore_address_pooltable.open()fills the checksum for every pre-existing row (SQLite has no SHA-256 builtin, so this can't be a pure-DDL default). By the timeload()runs, every row is covered — no legacy grace period.ClientStartState.skippedchannel —CorruptKind::ManifestIntegrityMismatch(#[non_exhaustive]-safe), FFI wire code 104 (SnapshotIdentityMismatch already claimed 103 upstream). One tampered row never aborts the whole load.all_platform_payment_registrations(used before the per-wallet loop) now also verifies-and-drops on mismatch — without this, a tamperedplatform_paymentrow would abort the load before the per-wallet skip machinery ever ran.Rebased onto #3986's final state (
core_address_pool's widened PK, deterministic pool-script lookup, blob-size gates, restore-ordering fix) — no interaction between the two: this checksum touches onlyaccount_registrations; #3986's fixes touchcore_address_pool. Verified independently.How Has This Been Tested?
platform-wallet-storage,platform-wallet(217+ lib tests),platform-wallet-ffi— 0 failures.cargo fmt --allclean;clippy --all-features --all-targets -- -D warningsclean.Breaking Changes
None. V001/V002 untouched; V003 is additive with automatic backfill. No
SkipReason/CorruptKindshape changes beyond the new additive variant.Checklist:
For repository code-owners and collaborators only
Attribution
🤖 Co-authored by Claudius the Magnificent AI Agent