multi: consolidated security fixes (May 15) - #459
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several security and robustness enhancements focused on protecting user funds and ensuring reliable recovery. Key changes include the implementation of strict ancestry path depth validation and input coverage checks to prevent malicious indexers from stranding funds. It also adds TLS binding signatures to secure client identity registration against replay attacks and introduces "realised-fee" enforcement to prevent operators from bypassing fee caps by manipulating change outputs. Additionally, the unroll registry and artifact storage were refactored to improve resilience against transient failures and ensure idempotency during persistence. As there were no review comments provided, I have no feedback to offer on the code review process.
7a3263c to
f0c6de5
Compare
|
Addressed Lint failure (dupl between |
|
@claude review this |
|
Claude finished @ellemouton's task in 4m 12s —— View job PR Review: multi: consolidated security fixes (May 15)
Overall AssessmentThis is a well-structured, defense-in-depth security PR. Each commit is independently reviewable and tightly scoped. The layered trust-boundary enforcement pattern (RPC ingest → FSM validation → persistence → unroll) is well-executed. Comments are informative and consistently explain why a given gate exists (fund-safety, timing surfaces, etc.). A few observations follow.
|
f0c6de5 to
2880ed3
Compare
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. Additionally, harden UpsertPackage: previously it 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.
The foreign-ancestor fallback in ResolveUnrollPackages loads a session-keyed package when no local outpoint binding exists for a checkpoint input. Until now it only bounds-checked the referenced output index, which meant any stored row whose session_id column matched the checkpoint input's previous hash would be accepted as resolved ancestry, even when the stored Ark transaction did not actually hash to that session id or when the index landed on the non-spendable anchor output. Ancestor packages are operator/indexer-supplied artifacts, so a poisoned or mismatched row could otherwise be grafted onto an unroll-chain at resolution time and corrupt unilateral-exit proof assembly. Re-verify the txid binding and reject anchor outputs at read time so the resolver does not depend on writers having enforced those invariants. Mismatches are treated as unresolved inputs rather than hard errors so the chain still degrades to the existing recovery path. Closes #371.
7ce3c3d to
b714ae2
Compare
|
Updated head: Addressed from Claude's review:
Additional CI follow-ups on this branch:
Local checks run:
|
878c626 to
239ecea
Compare
validateIncomingAncestry checked that each fragment's InputIndices was non-empty and within range, but did not reject duplicate indices or require the union of all fragments to cover every Ark tx input. An indexer/operator could return ancestry for only a subset of a multi-input OOR Ark tx — for example a two-input Ark tx with a single fragment naming only input 0 — and the descriptor would be accepted and persisted with incomplete recovery lineage. If the operator later disappears, the uncovered input has no rooted-path material for fraud-watch or unilateral-exit assembly, stranding the received VTXO. Track which input indices each fragment claims, reject duplicates at the malformed fragment, and require coverage of every Ark tx input before returning. Add direct table-driven coverage for the partition checks and a two-input materialization helper so the existing primary-ancestry normalization test exercises a genuine cross-round multi-input shape rather than a single-input PSBT with two fake fragments. Closes #374.
The registry used to mark each non-terminal job PhaseFailed when
RestoreNonTerminal could not spawn or resume its child actor, and
handleEnsure short-circuited on any existing record. Resume can
fail for transient external dependencies — chain backend
SubscribeBlocks / RegisterSpend through ChainSource, or a flaky
DB. Once the record was marked terminal, ListNonTerminalRecords
skipped it on every subsequent boot, turning a transient outage
during daemon start into a permanent shutdown of the recovery
path for that VTXO. For a VTXO that is in unilateral_exit and
near expiry, that translates into locked or lost funds.
This commit makes restore failures non-sticky on two paths:
1. restoreNonTerminal no longer calls MarkTerminal on spawn /
resume failure. The durable record stays non-terminal, is
logged, and gets retried on the next daemon restart and on
any in-boot EnsureUnrollRequest for the same outpoint.
2. handleEnsure detects "non-terminal store record, no active
child" — the signature of a previous restore that did not
wire up — and attempts an inline restore via a shared
tryRestoreOne helper. Success returns Created=false with
the historical ActorID; failure surfaces the error so the
caller can retry.
Three new tests lock the behavior in:
- TestRegistryRestoreFailureLeavesRecordRetryable:
first RestoreNonTerminal fails on resume, record stays
non-terminal, a second RestoreNonTerminal with a healthy
spawn succeeds.
- TestRegistryEnsureRestoresFailedNonTerminalRecord:
a fresh EnsureUnroll on an unrestored non-terminal record
triggers inline restore.
- TestRegistryEnsureRetriesAfterInlineRestoreFailure:
a failed inline restore inside Ensure does not strand the
job; the next Ensure retries.
Closes #381.
The quote.OperatorFeeSat field is operator-attested; a malicious operator could quote a small fee within env.MaxOperatorFee while shaving the IsChange=true output by a much larger delta. The echo validator intentionally permits change-output amount deviation, so without a second gate the client would accept and sign a round whose actual economic fee exceeds the cap. Recompute the realised fee at evaluateQuote time as Σ(authoritative inputs) − Σ(quoted outputs), with inputs sourced from the client's own intent composition (boarding ChainInfo amounts and VTXOStore forfeit values) and outputs from the quote's positional slices. Reject when the realised value exceeds the cap, is negative, or disagrees with the declared OperatorFeeSat (operator dishonesty that would otherwise drift downstream fee accounting). Closes #379.
validateQuoteEchoes previously skipped the non-change amount-equality check entirely whenever the combined VTXORequests + LeaveRequests count was one. The shortcut was added to mirror the server's implicit-change relaxation (the lone slot absorbs the residual) but was too broad: it admitted any echoed amount on that slot, including zero. A malicious or compromised operator endpoint could shave arbitrary value from any single-output flow -- the worst case being a single-recipient directed send with coin-selection-exact change=0, where the lone slot is a third-party recipient marked IsChange=false and downstream commitment validation treats the quote's AmountSat as authoritative. The server's residual on the implicit-change slot is exactly (intent target - OperatorFeeSat). Enforce that equality on the lone slot for both VTXO and leave channels. The OperatorFeeSat itself is already bounded by env.MaxOperatorFee at line 840, so the only honest deviation reduces to a capped fee deduction -- not unbounded shaving. Closes #378.
The receive path copied AncestryPath.tree_depth directly from the indexer into the persisted descriptor without validation. An untrusted indexer could therefore return a matching VTXO with tree_depth = 0 (or a non-zero value that disagrees with the supplied tree_path) and either silently strand the OOR VTXO at unroll time (the proof assembler rejects zero-depth fragments as proof-unavailable) or under-report MaxTreeDepth and delay refresh/unilateral-exit past the safe CSV deadline. Both outcomes are fund-availability bugs. Introduce arkrpc.ValidateAncestryPathDepth as the shared validator at the indexer→client boundary. It rejects zero claims, claims above MaxAncestryTreeWalkDepth (the same cap the receive-side tree walk enforces, so any tree that survives decode also survives this gate), and claims that disagree with the reconstructed tree path's actual depth. Both ancestryFromRPC helpers (darepod + oor) call it before materializing vtxo.Ancestry, and validateIncomingAncestry calls it again as defense-in-depth so in-process descriptor construction is also gated. Closes #370.
The per-fragment validateProofDescriptorShape rejected any Ancestry fragment whose TreeDepth scalar was zero. TreeDepth is expiry-timing metadata (see vtxo.Descriptor.MaxTreeDepth and vtxo/expiry.go); the proof assembler walks TreePath.Root directly and never reads the scalar. Because incoming OOR ancestry is built from indexer RPC data by copying p.GetTreeDepth() verbatim, a malicious or version-skewed indexer that supplies a non-empty TreePath with TreeDepth omitted or forged to zero can persist a VTXO that accepts and validates fine on receive but is permanently rejected at unroll time. That turns an indexer-controlled scalar into a fund-stranding lever on the cooperative-operator-unavailable path, which is the exact threat model unilateral exit exists to defend against. Drop the gate so a zero TreeDepth no longer blocks proof assembly, and document the receive-side ingest boundary as the proper place to validate the scalar against TreePath.Depth() (issue #370). Closes #372.
Add a secp256k1 → TLS-leaf binding signature carried in a new x-mailbox-tls-bind-sig envelope header so the server can verify, on first-contact Send, that the secp256k1 mailbox key holder chose the TLS leaf the server observes on the connection. Without this binding, the existing Schnorr auth signature (which covers only the mailbox identity + envelope contents) does not constrain which TLS session the envelope is replayed across. An attacker who captured a victim's signed first Send could replay the same bytes over a TLS connection backed by an attacker-controlled leaf and have their fingerprint bound to the victim's mailbox ID, defeating the post-registration fingerprint defense added in #443. The new digest uses a dedicated BIP-340 tag (mailbox-tls-bind) so neither signature can be reinterpreted as the other. The leaf is identified by its SubjectPublicKeyInfo DER bytes, which commits to the curve and algorithm identifier alongside the raw key — TLS certs in this deployment use P-256, distinct from the secp256k1 mailbox key, so the binding has to come from a signature rather than key identity. darepod stamps the binding header on every outbound envelope from the connector and on the inline response-send path so the server can complete first-contact registration regardless of which Send is the one that triggers it. Closes #448.
239ecea to
8f70cd2
Compare
|
@claude re review this |
|
Claude finished @ellemouton's task in 2m 48s —— View job Re-review: multi: consolidated security fixes (May 15)
Previous Review Items — Both Addressed
New Changes — Chained Multi-Fragment OOR Ancestry
This correctly admits the chained case: two fragments can legally cover the same Ark input index when that input is itself funded by an earlier multi-fragment OOR receive. The test suite adds the key case One minor observation: the The defense-in-depth New Changes — Single-Output Quote Echo ValidationThe
The
New Changes — Forfeit Lookups Detached from Caller Cancellation
Static-Check / Lint Fixes
SummaryAll items from the previous review are addressed. The three substantive new fixes (chained multi-fragment ancestry, single-output quote echo relaxation, forfeit context detachment) are all correct and well-tested. The security properties are intact: ancestry coverage is enforced, the realised-fee cap applies uniformly regardless of implicit-change designation, and TLS binding cannot be silently stripped from the auth flow. LGTM. |
| // Accepting a missing input would leave the uncovered input with no | ||
| // rooted-path material for unilateral exit, stranding the received | ||
| // VTXO if the operator later refuses cooperation. | ||
| for idx, ok := range covered { |
There was a problem hiding this comment.
A simpler variant on this would just be to check a running counter. Tho this is nice as it gives you the exact index that isn't covered in the error.
| // Returns an error only when the VTXOStore lookup for a forfeited | ||
| // VTXO fails; an unset store falls back to the embedded forfeit | ||
| // Amount hint so harness paths without persistence keep working. | ||
| func realisedQuoteFee(ctx context.Context, env *ClientEnvironment, |
| return fmt.Sprintf("leave[%d] non-change "+ | ||
| "amount %d != intent target %d", i, | ||
| entry.AmountSat, leaveReq.Output.Value), false | ||
| if !implicitChange && !leaveReq.IsChange { |
There was a problem hiding this comment.
AFAICT, this is the exact same logic as before.
Perhaps a rebase ended up modifying this for some lint/style issue?
So commit primarily adds extra test coverage.
3 participants
Bundles all of the in-flight client-side security fixes into a single PR to reduce CI load. Each commit closes exactly one issue and is independently reviewable.
Commits
Closes
Closes #366. Closes #370. Closes #371. Closes #372. Closes #374. Closes #378. Closes #379. Closes #381.
Companion server-side PR
The
serverconncommit (closes lightninglabs/darepo#448) is paired with the server-side enforcement in the consolidated darepo PR (https://github.com/lightninglabs/darepo/pull/453).CI scope
This branch combines work that previously lived in 9 separate PRs. CI runs here cover the union of those PRs' diffs; per-commit equivalence to the source PR diffs was independently verified.
Notable cross-PR interactions
round/evaluateQuotesignature: round: enforce fee cap against realised quote economics (#379) #452 addedctx context.Contextas the first parameter; round: tighten single-output quote echo amount validation (#378) #453's tests adopt the new signature.round/quote_echo_test.gohelpersbuildSingleVTXOIntents/buildSingleLeaveIntents(from round: tighten single-output quote echo amount validation (#378) #453) had to be extended with a 100k-sat boarding input so the [security][high] Quoted fee cap can be bypassed via change underpayment #379 realised-fee check accepts honest paths. The test-helper fixup does NOT weaken any production check —validateQuoteEchoesis byte-identical to source.TestEvaluateQuoteRejectsSingleOutputUnderpayment(originating in round: enforce fee cap against realised quote economics (#379) #452) now hits [security][high] Single-output quote echo can underpay fixed recipients #378's earlierimplicit-change amountcheck; assertion widened to accept either error message.Note on superseded PRs
PRs #439, #441, #442, #443, #452, #453, #454, #455, #456 will be closed once this PR is up. Closing them now would lose review history; folks pulling from those branches should rebase onto this one.