fix: return Err on malformed proofs and out-of-range params instead of panicking#14
Open
mwaddip wants to merge 1 commit into
Open
fix: return Err on malformed proofs and out-of-range params instead of panicking#14mwaddip wants to merge 1 commit into
mwaddip wants to merge 1 commit into
Conversation
…f panicking `BatchAVLVerifier` tree reconstruction and the value-length checks in the modify path used raw slice indexing, `stack.pop().unwrap()`, and `assert!`, so a malformed / truncated / empty proof, an out-of-range key length, or an operation whose value length does not match the tree's fixed value length panicked instead of failing gracefully. The reference (scorex) `BatchAVLVerifier` wraps reconstruction in a `Try` and treats any failure as "no reconstructed tree", so downstream consumers (e.g. sigma's `CErgoTreeEvaluator` / `CAvlTreeVerifier`) never observe a panic — they see a verifier whose operations fail and whose digest is `None`. Honor the crate's existing `Result` / `ensure!` contract at the panic sites so the same inputs return `Err` here too: - `reconstruct_tree`: bounds-checked proof reads via a `read_proof_slice` helper (with `checked_add` to avoid length overflow), a guarded loop bound, and `ok_or` on the stack pops. - `modify_helper`: `assert!` -> `ensure!` on the value-length checks. Adds `tests/malformed_proof.rs` covering empty / garbage / truncated proofs, an oversized key length, and a wrong-length operation value. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author
|
Consumer side: ergoplatform/sigma-rust#892 (eval-layer regression tests, Draft). These should merge together — that PR's tests exercise this fix through sigma-rust's AVL evaluator and are |
mwaddip
added a commit
to mwaddip/sigma-rust
that referenced
this pull request
Jun 7, 2026
…annot start Scala's CAvlTreeVerifier.treeHeight reads BatchAVLVerifier.rootNodeHeight, which is assigned from the digest's trailing byte only after reconstruction's up-front requires (keyLength > 0, digest length) pass. A non-positive keyLength (signed Int on the JVM; wire values with the high bit set) fails before the assignment, so the JVM charges the degenerate tree a zero-height walk. We read the digest byte unconditionally, overcharging the height-scaled Lookup/Insert/Update/Remove ops on those shapes. Failures during proof parsing (malformed proof bytes, wrong value length) happen after the assignment and correctly keep the digest-derived height. The degenerate path is observable end-to-end only with ergoplatform/ergo_avltree_rust#14 (the crates.io verifier panics on these inputs before any per-op charge is reached); pinned here at the unit level. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mwaddip
added a commit
to mwaddip/sigma-rust
that referenced
this pull request
Jun 7, 2026
…annot start Scala's CAvlTreeVerifier.treeHeight reads BatchAVLVerifier.rootNodeHeight, which is assigned from the digest's trailing byte only after reconstruction's up-front requires (keyLength > 0, digest length) pass. A non-positive keyLength (signed Int on the JVM; wire values with the high bit set) fails before the assignment, so the JVM charges the degenerate tree a zero-height walk. We read the digest byte unconditionally, overcharging the height-scaled Lookup/Insert/Update/Remove ops on those shapes. Failures during proof parsing (malformed proof bytes, wrong value length) happen after the assignment and correctly keep the digest-derived height. The degenerate path is observable end-to-end only with ergoplatform/ergo_avltree_rust#14 (the crates.io verifier panics on these inputs before any per-op charge is reached); pinned here at the unit level. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jun 7, 2026
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
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.
Problem
BatchAVLVerifierpanics on several classes of malformed input instead of returning an error:stack.pop().unwrap()panic during tree reconstructionassert!panic in the modify pathThe reference Scala/scorex
BatchAVLVerifierwraps reconstruction in aTryand treats any failure as "no reconstructed tree", so its consumers (sigma'sCErgoTreeEvaluator/CAvlTreeVerifier) never observe a panic — a failed construction yields a verifier whose operations fail and whosedigestisNone. Because this crate panics instead, a consumer that feeds it crafted / attacker-supplied proof bytes (e.g. sigma-rust evaluating anAvlTreeoperation) crashes rather than failing the operation cleanly — a crash-on-deserialize / DoS risk.Fix
Honor the crate's existing
Result/ensure!contract at the panic sites so the same inputs returnErr:reconstruct_tree: bounds-checked proof reads via a smallread_proof_slicehelper (withchecked_addagainst length overflow), a guarded loop bound, andok_oron the stack pops.modify_helper:assert!→ensure!on the two value-length checks.No behavior change for valid proofs — the new checks only fire where the code would previously have panicked. Works under
no_stdand is unaffected bypanic = "abort".Tests
tests/malformed_proof.rs— empty, single-garbage-byte, truncated-label, and truncated-real proofs; an oversized key length; and a wrong-length operation value. Each assertsErrrather than a panic.Found via SANTA conformance testing of the sigma-rust port against the JVM oracle.