test(indexer): fuzz the XDR decode path against malformed and hostile input - #548
Open
Salmatcre8 wants to merge 3 commits into
Open
test(indexer): fuzz the XDR decode path against malformed and hostile input#548Salmatcre8 wants to merge 3 commits into
Salmatcre8 wants to merge 3 commits into
Conversation
|
@Salmatcre8 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
…ecoder The decoder degraded quietly in three ways: - scval_to_string and scval_to_json ended in 'other =>' catch-alls that coerced ContractInstance, LedgerKeyContractInstance, and LedgerKeyNonce (and containers in topic position) to Debug strings, bumping a metric no alert rule consumed. A variant the decoder did not know about was stored wrong and nobody was told. - scaddress_to_string fell back to Debug for the muxed-account, claimable-balance, and liquidity-pool address forms added in stellar-xdr 26.x, so those addresses stored as Rust debug dumps instead of strkeys. - crates/backfill carried a stale pre-Telocel-Labs#415 copy of the whole decoder: broken U256/I256 rendering (unpadded hex limb concatenation), no Timepoint/Duration/Error arms, zero tests. A backfilled event could store different values than the live path stored for the same XDR. The decode helpers now live in trident-common::scval, used by both the indexer and backfill, so the two paths cannot diverge again. Every match is exhaustive with no wildcard arm: ScVal is a closed enum, so a new variant introduced by an XDR upgrade fails compilation instead of silently degrading in production — unknown variants are impossible to coerce quietly. Variants that are structurally valid but never legitimately appear in event payloads (ContractInstance and the ledger-key forms) decode faithfully into tagged JSON objects and are surfaced loudly: a warn log names the variant and context, trident_scval_unexpected_variant_total counts it (seeded and described by the indexer's metrics installer, so it is present from first scrape), and the new TridentIndexerUnexpectedScValVariant alert pages on any occurrence — the previous unhandled-variant counter had no consumer at all. Containers in topic position render as canonical JSON rather than Debug (Vec(None) now stores "[]", not "Vec(None)"). Tests cover every new arm with values round-tripped through real XDR encoding, including strkey renderings for all five ScAddress forms and U256/I256 extremes; the existing parser suite (proptests included, still addressed as parser::tests:: by the CI fuzz step) passes unchanged through the re-exports. Closes Telocel-Labs#506
… input The parser consumes untrusted network data through decode_scval, which decoded with Limits::none(): unbounded recursion depth and no byte budget. A hostile deeply-nested payload recursed until the stack overflowed — a SIGABRT, not an Err — and a lying length prefix had no bound tying it to the actual input. Trailing bytes after a valid value were silently accepted, letting production disagree with the testnet-correctness reference path (ScVal::from_xdr), which rejects them. decode_scval is now hardened: - container depth is capped at MAX_SCVAL_DEPTH (100 — the Soroban host caps real values far below this), turning stack-overflow inputs into handled ParseErrors; - input size is capped at MAX_SCVAL_BYTES (2 MiB) and rejected before the base64 decoder allocates for oversized input; the XDR reader's byte budget is the actual input length, so a hostile length claim cannot drive reads or allocation past the payload; - trailing bytes after the value are rejected, aligning the production decoder with the verification path. The fuzz battery covers the shapes the issue names: truncated payloads (every strict prefix of a valid encoding must error), wrong-length fields (arbitrary length prefixes over near-empty payloads), deeply nested values (arbitrary depths render or error — never crash — and depths past the budget always error; a deterministic 50k-deep case proves the SIGABRT is gone), and oversized collections. The randomized properties live in parser::tests, so the existing CI fuzz step (PROPTEST_CASES=50000 on every push and PR touching the workspace) exercises them on every change to the parser; deterministic boundary cases live beside the decoder in trident-common. Verified locally at PROPTEST_CASES=20000 in addition to the default run. Based on Telocel-Labs#506 (the decoder's move to trident-common). Closes Telocel-Labs#507
Salmatcre8
force-pushed
the
test/507-xdr-parser-fuzzing
branch
from
August 30, 2026 13:00
6e8fa6d to
e4c2b54
Compare
Contributor
Author
|
Rebased onto current dev (on top of #547's rebase — still stacked). dev's #219 landed whole-entry-point fuzzing in the same tests region; the two suites are complementary and both survive the merge: #219's |
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
2 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.
Closes #507 (related to #219)
Problem
decode_scvalconsumed untrusted network data withLimits::none(): unbounded recursion depth (a hostile deeply-nested payload overflows the stack — a SIGABRT, not anErr) and no byte budget tying a lying length prefix to the actual input. Trailing bytes after a valid value were silently accepted, diverging from the testnet-correctness reference path (ScVal::from_xdr), which rejects them.What this does
MAX_SCVAL_DEPTH = 500XDR frames — deliberately mirroring soroban-env-host'sDEFAULT_XDR_RW_LIMITS(each container level holds ~4-5 frames, so this admits the full 100 container levels the host itself permits viaDEFAULT_HOST_DEPTH_LIMITwhile still turning stack-overflow inputs into handled errors). Input size capped at 2 MiB and rejected before the base64 decoder allocates; the XDR reader's byte budget is the exact input length.parser::tests(rides the existing CI fuzz step,PROPTEST_CASES=50000, on every change): truncated payloads (every strict prefix must error), trailing garbage, arbitrary nesting depths (never a crash; everything the host can legally emit — ≤100 container levels — must decode, pinned by assertion), lying length prefixes over near-empty payloads, and a deterministic 50,000-deep case proving the SIGABRT is gone.Done-when check
The fuzz target runs clean (verified locally at
PROPTEST_CASES=20000beyond the default), and the existing CI job exercises it on every change to the parser.