fix(chain): reject coinbase Sapling spends during deserialization - #10527
Merged
Conversation
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
upbqdn
force-pushed
the
coinbase-data-alloc
branch
from
May 1, 2026 09:53
0fa4c13 to
043612b
Compare
upbqdn
force-pushed
the
fix/ghsa-rgwx-8r98-p34c
branch
from
May 1, 2026 09:53
23ca0b1 to
3c5a047
Compare
mpguerra
previously approved these changes
May 1, 2026
upbqdn
force-pushed
the
coinbase-data-alloc
branch
from
May 1, 2026 13:15
043612b to
cff74ac
Compare
upbqdn
force-pushed
the
fix/ghsa-rgwx-8r98-p34c
branch
from
May 1, 2026 13:15
3c5a047 to
5f5b8d0
Compare
upbqdn
force-pushed
the
coinbase-data-alloc
branch
from
May 1, 2026 15:25
cff74ac to
b21321d
Compare
upbqdn
force-pushed
the
fix/ghsa-rgwx-8r98-p34c
branch
from
May 1, 2026 15:25
5f5b8d0 to
0ad6ff6
Compare
conradoplg
force-pushed
the
coinbase-data-alloc
branch
from
May 1, 2026 17:00
b21321d to
6246fd5
Compare
…ejection Update transaction_roundtrip, block_roundtrip, and the reproduction vector test to assert that coinbase transactions with Sapling spends are rejected during deserialization. The arbitrary generators are intentionally left unchanged so they continue producing the invalid combination — the proptests now verify rejection for that case and round-trip for all others.
conradoplg
force-pushed
the
fix/ghsa-rgwx-8r98-p34c
branch
from
May 1, 2026 17:04
0ad6ff6 to
397ab72
Compare
upbqdn
added a commit
that referenced
this pull request
May 1, 2026
…ction proptests The `any::<Transaction>()` arbitrary strategy can produce coinbase transactions that contain Sapling spends. Since #10527 (GHSA-rgwx-8r98-p34c), those transactions are rejected during deserialization, so `send_raw_transaction` returns a deserialization error before reaching the mempool. The four affected proptests then time out waiting for a mempool request that never arrives. Skip those cases with `prop_assume!` so each test exercises the mempool path it is intended to cover. The `transaction_roundtrip` proptest in `zebra-chain` was updated to handle the same combination in #10527.
6 tasks
upbqdn
added a commit
that referenced
this pull request
May 1, 2026
…ction proptests The `any::<Transaction>()` arbitrary strategy can produce coinbase transactions that contain Sapling spends. Since #10527 (GHSA-rgwx-8r98-p34c), those transactions are rejected during deserialization, so `send_raw_transaction` returns a deserialization error before reaching the mempool. The four affected proptests then time out waiting for a mempool request that never arrives. Skip those cases with `prop_assume!` so each test exercises the mempool path it is intended to cover. The `transaction_roundtrip` proptest in `zebra-chain` was updated to handle the same combination in #10527.
upbqdn
added a commit
that referenced
this pull request
May 1, 2026
…ction proptests The `any::<Transaction>()` arbitrary strategy can produce coinbase transactions that contain Sapling spends. Since #10527 (GHSA-rgwx-8r98-p34c), those transactions are rejected during deserialization, so `send_raw_transaction` returns a deserialization error before reaching the mempool. The five affected proptests then time out waiting for a mempool request that never arrives. Skip those cases with `prop_assume!` so each test exercises the mempool path it is intended to cover. The `transaction_roundtrip` proptest in `zebra-chain` was updated to handle the same combination in #10527.
upbqdn
added a commit
that referenced
this pull request
May 1, 2026
After #10527 (GHSA-rgwx-8r98-p34c) rejects coinbase transactions with Sapling spends during deserialization, three `zebra-state` proptests panic when arbitrary chains contain such transactions: - `roundtrip_transaction` reads back a generated `Transaction` via `IntoDisk`/`FromDisk` and panics in `FromDisk::from_bytes`. - `blocks_with_v5_transactions` and `all_upgrades_and_wrong_commitments_with_fake_activation_heights` commit chains generated by `PreparedChain` to the finalized state, whose underlying `partial_chain_strategy` produces coinbase transactions with arbitrary Sapling shielded data. Fix the chain-builder path by clearing `sapling_shielded_data` on coinbase transactions in `fix_generated_transaction`. Add a `prop_assume!` to `roundtrip_transaction` since it bypasses the chain builder and uses `any::<Transaction>()` directly. The `Transaction::Arbitrary` strategy is intentionally left unchanged so the rejection path is still exercised by the `transaction_roundtrip` proptest and the GHSA-rgwx-8r98-p34c reproduction vector in `zebra-chain` (per the design choice in #10527).
conradoplg
pushed a commit
that referenced
this pull request
May 1, 2026
#10527 (#10533) * test(rpc): skip invalid coinbase Sapling-spend txs in send_raw_transaction proptests The `any::<Transaction>()` arbitrary strategy can produce coinbase transactions that contain Sapling spends. Since #10527 (GHSA-rgwx-8r98-p34c), those transactions are rejected during deserialization, so `send_raw_transaction` returns a deserialization error before reaching the mempool. The five affected proptests then time out waiting for a mempool request that never arrives. Skip those cases with `prop_assume!` so each test exercises the mempool path it is intended to cover. The `transaction_roundtrip` proptest in `zebra-chain` was updated to handle the same combination in #10527. * style: rustfmt drift in inbound/downloads.rs * test(state): handle coinbase Sapling spends in finalized-state proptests After #10527 (GHSA-rgwx-8r98-p34c) rejects coinbase transactions with Sapling spends during deserialization, three `zebra-state` proptests panic when arbitrary chains contain such transactions: - `roundtrip_transaction` reads back a generated `Transaction` via `IntoDisk`/`FromDisk` and panics in `FromDisk::from_bytes`. - `blocks_with_v5_transactions` and `all_upgrades_and_wrong_commitments_with_fake_activation_heights` commit chains generated by `PreparedChain` to the finalized state, whose underlying `partial_chain_strategy` produces coinbase transactions with arbitrary Sapling shielded data. Fix the chain-builder path by clearing `sapling_shielded_data` on coinbase transactions in `fix_generated_transaction`. Add a `prop_assume!` to `roundtrip_transaction` since it bypasses the chain builder and uses `any::<Transaction>()` directly. The `Transaction::Arbitrary` strategy is intentionally left unchanged so the rejection path is still exercised by the `transaction_roundtrip` proptest and the GHSA-rgwx-8r98-p34c reproduction vector in `zebra-chain` (per the design choice in #10527).
mergify Bot
pushed a commit
that referenced
this pull request
May 1, 2026
* test(rpc): skip invalid coinbase Sapling-spend txs in send_raw_transaction proptests The `any::<Transaction>()` arbitrary strategy can produce coinbase transactions that contain Sapling spends. Since #10527 (GHSA-rgwx-8r98-p34c), those transactions are rejected during deserialization, so `send_raw_transaction` returns a deserialization error before reaching the mempool. The five affected proptests then time out waiting for a mempool request that never arrives. Skip those cases with `prop_assume!` so each test exercises the mempool path it is intended to cover. The `transaction_roundtrip` proptest in `zebra-chain` was updated to handle the same combination in #10527. * style: rustfmt drift in inbound/downloads.rs * test(state): handle coinbase Sapling spends in finalized-state proptests After #10527 (GHSA-rgwx-8r98-p34c) rejects coinbase transactions with Sapling spends during deserialization, three `zebra-state` proptests panic when arbitrary chains contain such transactions: - `roundtrip_transaction` reads back a generated `Transaction` via `IntoDisk`/`FromDisk` and panics in `FromDisk::from_bytes`. - `blocks_with_v5_transactions` and `all_upgrades_and_wrong_commitments_with_fake_activation_heights` commit chains generated by `PreparedChain` to the finalized state, whose underlying `partial_chain_strategy` produces coinbase transactions with arbitrary Sapling shielded data. Fix the chain-builder path by clearing `sapling_shielded_data` on coinbase transactions in `fix_generated_transaction`. Add a `prop_assume!` to `roundtrip_transaction` since it bypasses the chain builder and uses `any::<Transaction>()` directly. The `Transaction::Arbitrary` strategy is intentionally left unchanged so the rejection path is still exercised by the `transaction_roundtrip` proptest and the GHSA-rgwx-8r98-p34c reproduction vector in `zebra-chain` (per the design choice in #10527). * docs(changelog): prepare 4.4.0 release notes * docs(changelog): consolidate allocation-amplification advisories into GHSA-438q-jx8f-cccv * docs(changelog): cover all API changes since 4.3.1 Audited against `check-api.sh 4.3.1 HEAD` and the PR list since 4.3.1. Workspace CHANGELOG.md: - Add Security entry for the stalled-peer-disconnect change merged from the private fork (FindBlocks/FindHeaders stall classification in PeerSet). zebra-chain: - Consolidate `[7.0.0] - PLANNED` into `[Unreleased]` so the 4.4.0 cut isn't split across two sections. - Document new `serialization::MAX_HEADERS_PER_MESSAGE` constant. zebra-consensus: - Document groth16 module cleanup: removed `DescriptionWrapper`, `SAPLING`, `Description` trait + impl, and the `TryFrom<DescriptionWrapper<&T>>` conversions; added `groth16::Item::from_joinsplit`. zebra-network: - Document the new `Option<PeerSocketAddr>` field on `Request::AdvertiseBlock`. zebra-rpc: - Consolidate `[7.0.0] - PLANNED` into `[Unreleased]`. - Document `BlockObject::new` gaining a required `n_tx: usize` parameter and the new `BlockObject::n_tx` accessor. - Document `HttpRequestMiddleware{,Layer}::new` gaining a required `max_request_body_size: usize` parameter (GHSA-8r29-5wjm-jgvx). zebra-script: - Consolidate `[6.0.0] - PLANNED` into `[Unreleased]`. * release: bump crate versions for Zebra 4.4.0 Drove the bumps via `cargo release version` per the release checklist. - zebra-chain 6.0.2 -> 7.0.0 (major: VerifiedUnminedTx::new signature, librustzcash 0.27) - zebra-consensus 5.0.2 -> 6.0.0 (major: groth16 module cleanup) - zebra-network 5.0.1 -> 6.0.0 (major: Request::AdvertiseBlock tuple shape) - zebra-rpc 6.0.2 -> 7.0.0 (major: BlockObject::new + middleware constructors) - zebra-script 5.0.1 -> 6.0.0 (major: Sigops::scripts return type, librustzcash 0.27) - zebra-state 5.0.0 -> 5.0.1 (patch: internal-only changes) - zebrad 4.3.1 -> 4.4.0 (minor: nTx field, security fixes, sentry module made crate-private) Also ran `cargo release replace -p zebrad` (rewrote the README install tag to v4.4.0) and refreshed Cargo.lock with `cargo update -w`. Verified with `cargo check --workspace --locked` on d.lan. * docs(changelog): finalize crate changelog headers for 4.4.0 release Replace `[Unreleased]` placeholders with versioned entries dated 2026-05-01: - zebra-chain -> [7.0.0] - zebra-consensus -> [6.0.0] - zebra-network -> [6.0.0] (added security context for the inbound deserializer hardening cohort) - zebra-rpc -> [7.0.0] (added Security section listing the four RPC advisories fixed in this cycle) - zebra-script -> [6.0.0] - zebra-state -> [5.0.1] (note: internal-only changes, no public-API delta) * docs(changelog): adopt librustzcash changelog style for 4.4.0 entries - Drop the `### Breaking Changes` header in favor of `### Added`/`### Changed`/ `### Removed` (matches `~/zcash/librustzcash/*/CHANGELOG.md` convention). - Group sub-items under module-prefix headers ending with `:`. - zebra-consensus groth16 cleanup is split into a `### Removed` section. - Wrap entries at ~78 chars and use `impl Trait for Type` form for impl items. * docs(changelog): prune trivial detail in 4.4.0 entry - Shorten the consensus-divergence sighash defense-in-depth bullet; the long technical explanation lives in the GHSA, not the operator changelog. - Merge the Sentry SDK upgrade, Sentry CI metadata, OpenTelemetry default- release-binaries inclusion, and `zebrad::sentry` privatization (all from PR #10490) into a single observability-refresh bullet. * docs(changelog): pad librustzcash-style subheadings for markdown-lint Add blank lines around the `### Added` / `### Changed` / `### Removed` subheadings introduced in the 4.4.0 entries (MD022/MD032).
judah-caruso
pushed a commit
to ShieldedLabs/zebra-crosslink-staging
that referenced
this pull request
May 28, 2026
…ashFoundation#10527) * test: add reproduction for GHSA-rgwx-8r98-p34c coinbase Sapling spend allocation gap * fix: reject coinbase Sapling spends during deserialization (GHSA-rgwx-8r98-p34c) * test: update proptests and vectors to expect coinbase Sapling spend rejection Update transaction_roundtrip, block_roundtrip, and the reproduction vector test to assert that coinbase transactions with Sapling spends are rejected during deserialization. The arbitrary generators are intentionally left unchanged so they continue producing the invalid combination — the proptests now verify rejection for that case and round-trip for all others. --------- Co-authored-by: Alfredo Garcia <oxarbitrage@gmail.com>
judah-caruso
pushed a commit
to ShieldedLabs/zebra-crosslink-staging
that referenced
this pull request
May 28, 2026
ZcashFoundation#10527 (ZcashFoundation#10533) * test(rpc): skip invalid coinbase Sapling-spend txs in send_raw_transaction proptests The `any::<Transaction>()` arbitrary strategy can produce coinbase transactions that contain Sapling spends. Since ZcashFoundation#10527 (GHSA-rgwx-8r98-p34c), those transactions are rejected during deserialization, so `send_raw_transaction` returns a deserialization error before reaching the mempool. The five affected proptests then time out waiting for a mempool request that never arrives. Skip those cases with `prop_assume!` so each test exercises the mempool path it is intended to cover. The `transaction_roundtrip` proptest in `zebra-chain` was updated to handle the same combination in ZcashFoundation#10527. * style: rustfmt drift in inbound/downloads.rs * test(state): handle coinbase Sapling spends in finalized-state proptests After ZcashFoundation#10527 (GHSA-rgwx-8r98-p34c) rejects coinbase transactions with Sapling spends during deserialization, three `zebra-state` proptests panic when arbitrary chains contain such transactions: - `roundtrip_transaction` reads back a generated `Transaction` via `IntoDisk`/`FromDisk` and panics in `FromDisk::from_bytes`. - `blocks_with_v5_transactions` and `all_upgrades_and_wrong_commitments_with_fake_activation_heights` commit chains generated by `PreparedChain` to the finalized state, whose underlying `partial_chain_strategy` produces coinbase transactions with arbitrary Sapling shielded data. Fix the chain-builder path by clearing `sapling_shielded_data` on coinbase transactions in `fix_generated_transaction`. Add a `prop_assume!` to `roundtrip_transaction` since it bypasses the chain builder and uses `any::<Transaction>()` directly. The `Transaction::Arbitrary` strategy is intentionally left unchanged so the rejection path is still exercised by the `transaction_roundtrip` proptest and the GHSA-rgwx-8r98-p34c reproduction vector in `zebra-chain` (per the design choice in ZcashFoundation#10527).
judah-caruso
pushed a commit
to ShieldedLabs/zebra-crosslink-staging
that referenced
this pull request
May 28, 2026
* test(rpc): skip invalid coinbase Sapling-spend txs in send_raw_transaction proptests The `any::<Transaction>()` arbitrary strategy can produce coinbase transactions that contain Sapling spends. Since ZcashFoundation#10527 (GHSA-rgwx-8r98-p34c), those transactions are rejected during deserialization, so `send_raw_transaction` returns a deserialization error before reaching the mempool. The five affected proptests then time out waiting for a mempool request that never arrives. Skip those cases with `prop_assume!` so each test exercises the mempool path it is intended to cover. The `transaction_roundtrip` proptest in `zebra-chain` was updated to handle the same combination in ZcashFoundation#10527. * style: rustfmt drift in inbound/downloads.rs * test(state): handle coinbase Sapling spends in finalized-state proptests After ZcashFoundation#10527 (GHSA-rgwx-8r98-p34c) rejects coinbase transactions with Sapling spends during deserialization, three `zebra-state` proptests panic when arbitrary chains contain such transactions: - `roundtrip_transaction` reads back a generated `Transaction` via `IntoDisk`/`FromDisk` and panics in `FromDisk::from_bytes`. - `blocks_with_v5_transactions` and `all_upgrades_and_wrong_commitments_with_fake_activation_heights` commit chains generated by `PreparedChain` to the finalized state, whose underlying `partial_chain_strategy` produces coinbase transactions with arbitrary Sapling shielded data. Fix the chain-builder path by clearing `sapling_shielded_data` on coinbase transactions in `fix_generated_transaction`. Add a `prop_assume!` to `roundtrip_transaction` since it bypasses the chain builder and uses `any::<Transaction>()` directly. The `Transaction::Arbitrary` strategy is intentionally left unchanged so the rejection path is still exercised by the `transaction_roundtrip` proptest and the GHSA-rgwx-8r98-p34c reproduction vector in `zebra-chain` (per the design choice in ZcashFoundation#10527). * docs(changelog): prepare 4.4.0 release notes * docs(changelog): consolidate allocation-amplification advisories into GHSA-438q-jx8f-cccv * docs(changelog): cover all API changes since 4.3.1 Audited against `check-api.sh 4.3.1 HEAD` and the PR list since 4.3.1. Workspace CHANGELOG.md: - Add Security entry for the stalled-peer-disconnect change merged from the private fork (FindBlocks/FindHeaders stall classification in PeerSet). zebra-chain: - Consolidate `[7.0.0] - PLANNED` into `[Unreleased]` so the 4.4.0 cut isn't split across two sections. - Document new `serialization::MAX_HEADERS_PER_MESSAGE` constant. zebra-consensus: - Document groth16 module cleanup: removed `DescriptionWrapper`, `SAPLING`, `Description` trait + impl, and the `TryFrom<DescriptionWrapper<&T>>` conversions; added `groth16::Item::from_joinsplit`. zebra-network: - Document the new `Option<PeerSocketAddr>` field on `Request::AdvertiseBlock`. zebra-rpc: - Consolidate `[7.0.0] - PLANNED` into `[Unreleased]`. - Document `BlockObject::new` gaining a required `n_tx: usize` parameter and the new `BlockObject::n_tx` accessor. - Document `HttpRequestMiddleware{,Layer}::new` gaining a required `max_request_body_size: usize` parameter (GHSA-8r29-5wjm-jgvx). zebra-script: - Consolidate `[6.0.0] - PLANNED` into `[Unreleased]`. * release: bump crate versions for Zebra 4.4.0 Drove the bumps via `cargo release version` per the release checklist. - zebra-chain 6.0.2 -> 7.0.0 (major: VerifiedUnminedTx::new signature, librustzcash 0.27) - zebra-consensus 5.0.2 -> 6.0.0 (major: groth16 module cleanup) - zebra-network 5.0.1 -> 6.0.0 (major: Request::AdvertiseBlock tuple shape) - zebra-rpc 6.0.2 -> 7.0.0 (major: BlockObject::new + middleware constructors) - zebra-script 5.0.1 -> 6.0.0 (major: Sigops::scripts return type, librustzcash 0.27) - zebra-state 5.0.0 -> 5.0.1 (patch: internal-only changes) - zebrad 4.3.1 -> 4.4.0 (minor: nTx field, security fixes, sentry module made crate-private) Also ran `cargo release replace -p zebrad` (rewrote the README install tag to v4.4.0) and refreshed Cargo.lock with `cargo update -w`. Verified with `cargo check --workspace --locked` on d.lan. * docs(changelog): finalize crate changelog headers for 4.4.0 release Replace `[Unreleased]` placeholders with versioned entries dated 2026-05-01: - zebra-chain -> [7.0.0] - zebra-consensus -> [6.0.0] - zebra-network -> [6.0.0] (added security context for the inbound deserializer hardening cohort) - zebra-rpc -> [7.0.0] (added Security section listing the four RPC advisories fixed in this cycle) - zebra-script -> [6.0.0] - zebra-state -> [5.0.1] (note: internal-only changes, no public-API delta) * docs(changelog): adopt librustzcash changelog style for 4.4.0 entries - Drop the `### Breaking Changes` header in favor of `### Added`/`### Changed`/ `### Removed` (matches `~/zcash/librustzcash/*/CHANGELOG.md` convention). - Group sub-items under module-prefix headers ending with `:`. - zebra-consensus groth16 cleanup is split into a `### Removed` section. - Wrap entries at ~78 chars and use `impl Trait for Type` form for impl items. * docs(changelog): prune trivial detail in 4.4.0 entry - Shorten the consensus-divergence sighash defense-in-depth bullet; the long technical explanation lives in the GHSA, not the operator changelog. - Merge the Sentry SDK upgrade, Sentry CI metadata, OpenTelemetry default- release-binaries inclusion, and `zebrad::sentry` privatization (all from PR ZcashFoundation#10490) into a single observability-refresh bullet. * docs(changelog): pad librustzcash-style subheadings for markdown-lint Add blank lines around the `### Added` / `### Changed` / `### Removed` subheadings introduced in the 4.4.0 entries (MD022/MD032).
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.
Motivation
Closes GHSA-rgwx-8r98-p34c (CWE-770: Allocation of Resources Without Limits or Throttling).
The V4, V5, and V6 transaction deserializers read and allocate Sapling spend vectors
before any coinbase check runs. An attacker can craft a coinbase transaction with a
large (but within
TrustedPreallocatebounds) Sapling spend count, forcing the parserto allocate ~5,000+ spend structs before the consensus layer ever rejects the
transaction.
The advisory's claim that v4.3.1 is not vulnerable is incorrect — the parser code is
byte-for-byte identical between 4.3.0 and 4.3.1. Both versions allocate spend vectors
before any coinbase rule check.
Solution
Read the Sapling spend count as a
CompactSizeMessageand reject coinbase transactionswith
spend_count > 0before allocating the spend vector. The check is added to allthree deserialization paths (V4, V5, V6).
For V5/V6 the existing
ZcashDeserializeimpl forOption<ShieldedData<SharedAnchor>>is extracted into a standalone
deserialize_v5_sapling_shielded_data(reader, is_coinbase)helper so the coinbase flag can be threaded through without changing the trait signature.
The trait impl delegates with
is_coinbase: false, preserving the existing call sites.For V4 the check is inlined at the spend-count read site.
Tests
The commits are structured for reviewer auditability:
77075cfd— reproduction test: constructs a coinbase V5 transaction with Saplingspends and confirms the parser accepts it (proving the vulnerability exists).
5442ea47— fix only: modifies onlyserialize.rs. All 242 existing tests(excluding proptests that generate the now-rejected combination) pass without any
test changes, proving the fix does not alter non-coinbase deserialization behavior.
2ef0e51a— test updates: updatestransaction_roundtrip,block_roundtrip,and the reproduction vector test to assert rejection. The arbitrary generators are
intentionally left unchanged — proptests still generate coinbase+spends and verify
that specific combination is rejected.
Specifications & References
Follow-up Work
TrustedPreallocatebounds for Sapling spends should betightened independently of this fix.
AI Disclosure
PR Checklist