fix(frost): harden the cgo native-signer FFI boundary#4115
Merged
mswilkison merged 2 commits intoJun 27, 2026
Merged
Conversation
Three defensive fixes at the Go<->Rust signer FFI boundary in
pkg/frost/signing, so a malformed/oversized response or a panic from the native
signer fails one attempt instead of crashing the node or leaking secrets:
- parseBuildTaggedTBTCSignerResult: bounds-check the response buffer length
before the size_t -> C.int narrowing in C.GoBytes. A length >= 2^31 would
overflow to a negative value and panic ("length out of range") at the cgo
boundary, or silently truncate to a wrong length; reject it (the buffer is
still released by the deferred free).
- the request-call helper: scrub the secret request bytes from the C heap (the
C.CBytes copy) before C.free. The request can carry signing-share / nonce
material, and plain C.free does not overwrite; this mirrors the existing
Go-side zeroBytes hygiene applied to the caller's own copy.
- nativeExecutionFFIExecutorAdapter.Execute: recover panics raised along the
cgo signing path and surface them as a failed attempt, so a single malformed
native-signer response cannot take down the signing process. Adds a unit test
with a panicking primitive (verified to crash the process without the
recover).
The cgo paths are compile-verified under -tags 'frost_native frost_tbtc_signer
cgo'; the recover is exercised by an untagged unit test. The two cgo-tagged
changes still need a runtime pass in the cgo + linked-libfrost_tbtc
environment.
Co-Authored-By: Claude Opus 4.8 (1M context) <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 Plus 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 |
…fi-boundary-hardening
mswilkison
merged commit c741008
into
feat/frost-schnorr-migration-scaffold
17 checks passed
mswilkison
added a commit
that referenced
this pull request
Jun 27, 2026
…ror type (#4122) ## Summary Addresses a Codex finding (relayed via the #4119 review): `TbtcChain.GetWallet` derived a **legacy** wallet ID on **any** error from the canonical `walletID` accessor. For a FROST wallet on a canonical Bridge, a transient call failure would silently yield the left-padded legacy ID — and callers use `WalletChainData.WalletID` to choose **P2TR (FROST)** vs **P2WPKH (legacy)** scripts, so the node would build or search the **wrong wallet script**. ## Why route by scheme (revised after Codex P1) The first revision distinguished by error type (a sentinel for the missing accessor, surface everything else). Codex correctly flagged a **P1 regression**: a *legacy on-chain Bridge* built with the *current* generated binding still satisfies the accessor interface, so its missing `walletID` function returns a normal RPC/ABI error — not the sentinel — and that revision would surface it and **break `GetWallet` on exactly the legacy deployments the fallback exists for**. Error type cannot reliably separate "function absent on-chain" from "transient." So this routes by **scheme**, using the wallet's `EcdsaWalletID` (which `GetWallet` already reads, and which the codebase already uses to infer scheme — zero ⇒ FROST): - **Legacy ECDSA wallet** (`EcdsaWalletID != 0`): its canonical wallet ID *equals* its legacy derivation, so fall back on **any** accessor error — and it's the only option on a legacy Bridge lacking the accessor. - **FROST wallet** (`EcdsaWalletID == 0`): requires the canonical ID; **surface** the error rather than return a wrong legacy ID. A FROST wallet only exists on a canonical-ID Bridge, so such an error is genuinely transient. Logic is extracted into `resolveWalletID(bridge, walletPublicKeyHash, ecdsaWalletID)`. ## Tests `TestResolveWalletID` covers all four cases: accessor success → canonical; FROST + accessor error → surfaced; **legacy + accessor error → legacy fallback** (the P1 regression guard — verified to fail if the routing surfaces errors for legacy wallets); legacy + missing-accessor binding → legacy fallback. gofmt + `go vet` clean; full `pkg/chain/ethereum` suite passes. _Found during the Codex review batch on #4115–#4120; revised per the Codex P1 re-review._ Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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
1 participant
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
Follow-up to #3866 (Go FROST/ROAST coordinator). Three defensive fixes at the cgo Go↔Rust signer FFI boundary in
pkg/frost/signing, so a malformed/oversized response — or a panic — from the native signer fails a single attempt instead of crashing the node or leaking secrets.Fixes
Bounds-check the response length before
C.GoBytes(parseBuildTaggedTBTCSignerResult). The Rust-suppliedbuffer.len(C.size_t) was narrowed toC.intwith no check: a length≥ 2³¹overflows to a negative value →C.GoBytespanics (length out of range) at the cgo boundary, or silently truncates to a wrong length. Now rejected with a clear error (the buffer is still freed by the deferred free).Zeroize the secret request bytes on the C heap before
C.free(the request-call helper).C.CBytes(requestPayload)copies the request (which can carry signing-share / nonce material) to the C heap; plainC.freedoes not overwrite, so the secret lingered in freed memory. Now scrubbed via the existingzeroBytes, mirroring the Go-side hygiene already applied to the caller's own copy.recover()at the FFI boundary (nativeExecutionFFIExecutorAdapter.Execute). A panic anywhere along the cgo signing path (e.g. fix Initial CircleCI setup #1's overflow panic, or a nil-deref decoding a malformed engine response) previously took down the whole signing process. It's now converted to a failed attempt the outer tBTCsigningRetryLoophandles cleanly.Tests / verification
TestNativeExecutionFFIExecutorAdapter_Execute_RecoversCgoBoundaryPanic— a panicking primitive; verified to crash the process without the recover and pass with it. Full untaggedpkg/frost/signingsuite stays green.-tags 'frost_native frost_tbtc_signer cgo'(build +go vet). They still need a runtime pass in the cgo + linked-libfrost_tbtcenvironment — I can't exercise the real FFI here.Addresses the cgo-boundary cluster from the review of #3866 (length-narrowing crash, secret-in-C-heap, missing recover). Found during review of #3866.
🤖 Generated with Claude Code