Skip to content

oor: validate incoming ancestor packages (#366) - #439

Closed
ellemouton wants to merge 3 commits into
mainfrom
fix/366-validate-oor-ancestor-packages
Closed

oor: validate incoming ancestor packages (#366)#439
ellemouton wants to merge 3 commits into
mainfrom
fix/366-validate-oor-ancestor-packages

Conversation

@ellemouton

Copy link
Copy Markdown
Member

Closes #366.

Summary

  • Validate operator/indexer-supplied OOR ancestor packages on the incoming receive path before persisting them.
  • Reject same-direction UpsertPackage calls that try to overwrite an existing row's Ark PSBT or checkpoint payload.

The bug

The incoming OOR receive path persists operator/indexer-supplied ancestor packages as recovery artifacts for later unroll. Two attack vectors were open:

  1. Plant fake artifacts under arbitrary session ids. The materialization path only checked that each ancestor PSBT was parseable, so a malicious operator/indexer response could stage arbitrary "ancestor" packages under any session id. A later unilateral exit would then load forged artifacts and either fail or chase a chain that never confirms, making the received VTXO effectively unexitable.
  2. Overwrite legitimate recovery artifacts for known session ids. UpsertPackage checked only the stored direction before falling through to delete and re-insert the Ark PSBT and checkpoint rows. A response producing a parseable package for a session id the attacker already knew (e.g. they hold the original Ark transaction) could overwrite the previously stored recovery artifacts, destroying recovery state for a real received VTXO.

The fix

Vector (a) — ingest-time validation (a9147ad8). Require SessionID == ark_txid, run oortx.ValidateFinalizePackage to check the canonical Ark PSBT shape, the exact checkpoint set, and the finalize-sig material, then walk the checkpoint-input graph to require each ancestor checkpoint to be reachable from the root. Ancestor count is capped via the existing ReceiveLimits. Validation runs in two places for defense in depth:

  • IncomingTransferEventFromResponseWithLimits — the RPC adapter, so bad responses are rejected before they touch the FSM.
  • validateMaterializeIncoming — the persistence handler, so any future code path that reaches materialization without going through the adapter is still gated.

Vector (b) — payload-equality on duplicate upsert (64228380). On a duplicate-PK upsert, compare the existing Ark PSBT and checkpoint bytes against the incoming payload. Equal payloads are treated as idempotent (early return). A mismatched payload fails with "oor package %x already exists with different payload", preserving the original artifact. The compare runs inside the existing ExecTx; ListOORPackageCheckpoints orders by index ASC, so positional bytes.Equal is sound.

Commit layout

Three atomic commits per the repo's commit style:

  • a9147ad8 oor: Validate incoming ancestor packages before storing
  • 9ebf1444 oor: Add coverage for ancestor package validation graph
  • 64228380 db: Reject same-direction OOR package payload rewrites

The db: payload-rewrite rejection is in scope for #366 (vector b) but kept as its own commit because it is an independently reasoning-able change at a different layer. Happy to squash before merge if a reviewer prefers a single security-fix commit.

Test plan

  • oor unit tests cover the positive (ancestor actually spent by checkpoint), duplicate-ancestor rejection, and per-ancestor checkpoint cap paths.
  • db unit tests cover the same-direction same-payload (idempotent) and same-direction divergent-payload (rejected) upsert cases.
  • make fmt-changed + native linter clean on the branch.
  • Reviewer to sanity-check the reachability walk against any known multi-hop ancestor fixtures.

Incoming receive materialization persists ancestor OOR artifacts for
later unroll recovery. Those artifacts come from operator/indexer
responses, so accepting only parseable PSBTs lets a bad response
overwrite recovery state.

Validate the finalized package shape, txid-derived session id,
and ancestor reachability before converting RPC responses or writing
package rows.
Cover the positive case where a checkpoint actually spends an
ancestor Ark output, the duplicate-ancestor rejection path, and the
per-ancestor checkpoint count cap added on the RPC adapter. The
existing validation commit only asserted the negative cases, so a
later regression that silently dropped reachability or bounding
would not have been caught.
UpsertPackage previously checked only the stored direction before
falling through to delete and re-insert the Ark PSBT and checkpoint
rows. That made the artifact store a rewrite surface: a malicious
operator/indexer response that produced a parseable package for a
known session id could overwrite previously stored recovery
artifacts even when the FSM-layer validation passed (e.g. when the
attacker holds the original Ark transaction).

Treat retried upserts as idempotent only when the serialized Ark
PSBT and checkpoint payload match the existing row. Any divergence
now fails the upsert so the original recovery artifact survives.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements validation for incoming Out-of-Round (OOR) package graphs to ensure that ancestor artifacts are valid and reachable from the root package's checkpoint chain. It also adds an idempotency check in the database layer to prevent overwriting existing packages with conflicting payloads and enforces receive limits on ancestor checkpoints. I have no feedback to provide.

@ellemouton
ellemouton marked this pull request as ready for review May 15, 2026 02:06
@ellemouton

Copy link
Copy Markdown
Member Author

Superseded by consolidated PR #459. Closing to reduce CI load.

@ellemouton ellemouton closed this May 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[security][critical] Unvalidated OOR ancestor packages poison recovery state

1 participant