Skip to content

security: apply 2026-06 source-review fixes (consensus, memory, crypto, FFI, P2P, wallet) - #3

Merged
VictorLux merged 1 commit into
masterfrom
fix/security-review-2026-06
Jun 12, 2026
Merged

security: apply 2026-06 source-review fixes (consensus, memory, crypto, FFI, P2P, wallet)#3
VictorLux merged 1 commit into
masterfrom
fix/security-review-2026-06

Conversation

@VictorLux

Copy link
Copy Markdown
Owner

Summary

Applies the fixes from a full multi-domain security review of the ZClassic source (crypto, C/C++ memory safety, Rust/FFI, consensus, P2P/DoS, wallet/RPC, performance/stability, supply chain). Each fix is traced to verified source lines and was functionally exercised on a mainnet node at tip ~3.14M.

18 files changed, +297 / −65. This is not a consensus change versus the network — every fix is either non-consensus, node-local, or (for BLK-01) brings an import code path in line with the block-size rule CheckBlock already enforces. No hard fork; nothing here changes block/transaction acceptance for peers.

Fixes

Consensus / validation

ID File Change
BLK-01 consensus/consensus.h, main.cpp Shared GENEROUS_BLOCK_SIZE_LIMIT; widen LoadExternalBlockFile buffer and the nSize gate so -reindex / -loadblock import the 1,272 canonical mainnet blocks between 200 KB and 2 MB that the old MAX_BLOCK_SIZE cap silently skipped (which would otherwise produce a divergent/incomplete chainstate on rebuild).
CON-01 main.cpp LoadBlockIndexDB value-pool rebuild now uses overflow-safe CheckedAdd (was raw signed + on CAmount — undefined behaviour on overflow, feeding the ZIP-209 turnstile).
CON-05 main.cpp Drop the no-op vtx.size() > GENEROUS_BLOCK_SIZE_LIMIT conjunct (tx count vs a byte constant) in CheckBlock; the serialized-size check is the real guard.

Memory safety

ID File Change
MEM-01 streams.h CBaseDataStream::read/ignore compute bounds in size_t with a pre-addition overflow guard (was unsigned int narrowing of a size_t length → latent OOB memcpy).
MEM-02 serialize.h ReadVarInt guards against overflow of I before the shift and before n++ (upstream Bitcoin Core form) — prevents value wrap / signed UB (VARINT is used on signed fields like nFile/nPos) and unbounded spin.
MEM-03 bootstrap.cpp Bound the addr element count before allocating the vector in bootstrap discovery (was deserialize-then-check).

Cryptography

ID File Change
CRY-01 zcash/NoteEncryption.cpp Zeroize the symmetric key K and dhsecret via a RAII MemoryCleanser declared right after each buffer — cleared on every scope exit (normal/early return, AEAD-failure, and the KDF/DH-failure throw).
CRY-03 zcash/Proof.cpp Explicit G1 prime-order subgroup check in to_libsnark_g1(), mirroring the existing G2 check (defense-in-depth; rejects nothing today as G1 cofactor = 1).

Rust / FFI

ID File Change
RUST-01 transaction_builder.cpp Check the bool returns of librustzcash_sapling_spend_sig / _binding_sig; free the proving ctx and return an error instead of building a silently zero-signed transaction.
RUST-02 init.cpp ferror guard in the check_file_hash read loop — no infinite 100%-CPU spin on a real I/O error during startup.

P2P / performance / stability

ID File Change
NET-01 main.cpp uintptr_t (not unsigned int) for the addr-relay pointer hash so the full 64-bit pointer mixes in.
NET-02 net.cpp Prune expired setBanned entries on each Ban() (bounds map growth / the per-connection linear scan).
PERF-01 wallet/wallet.cpp Drop the redundant LOCK(mempool.cs) in ReacceptWalletTransactions (ATMP takes it internally) — removes the flagged cs_wallet → mempool.cs lock-order edge.
PERF-03 net.cpp Cap mapRelay growth (evict oldest) so a unique-txid flood can't grow it unbounded before the 15-min timer.
PERF-04 bootstrapvalidation.cpp Reduce the background validator's cs_main batch budget 80 → 20 ms (shorter stalls to live message processing).

Wallet / RPC

ID File Change
WAL-01 wallet/rpcwallet.cpp z_sendmany help text warns that -debug=zrpcunsafe / =all writes shielded sender/recipients/amounts/memo to debug.log.
WAL-02 httpserver.cpp Warn when -rpcallowip is set without -rpcbind (RPC then binds all interfaces).
WAL-03 wallet/wallet.cpp Raise the wallet KDF iteration floor 25,000 → 100,000 (encrypt + change-passphrase).
WAL-05 wallet/rpcdump.cpp dumpwallet sets the export file to 0600 immediately (it contains all private keys + the HD seed).
WAL-06 rpc/server.h, wallet/rpcwallet.cpp, rpc/misc.cpp nWalletUnlockTimestd::atomic<int64_t>; reads use .load() (no torn read vs the relock timer). Writes still under the lock.

CI

ID File Change
SUP-04 qa/pull-tester/rpc-tests.sh Re-enable the shielded regression tests (incl. zcjoinsplitdoublespend.py, shieldcoinbase, nullifiers, treestate, anchorfork, zkey import/export, NU-activation).

Documentation only (no behaviour change)

  • CON-02/03 — documented the ZIP-209 invariant: on a per-block overflow nChainSaplingValue becomes none and propagates, so the turnstile is skipped (not enforced) across the unknown window — corrupted data can never cause a wrong turnstile pass, but it is also not enforced there (acceptable for corruption recovery).
  • CON-04 — restoring the tight 102 KB post-Sapling tx-size limit for new blocks is a soft fork and must be staged at a fixed future activation height in ContextualCheckTransaction; left unchanged.

Out of scope / deferred (tracked, not in this PR)

  • CON-06 — Buttercup "triple-halving" emission: confirm-intent only.
  • CON-07 — Bubbly and Buttercup share consensus branch ID 0x930b540d (upgrades.cpp:38,43). Immutable (active since height 707000; baked into the sighashes of all v4 transactions since) — must not be changed retroactively; the next network upgrade must use a fresh, unique branch ID.
  • SUP-01/02/03 — dependency bumps (OpenSSL 1.1.1w EOL, Rust 1.70/rand 0.4.2, libevent/leveldb/proton): require maintainer re-pinning of verified sha256 hashes + cargo audit.
  • PERF-02 (-maxmempool eviction), WAL-04 (passphrase mlock), SUP-05 (orphaned crate_*.mk).

Verification

  • Builds and runs: rebuilt and restarted; clean startup, full block-index load, block verify with no inconsistencies, wallet load, sync to tip.
  • MEM-02 / CON-01 exercised: VARINT decoding ran across all ~3.14M blocks during index load with no errors; index value-pool rebuild clean (no turnstile/overflow lines).
  • CRY-01 + RUST-01 exercised end-to-end: a shielded z_sendmany round-trip succeeded — note encrypted, spend + binding signatures created (the RUST-01 error branch did not fire), broadcast, mined, and decrypted on receipt at the destination. Sapling note decryption (balance read) also confirmed.
  • BLK-01 confirmed by a full -reindex on a datadir copy: the block at height 6587 (hash 000000077ff04de591789d4e11b26e4387e14753cc14a997168acf6cea788f70, 226 001 bytes — the first block over the old 200 000-byte MAX_BLOCK_SIZE) was read and connected, and the chain advanced past it (6588, 6589, …). The unpatched LoadExternalBlockFile would have skipped it and stalled at 6586. Hash matches the audit-full.json oversized-block record.
  • git diff --check is clean.

Reviewer checklist

  • Build with the depends tree.
  • Run qa/pull-tester/rpc-tests.sh — the re-enabled shielded tests are not yet run in CI here; triage any failure rather than re-commenting it.
  • -reindex on a copy of a full-history datadir to confirm BLK-01 imports the large historical blocks — done (height 6587 / 226 KB connected, hash-matched; see Verification). Reviewers may re-run to a full matching tip if desired.
  • Spot-check the consensus diff in main.cpp (LoadBlockIndexDB, CheckBlock).

Notes

  • No new dependencies. No public API/RPC removed (only a warning + help-text addition).
  • The shielded-test re-enable (SUP-04) may surface pre-existing failures; those should be triaged, not hidden.

🤖 Generated with Claude Code

…o, FFI, P2P, wallet)

From the full multi-domain security review; each fix traced to verified source
lines and functionally exercised on mainnet at tip ~3.14M. Not a consensus
change vs the network.

Verification:
- Full block-index load (MEM-02 VARINT path) and clean block verify across
  ~3.14M blocks (CON-01 value-pool rebuild) with no errors.
- Shielded z_sendmany round-trip succeeded: note encrypted (CRY-01), spend +
  binding signatures created (RUST-01 error branch did not fire), broadcast,
  mined, and decrypted on receipt.
- BLK-01 confirmed by a full -reindex on a datadir copy: block at height 6587
  (hash 000000077ff04de591789d4e11b26e4387e14753cc14a997168acf6cea788f70,
  226001 bytes -- the first block over the old 200000-byte MAX_BLOCK_SIZE)
  was read and connected; the chain advanced past it instead of stalling at
  6586 as the unpatched LoadExternalBlockFile would have.

Consensus / validation:
- CON-01  LoadBlockIndexDB value-pool rebuild uses overflow-safe CheckedAdd
          (was raw signed + on CAmount, undefined behaviour on overflow)
- CON-05  drop the no-op tx-count-vs-byte-constant conjunct in CheckBlock
- BLK-01  shared GENEROUS_BLOCK_SIZE_LIMIT; widen LoadExternalBlockFile buffer
          and nSize gate so -reindex/-loadblock import the 1,272 canonical
          mainnet blocks between 200 KB and 2 MB (were silently skipped)

Memory safety:
- MEM-01  size_t bounds + pre-add overflow guard in CBaseDataStream read/ignore
- MEM-02  overflow guard in ReadVarInt before the shift and the increment
          (upstream form; VARINT is used on signed fields too)
- MEM-03  bound the addr count before vector allocation in bootstrap discovery

Cryptography:
- CRY-01  RAII zeroization of symmetric key K and dhsecret in NoteEncryption
          (cleared on every scope exit, incl. KDF/DH-failure throw)
- CRY-03  explicit G1 prime-order subgroup check (mirrors the G2 check)

Rust / FFI:
- RUST-01 check librustzcash_sapling_spend_sig / _binding_sig returns; return an
          error instead of building a silently zero-signed transaction
- RUST-02 ferror guard in check_file_hash read loop (no infinite spin on I/O error)

P2P / performance / stability:
- NET-01  uintptr_t for the addr-relay pointer hash (full 64-bit mixes in)
- NET-02  prune expired setBanned entries on Ban()
- PERF-01 drop redundant LOCK(mempool.cs) in ReacceptWalletTransactions
- PERF-03 cap mapRelay growth (evict oldest)
- PERF-04 reduce bootstrap-validator cs_main batch budget 80 -> 20 ms

Wallet / RPC:
- WAL-01  z_sendmany help warns that -debug=zrpcunsafe leaks shielded details
- WAL-02  warn when -rpcallowip is set without -rpcbind (binds all interfaces)
- WAL-03  raise wallet KDF iteration floor 25000 -> 100000
- WAL-05  dumpwallet writes the export file 0600 (contains keys + HD seed)
- WAL-06  nWalletUnlockTime -> std::atomic (no torn read vs the relock timer)

CI:
- SUP-04  re-enable the shielded regression tests

Documentation only (no behaviour change): CON-02/03 (turnstile check-skipped
invariant on unknown chain value), CON-04 (restoring the tight post-Sapling tx
size limit is a future soft fork, must be staged at an activation height).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@VictorLux
VictorLux merged commit bb02cac into master Jun 12, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant