rpc: fuzz the OOR and round proto parsers for panic safety#786
rpc: fuzz the OOR and round proto parsers for panic safety#786Roasbeef wants to merge 2 commits into
Conversation
The server's OOR RPC handlers call ParseSubmitPackageRequest and ParseFinalizePackageRequest on fully client-controlled proto bytes, so a panic, slice OOB, or integer overflow in these decode paths is a remote server-crash vector: one hostile client could take down the operator. Add Go native fuzz targets that assert only the no-panic invariant (the parsers may return errors freely). Each parser gets a field-level target that builds the proto in-harness from fuzzer-controlled bytes (ark PSBT, checkpoint PSBT, descriptor outpoint/policy/spend-path, recipient script/value) and a wire-bytes target that runs proto.Unmarshal then the parser to reach shapes the field fuzzer cannot construct. Seeds cover a representative valid value, empty input, and 31/33-byte session-hash edge cases that straddle the fixed-length copy gates. The response parsers are fuzzed too: a compromised operator must not be able to panic the client. No crashers surfaced across all targets.
The round RPC converters decode proto messages that originate from a remote peer, so a panic, OOM, slice OOB, or integer overflow in any decode helper is a process-crash vector. Add Go native fuzz targets asserting only the no-panic invariant for OutpointFromProto, OutpointsFromProto, TxOutFromProto, PSBTFromBytes, MsgTxFromBytes, SchnorrSigFromBytes, TxIDFromHex, OutpointFromMapKey, and TreeFromProto (both a field-level and a wire-bytes target each where useful). TreeFromProto gets extra attention because it reconstructs the recursive VTXO tree from a flattened node array. The wire target pins an explicit WithMaxTreeNodes bound and seeds a many-node corpus so the fuzzer can grow node counts toward and past the cap, proving the bound is enforced rather than ignored and that hostile child-index maps cannot drive OOB or unbounded work. Length-gate seeds straddle 31/33-byte hashes and 63/64/65-byte signatures. No crashers surfaced across all targets.
![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 comprehensive fuzz tests for RPC payload parsers and converters in both the oorpb and roundpb packages to ensure they are resilient against remote crash vectors like panics, OOMs, and out-of-bounds accesses. The feedback suggests expanding the fuzzing coverage to include additional nil pointer edge cases, specifically by testing nil nodes and outputs in VTXOTree and a nil outpoint within the signing descriptors, ensuring the parser logic is fully robust against nil-dereference panics.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| pt := &VTXOTree{ | ||
| Nodes: []*TreeNode{ | ||
| { | ||
| Input: &Outpoint{ | ||
| TxHash: txHash, | ||
| OutputIndex: 0, | ||
| }, | ||
| Outputs: []*TxOut{ | ||
| { | ||
| Value: amount, | ||
| PkScript: pkScript, | ||
| }, | ||
| }, | ||
| CoSigners: [][]byte{coSigner}, | ||
| Children: map[uint32]uint32{ | ||
| childOut: childIdx, | ||
| }, | ||
| Amount: amount, | ||
| Signature: sig, | ||
| }, | ||
| { | ||
| Input: &Outpoint{ | ||
| TxHash: txHash, | ||
| OutputIndex: 1, | ||
| }, | ||
| Outputs: []*TxOut{ | ||
| { | ||
| Value: amount, | ||
| PkScript: pkScript, | ||
| }, | ||
| }, | ||
| Children: map[uint32]uint32{}, | ||
| Amount: amount, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
To ensure the parser is fully panic-safe against nil pointers, we should include nil elements in the Nodes and Outputs slices of the VTXOTree struct, similar to how nil elements are tested in FuzzParseSubmitPackageRequestFields. Currently, TreeFromProto does not check if a node in pt.Nodes is nil before dereferencing it in treeNodeFromProto, which will cause a panic.
pt := &VTXOTree{
Nodes: []*TreeNode{
{
Input: &Outpoint{
TxHash: txHash,
OutputIndex: 0,
},
Outputs: []*TxOut{
{
Value: amount,
PkScript: pkScript,
},
nil,
},
CoSigners: [][]byte{coSigner},
Children: map[uint32]uint32{
childOut: childIdx,
},
Amount: amount,
Signature: sig,
},
nil,
{
Input: &Outpoint{
TxHash: txHash,
OutputIndex: 1,
},
Outputs: []*TxOut{
{
Value: amount,
PkScript: pkScript,
},
},
Children: map[uint32]uint32{},
Amount: amount,
},
},| SigningDescriptors: []*OORSigningDescriptor{ | ||
| { | ||
| Outpoint: &OOROutPoint{ | ||
| Txid: descTxid, | ||
| Vout: descVout, | ||
| }, | ||
| VtxoPolicyTemplate: descPolicy, | ||
| SpendPath: descSpendPath, | ||
| OwnerLeafPolicy: descPolicy, | ||
| }, | ||
| // A nil descriptor entry must be rejected, not | ||
| // dereferenced. | ||
| nil, | ||
| }, |
There was a problem hiding this comment.
To ensure comprehensive fuzz coverage for nil nested fields, we should also include a signing descriptor with a nil Outpoint in the SigningDescriptors slice. This will verify that decodeSigningDescriptor and decodeOutPoint correctly handle nil outpoints without panicking.
SigningDescriptors: []*OORSigningDescriptor{
{
Outpoint: &OOROutPoint{
Txid: descTxid,
Vout: descVout,
},
VtxoPolicyTemplate: descPolicy,
SpendPath: descSpendPath,
OwnerLeafPolicy: descPolicy,
},
{
Outpoint: nil,
VtxoPolicyTemplate: descPolicy,
SpendPath: descSpendPath,
OwnerLeafPolicy: descPolicy,
},
// A nil descriptor entry must be rejected, not
// dereferenced.
nil,
}![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0436b6a10
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| []byte("psbt"), []byte("checkpoint"), validSessionID, | ||
| []byte("policy"), []byte("spendpath"), uint32(0), | ||
| []byte{0x51}, int64(1000), []byte("vtxopolicy"), |
There was a problem hiding this comment.
Every seed in this target supplies invalid Ark/checkpoint PSBT bytes, so ParseSubmitPackageRequest returns from psbtutil.Parse or decodePSBTSlice before it reaches the descriptor and recipient fields this harness is trying to fuzz. Since the request also always includes a nil descriptor before RecipientOutputs are processed, even a future valid-PSBT mutation would still return before the recipient script/value/policy parameters are read; seed with serialized PSBTs and split nil-entry cases into separate parser calls so these fields actually get coverage.
Useful? React with 👍 / 👎.
| PkScript: pkScript, | ||
| }, | ||
| }, | ||
| CoSigners: [][]byte{coSigner}, |
There was a problem hiding this comment.
This field-level tree harness always adds exactly one co-signer entry, but the seed co-signer values are empty or all-zero encodings, and TreeFromProto parses co-signers in treeNodeFromProto before it validates the Children map. Generating a valid compressed secp256k1 key by mutation is impractical, so the childOut/childIdx parameters and the child-index guards are effectively unreachable from the seed corpus; add a no-cosigner/no-signature path or seed a known-valid public key before fuzzing child indices.
Useful? React with 👍 / 👎.
levmi
added
enhancement
|
@Roasbeef, remember to re-request review from reviewers when ready |
3 participants
Motivation
The darepo server's OOR and round RPC handlers parse client-controlled
proto bytes through helpers that live in this module (
rpc/oorpb,rpc/roundpb). A panic in one of those parsers is a server-crash vectorreachable by any client, so they deserve dedicated fuzz coverage.
This PR adds it. It is independent of the TLV-codec hardening in #785.
What this adds
Fuzz harnesses over the RPC proto parsers, driven both field-level
(fuzzer-controlled PSBT / descriptor / outpoint / signature / tree-node
bytes) and via the realistic
proto.Unmarshalwire path:rpc/oorpb:ParseSubmitPackageRequest,ParseFinalizePackageRequest, and the response parsers.rpc/roundpb:OutpointFromProto,TxOutFromProto,PSBTFromBytes,SchnorrSigFromBytes,TxIDFromHex,MsgTxFromBytes, andTreeFromProto.All targets assert the no-panic contract; seeds include valid values,
empty input, off-length hashes/sigs, and (for the tree) oversize child
indices and large node counts.
Findings
No bugs. These parsers already length/shape-check before fixed-size
copies, guard nil sub-messages, and feed btcd's PSBT/wire/schnorr
parsers only through a bounded reader.
TreeFromProtois iterative (nodecode recursion) and enforces its
WithMaxTreeNodescap before anyper-node allocation, which bounds both node count and total allocation;
the pre-order
childIdx > iinvariant keeps the produced graph acyclic.Fuzzing confirmed all of this holds on hostile input.
One out-of-scope note for a possible follow-up:
lib/tree'sNode.Depth()/ForEachare recursive, so a valid ~50k-node linearchain accepted by the parser would recurse deeply in downstream
traversal. That is a
lib/treeconcern (not these parsers) and isbounded in practice by the node cap and Go's growable stacks.
Testing
go build ./rpc/...andgo vet ./rpc/oorpb/ ./rpc/roundpb/clean.Commits are atomic per package.