multi: harden TLV decoders against unbounded-allocation DoS, add fuzzing#785
Open
Roasbeef wants to merge 10 commits into
Open
multi: harden TLV decoders against unbounded-allocation DoS, add fuzzing#785Roasbeef wants to merge 10 commits into
Roasbeef wants to merge 10 commits into
Conversation
The client ledger actor decodes TLV messages that cross the client/server trust boundary and are replayed from the durable mailbox on restart. The lnd/tlv stream sizes its value buffers with make([]byte, declaredLength) before reading any value bytes -- both for unknown records (stream.go, under DecodeWithParsedTypes) and known []byte records (DVarBytes) -- and never caps that length. A handful of attacker-controlled bytes declaring a near-2^64 record length therefore panic the decoder with "makeslice: cap out of range", and a multi-gigabyte length drives the process to OOM. Read each message into a size-capped buffer and pre-validate the (type, length) framing before handing it to the decoder, rejecting any record whose declared length exceeds the bytes physically present. This caps every allocation at maxLedgerMessageSize and mirrors the server-side ledger fix. A fuzz harness over all six message decoders exercises the bound.
AckState and the wrapped-proto record decode bytes read back from the durable checkpoint store, so a corrupt or adversarial blob is attacker-reachable. As elsewhere, the lnd/tlv decoder allocates make([]byte, declaredLength) from the wire length before reading any bytes, so a tiny payload declaring a huge record length panics or OOMs the process. Route AckState.Decode through a size-capped, framing-validated reader and bound the wrapped-proto length against the message cap before allocating. Fuzz harnesses cover both decoders.
The serverconn connector decodes event, RPC, and unary-query TLV payloads that originate across the trust boundary and replay from the durable mailbox. Two unbounded-allocation vectors existed: the lnd/tlv decoder sizes value buffers from the declared record length before reading any bytes, and the durable unary query codec read a length-prefixed blob (and a blob-list count) straight from input into make() without bounds -- a few bytes could request a multi-gigabyte allocation. Route every Decode through a size-capped, framing-validated reader, and bound the blob length and list count against the bytes remaining in the reader before allocating. Fuzz harnesses cover the event/RPC/unary and list decoders.
The VTXO tree codec deserializes blobs read back from the database and
re-parsed, so a corrupt or adversarial row is attacker-reachable.
make([]byte, recordLength) in the lnd/tlv decoder and several
count-prefixed make([]T, count) sites (tree nodes, txouts, pubkeys)
could each turn a few bytes into a multi-gigabyte allocation
("makeslice" panic or OOM).
Route the tree and node decoders through a size-capped,
framing-validated reader and bound every decoded element count against
the bytes remaining before allocating, on top of the existing depth and
children-per-node limits. A fuzz harness over the tree deserializer
exercises the bounds.
The OOR durable actor messages, snapshots, and package artifacts decode TLV payloads that cross the client/server trust boundary and persist in the durable mailbox. The lnd/tlv decoder allocates make([]byte, declaredLength) from the wire length before reading any bytes, and the nested length-prefixed blob lists read a per-element length (and list count) into make() without bounds -- both reachable from a tiny hostile payload that panics or OOMs the process. Add a shared size-capped, framing-validated bounded-decode choke point (maxOORMessageSize is sized for the legitimate worst case of many 64 KiB aggregated PSBT blobs, which DecodeP2P's per-record cap would wrongly reject) and route every OOR DecodeWithParsedTypes through it; bound the blob-list count and per-element length against remaining input. A fuzz harness covers the durable-message, snapshot, and artifact decoders.
The actor envelope codec (MessageCodec) is the top-level decoder for every durable mailbox message, and AskResponse/RestartMessage carry []byte fields; all decode bytes replayed from disk across upgrades. The lnd/tlv decoder sizes make([]byte, declaredLength) from the wire length before reading any bytes -- including the unknown-record path under DecodeWithParsedTypes, which crashes even a scalar-only message -- so a tiny hostile payload panics or OOMs the process on replay. Bound the envelope payload length, and route the actor message decoders through a size-capped, framing-validated reader. The counter test messages get the same guard plus a fuzz harness, which surfaced that the unknown-record path is reachable regardless of a message's own field set.
DecodeJoinRoundAuthMessage and DeserializeTxProof parse bytes that cross the client/server trust boundary. The lnd/tlv decoder sizes make([]byte, declaredLength) from the wire length before reading any bytes, so a tiny payload declaring a near-2^64 record length panics with "makeslice: cap out of range" or OOMs the process. Read each payload into a size-capped buffer and pre-validate the (type, length) framing before decoding, rejecting any record whose declared length exceeds the bytes present. The existing per-entry blob-list bound is retained. Fuzz harnesses cover the auth message and tx-proof decoders.
The recovery proof and session-state codecs decode persisted bytes that can be corrupt or adversarial. The lnd/tlv decoder allocates make([]byte, declaredLength) from the wire length before reading any bytes, so a tiny hostile payload panics or OOMs the process. Read each payload into a size-capped buffer (sized for the legitimate worst case of MaxProofNodes) and pre-validate the (type, length) framing before decoding. Fuzz harnesses cover the proof and session-state decoders, plus the bip322 signature decoder (DecodeSig), which parses a remote peer's BIP-322 signature through btcd's internally-bounded wire.MsgTx deserializer.
DecodeTapTree and the OOR submit-package decoder parse bytes that cross the trust boundary and persist durably. The lnd/tlv decoder sizes make([]byte, declaredLength) from the wire length before reading any bytes, the per-leaf taptree scripts and the package blob list read a length/count straight from input into make() without bounds, and the blob reader accepted short reads -- so a tiny hostile payload panics, OOMs, or silently decodes truncated data. Route the taptree and package decoders through a size-capped, framing-validated reader, bound every blob count and length against the bytes remaining, and use io.ReadFull where the buffer must be filled. Fuzz harnesses cover the taptree and submit-package decoders.
The unroll mailbox messages, deferred-checkpoint codec, and unrollplan state codec decode bytes that persist in the durable mailbox and replay across upgrades. The lnd/tlv decoder allocates make([]byte, declaredLength) from the wire length before reading any bytes, the deferred-checkpoint codec read a count and per-entry length into make() without bounds and used a short-read-prone reader.Read, and the state codec decoded length-prefixed lists unbounded -- each reachable from a tiny hostile payload that panics or OOMs the process. Route every decoder through a size-capped, framing-validated reader, bound counts and lengths against the bytes remaining before allocating, and switch the entry reader to io.ReadFull. Fuzz harnesses cover the unroll messages, deferred checkpoints, and unrollplan state.
![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 hardens the TLV (Type-Length-Value) decoding paths across multiple packages to prevent unbounded-allocation DoS vulnerabilities, panics (such as 'makeslice: cap out of range'), and OOM errors. It introduces pre-validation helpers (e.g., safeTLVReader, validateTLVRecordLengths) that verify declared record lengths against the actual remaining bytes in the payload before any memory allocation occurs. Additionally, the PR adds a comprehensive suite of fuzz tests to ensure robust, panic-free decoding of untrusted inputs across all affected modules. No review comments were provided, so there is no feedback to address.
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.
![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: de1abf970c
ℹ️ 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".
| // BIP-322-signed join intent; TxProof is a boarding SPV proof the server | ||
| // verifies) and TxProof additionally persists durably, so the bound is | ||
| // load-bearing on both the live and replay path. | ||
| const maxTypesMessageSize = 16 << 20 |
There was a problem hiding this comment.
This 16 MiB cap is lower than this package's own accepted join-auth limits: decodeBoundedBlobList still allows each of the boarding/VTXO/forfeit/leave lists to contain up to 1024 entries of up to 64 KiB each. A signed JoinRoundAuth with a single list a little over 16 MiB (for example 1024 entries around 17 KiB, all below the per-entry limit) now fails in DecodeJoinRoundAuthMessage before the existing per-list validation runs, even though it was within the advertised limits. Please derive the total cap from the existing list limits or lower those limits consistently so large but valid join requests are not rejected.
Useful? React with 👍 / 👎.
Roasbeef
force-pushed
the
fuzz-tlv-serialization
branch
from
June 24, 2026 00:18
99eaf34 to
de1abf9
Compare
Collaborator
|
@Roasbeef, remember to re-request review from reviewers when ready |
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
3 participants
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.
Motivation
Every TLV decoder in the client parses bytes that either cross the
client/server trust boundary or are replayed from a durable actor
mailbox on restart. A single malformed payload that reaches one of these
decoders can take the process down, and because durable messages are
replayed from disk on every restart, one poisoned mailbox entry becomes
a permanent crash loop.
The bug
lnd/tlvsizes its value buffers withmake([]byte, declaredLength)before reading a single value byte. This happens in two spots:
DVarBytes, used for every known[]byterecord, andstream.go, which fires underDecodeWithParsedTypes.Neither path on the non-P2P decode API caps the declared length. So
roughly ten attacker-controlled bytes that declare a record length near
2^63 panic the decoder with
makeslice: cap out of range, and anin-range-but-huge length drives the process to OOM. The unknown-record
path is the nasty part: even a message whose own fields are all
fixed-width scalars is vulnerable, because an injected unknown odd type
carries its own attacker-chosen length. We confirmed this is reachable
from essentially every decoder in the client.
The fix
Each package gets a bounded-decode guard:
DecodeWithParsedTypesread the payload into asize-capped buffer and pre-validate the
(type, length)framing,rejecting any record whose declared length exceeds the bytes
physically present, before the real decoder ever allocates;
make([]T, count)/make([]byte, size)sites bound thecount or length against the bytes remaining in the reader;
reader.Readcalls becomeio.ReadFull.The caps are per-package and generous (OOR, for instance, is sized for
many aggregated 64 KiB PSBT blobs, which
DecodeP2P's fixed per-recordcap would wrongly reject), so well-formed payloads of any realistic size
still decode; only genuinely corrupt framing is rejected, with a clean
wrapped error.
Fuzzing
Each affected package gets a native Go fuzz harness over its decode
entry points. Every target seeds a valid encoding, the empty input, and
the canonical malicious-length payload, so the bound runs as a
deterministic regression on every
go test. The crashers that surfacedduring development are kept as regression corpus.
Packages hardened
ledger,mailbox/conn,serverconn,db(tree codec),oor,baselib/actor+internal/actortest,lib/types,lib/recovery(+ a
lib/bip322signature fuzz harness),lib/tx/checkpoint,lib/tx/oor,unroll, andunrollplan.indexeris encode-only andneeds no change.
Testing
go buildandgo vetclean across both modules.Commits are atomic per package.