Repository navigation
feat!: fork zcash_script 0.6.0 as zakura-zcash-script - #634
ValarDragon wants to merge 3 commits into
Conversation
Import zcash_script 0.6.0 from ZcashFoundation/zcash_script commit 8d2fc24 as the workspace member zakura-zcash-script, keeping the `zcash_script` library target. The published package's sources are byte-identical to that commit. The import adopts Common's version, edition 2024, and MSRV, applies rustfmt, and gates the test module's signature-validation imports so the tests compile with --no-default-features. It keeps upstream's authors and Apache-2.0 license. zakura-transparent, zakura-primitives, and zakura-keys (dev) now depend on the fork, so the registry zcash_script leaves the graph and their public script types come from zakura-zcash-script. Owning the crate lets Common move its digest dependencies (sha1, sha2, ripemd) to the RustCrypto 0.11 line together with the rest of the workspace. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Gmpc7ZEeScLE6Cb8QMyBEd
Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Gmpc7ZEeScLE6Cb8QMyBEd
The descriptor tests imported `ToString` from `std`, which the `no_std` crate only links with its `std` feature, so CI's transparent-inputs configuration check (descriptor without std) failed to build them. Import it from `alloc`. Record both test-only fixes in the record of fork, and correct the workspace comment: only zcash_transparent exposes the script types publicly. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Gmpc7ZEeScLE6Cb8QMyBEd
|
9 findings ·
|
![v12-auditor[bot]](https://avatars.githubusercontent.com/in/1962297?s=60&v=4){kind=small}
| /// Errors that can happen when creating scripts containing `CHECK*SIG*` operations. | ||
| #[allow(missing_docs)] | ||
| #[derive(Copy, Clone, PartialEq, Eq, Debug, Error)] | ||
| pub enum Error { | ||
| #[error("CHECKMULTISIG only supports 20 keys, but you provided {0}")] | ||
| TooManyPubKeys(usize), | ||
| #[error("PubKeys shouldn’t be longer than 65 bytes")] | ||
| OverlongPubKey, | ||
| } | ||
|
|
||
| /// Generate an [`Opcode`] that would push the provided pubkey onto the stack. | ||
| pub fn push_pubkey(pubkey: &[u8]) -> Result<Opcode, Error> { | ||
| pv::push_value(pubkey) | ||
| .map(PushValue) | ||
| .ok_or(Error::OverlongPubKey) | ||
| } | ||
|
|
||
| /// Performs a `sig_count`-of-`pks.len()` multisig. | ||
| /// | ||
| /// if `verify` | ||
| /// type: `sig_count*Signature -> 💥?` | ||
| /// else | ||
| /// type: `sig_count*Signature -> [Bool]` | ||
| pub fn check_multisig(sig_count: u8, pks: &[&[u8]], verify: bool) -> Result<Vec<Opcode>, Error> { | ||
| Ok([ | ||
| &[PushValue(push_num(sig_count.into()))], | ||
| &pks.iter() | ||
| .map(|pk| push_pubkey(pk)) | ||
| .collect::<Result<Vec<_>, _>>()?[..], | ||
| &[ | ||
| PushValue(push_num( | ||
| pks.len() | ||
| .try_into() | ||
| .map_err(|_| Error::TooManyPubKeys(pks.len()))?, | ||
| )), | ||
| if verify { | ||
| op::CHECKMULTISIGVERIFY | ||
| } else { | ||
| op::CHECKMULTISIG | ||
| }, | ||
| ], | ||
| ] | ||
| .concat()) |
There was a problem hiding this comment.
🔴 Critical · Excess-key multisig scripts are permanently unspendable
pattern::Error states that CHECKMULTISIG supports at most 20 keys, but check_multisig never compares pks.len() with that limit. Its conversion of the length only checks integer representability, so 21 or more keys are serialized successfully and the function returns Ok. At execution, OP_CHECKMULTISIG rejects a decoded key count above MAX_PUBKEY_COUNT == 20 before processing any signatures. The generated locking condition is therefore unsatisfiable regardless of the available private keys.
| /// One party has SHA-256 hashlock, other party can spend unconditionally | ||
| /// | ||
| /// type: `[Signature, PubKey, _, Bool] -> ([Signature, PubKey] + 💥) ∪ [Bool]` | ||
| pub fn sha256_htlc_with_unconditional( | ||
| sender_pk: &[u8], | ||
| recipient_sha: &[u8; 32], | ||
| recipient_pk: &[u8], | ||
| ) -> Vec<Opcode> { | ||
| branch( | ||
| &[ | ||
| &[op::SHA256], | ||
| &equals(push_256b_hash(recipient_sha), true)[..], | ||
| &pay_to_pubkey_hash(recipient_pk)[..], | ||
| ] | ||
| .concat(), | ||
| &[ | ||
| &ignored_value(pv::_1)[..], | ||
| &[op::HASH160], | ||
| &equals( | ||
| push_160b_hash(&Ripemd160::digest(Sha256::digest(sender_pk)).into()), | ||
| true, | ||
| )[..], | ||
| &[op::CHECKSIG], | ||
| ] | ||
| .concat(), | ||
| ) |
There was a problem hiding this comment.
🔴 Critical · Unconditional HTLC branch accepts attacker signatures
sha256_htlc_with_unconditional assembles its non-hashlock branch as HASH160 <HASH160(sender_pk)> EQUALVERIFY CHECKSIG without duplicating or otherwise binding one public-key stack value across both checks. OP_HASH160 consumes one attacker-supplied value, while the later OP_CHECKSIG consumes a separate attacker-supplied public key. The preceding ignored_value(pv::_1) only pushes and drops a constant and does not remove either supplied key. This differs from the correct P2PKH helper, which uses OP_DUP so the hashed key remains for signature verification.
| /// Produces a threshold multisig output script. | ||
| /// | ||
| /// Use [`sortedmulti`] instead if you are constructing a new P2SH address. This | ||
| /// descriptor is provided for cases where you have an existing P2SH address and you | ||
| /// cannot use [`sortedmulti`]. | ||
| /// | ||
| /// Specified in [BIP 383]. | ||
| /// | ||
| /// [BIP 383]: https://github.com/bitcoin/bips/blob/master/bip-0383.mediawiki#sortedmulti | ||
| pub fn multi(k: u8, keys: &[KeyExpression]) -> Result<script::Redeem, Error> { | ||
| multi_inner(k, keys, false) | ||
| } | ||
|
|
||
| /// Produces a threshold multisig output script. | ||
| /// | ||
| /// Specified in [BIP 383]. | ||
| /// | ||
| /// [BIP 383]: https://github.com/bitcoin/bips/blob/master/bip-0383.mediawiki#sortedmulti | ||
| pub fn sortedmulti(k: u8, keys: &[KeyExpression]) -> Result<script::Redeem, Error> { | ||
| multi_inner(k, keys, true) | ||
| } | ||
|
|
||
| fn multi_inner(k: u8, keys: &[KeyExpression], sorted: bool) -> Result<script::Redeem, Error> { | ||
| match u8::try_from(keys.len()) { | ||
| Ok(n @ 1..=20) => { | ||
| if k > n { | ||
| Err(Error::InvalidThreshold(k, n)) | ||
| } else { | ||
| let k = Opcode::from(pattern::push_num(k.into())); | ||
| let n = Opcode::from(pattern::push_num(n.into())); | ||
|
|
||
| let mut keys = keys | ||
| .iter() | ||
| .map(|expr| expr.key.derive_and_serialize()) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| if sorted { | ||
| // Sort lexicographically. | ||
| keys.sort(); | ||
| } | ||
|
|
||
| Ok(script::Component( | ||
| iter::empty() | ||
| .chain(Some(k)) | ||
| .chain( | ||
| keys.into_iter() | ||
| .map(|key| op::push_value(&key).expect("short enough")), | ||
| ) | ||
| .chain([n, op::CHECKMULTISIG]) |
There was a problem hiding this comment.
🔴 Critical · Zero threshold creates signatureless multisig
The public multi and sortedmulti APIs delegate to multi_inner, which rejects only thresholds larger than the key count, so k == 0 is accepted. The public pattern::check_multisig builder likewise serializes sig_count == 0 without a lower-bound check. During OP_CHECKMULTISIG, zero signatures pass the count comparison, skip the verification loop, leave success true, and consume only the legacy dummy argument. The non-VERIFY opcode then pushes true, so the generated locking condition requires no listed private key.
| // The remaining opcodes must be `PushData`s that are valid lengths for pubkeys. | ||
| let pubkeys = pubkeys | ||
| .iter() | ||
| .map(|op| match op { | ||
| Opcode::PushValue(data) => { | ||
| let pubkey_bytes = data.value(); | ||
| // Equivalent to `CPubKey::GetLen` | ||
| let expected_len = match pubkey_bytes.first() { | ||
| Some(2 | 3) => Some(PubKey::COMPRESSED_SIZE), | ||
| Some(4 | 6 | 7) => Some(PubKey::SIZE), | ||
| _ => None, | ||
| }; | ||
| // Equivalent to `CPubKey::ValidSize` | ||
| (expected_len == Some(pubkey_bytes.len())) | ||
| .then(|| pubkey_bytes.try_into().expect("bounds checked")) | ||
| } | ||
| _ => None, | ||
| }) | ||
| .collect::<Option<Vec<_>>>(); | ||
|
|
||
| pubkeys.and_then(|pubkeys| { | ||
| (pubkeys.len() == usize::from(keys) && required <= keys) | ||
| .then_some(ScriptKind::MultiSig { required, pubkeys }) | ||
| }) |
There was a problem hiding this comment.
🟠 High · Uncompressed multisig keys block repository signers
solver::standard accepts 65-byte uncompressed multisig public keys and returns them in ScriptKind::MultiSig; the descriptor module also intentionally constructs and tests multisig scripts containing an uncompressed key. The direct transparent signer and PCZT spend finalizer, however, require each solver-returned key to convert to [u8; 33] and abort on 65-byte entries. Inputs can therefore be accepted and classified as supported multisig but cannot be finalized through the repository's advertised signing flows.
| /// First splits `size_size` bytes to determine the size of the value to read, then splits the | ||
| /// value. | ||
| fn split_tagged_value( | ||
| script: &[u8], | ||
| size_size: usize, | ||
| ) -> (Result<&[u8], opcode::Error>, &[u8]) { | ||
| let (res, rem) = Self::split_value(script, size_size); | ||
| match res { | ||
| Err(_) => (res, rem), | ||
| Ok(bytes) => { | ||
| let mut size = 0; | ||
| for byte in bytes.iter().rev() { | ||
| size <<= 8; | ||
| size |= usize::from(*byte); | ||
| } | ||
| Self::split_value(rem, size) | ||
| } |
There was a problem hiding this comment.
🟠 High · Serialize PUSHDATA lengths using fixed-width unsigned fields
LargeValue serializes OP_PUSHDATA1, OP_PUSHDATA2, and OP_PUSHDATA4 length prefixes with variable-width signed num::serialize output, although the parser consumes fixed-width unsigned little-endian fields selected by the opcode. For example, a 128-byte OP_PUSHDATA1 is emitted with 80 00, while raw parsing treats only 80 as its one-byte length and shifts the remaining bytes across the data/code boundary. This makes authored component evaluation differ from evaluation of the bytes eventually serialized into a transaction, affecting the supported P2SH multisig builder flow that pushes a redeem script. An escaped payload byte can be interpreted as a control opcode such as OP_ELSE; because control opcodes update conditional state even where ordinary operations are skipped, this can suppress a subsequent signature check in a suitably authored locking script. Separately, parsing and reserializing chain components can reconstruct a different script_code for signature hashing, whereas raw Code evaluation retains the original bytes.
| /// Errors that can happen when creating scripts containing `CHECK*SIG*` operations. | ||
| #[allow(missing_docs)] | ||
| #[derive(Copy, Clone, PartialEq, Eq, Debug, Error)] | ||
| pub enum Error { | ||
| #[error("CHECKMULTISIG only supports 20 keys, but you provided {0}")] | ||
| TooManyPubKeys(usize), | ||
| #[error("PubKeys shouldn’t be longer than 65 bytes")] | ||
| OverlongPubKey, | ||
| } | ||
|
|
||
| /// Generate an [`Opcode`] that would push the provided pubkey onto the stack. | ||
| pub fn push_pubkey(pubkey: &[u8]) -> Result<Opcode, Error> { | ||
| pv::push_value(pubkey) | ||
| .map(PushValue) | ||
| .ok_or(Error::OverlongPubKey) | ||
| } | ||
|
|
||
| /// Performs a `sig_count`-of-`pks.len()` multisig. | ||
| /// | ||
| /// if `verify` | ||
| /// type: `sig_count*Signature -> 💥?` | ||
| /// else | ||
| /// type: `sig_count*Signature -> [Bool]` | ||
| pub fn check_multisig(sig_count: u8, pks: &[&[u8]], verify: bool) -> Result<Vec<Opcode>, Error> { | ||
| Ok([ | ||
| &[PushValue(push_num(sig_count.into()))], | ||
| &pks.iter() | ||
| .map(|pk| push_pubkey(pk)) | ||
| .collect::<Result<Vec<_>, _>>()?[..], | ||
| &[ | ||
| PushValue(push_num( | ||
| pks.len() | ||
| .try_into() | ||
| .map_err(|_| Error::TooManyPubKeys(pks.len()))?, | ||
| )), | ||
| if verify { | ||
| op::CHECKMULTISIGVERIFY | ||
| } else { | ||
| op::CHECKMULTISIG | ||
| }, | ||
| ], | ||
| ] | ||
| .concat()) |
There was a problem hiding this comment.
🟡 Medium · Validate public keys before building or classifying signature scripts
The raw-key script builders treat generic pushability as public-key validity: push_pubkey delegates to push_value, whose maximum payload is 520 bytes, despite exposing OverlongPubKey for keys above 65 bytes. Consequently, check_sig, pay_to_pubkey, and check_multisig can embed overlong keys as well as malformed 33- or 65-byte values in signature-locking scripts. The P2PK solver likewise classifies a pushed 33- or 65-byte value followed by CHECKSIG as ScriptKind::PubKey based solely on length. At execution, StrictEnc rejects keys that lack an allowed prefix and exact length, while the verifier returns false if secp256k1::PublicKey::from_slice cannot parse the supplied bytes. A shared public-key validator should enforce permitted encodings and successful secp256k1 parsing in raw-key builders and in P2PK classification, rather than relying on generic push limits or nominal length.
| /// Produces a minimally-encoded data value. It fails if the slice is larger than | ||
| /// `LargeValue::MAX_SIZE`. | ||
| pub fn push_value(value: &[u8]) -> Option<PushValue> { | ||
| PushValue::from_slice(value) | ||
| } |
There was a problem hiding this comment.
🟡 Medium · Rejected pushes copy oversized input four times
The public pv::push_value(&[u8]) API promises to reject values above 520 bytes, but LargeValue::from_slice performs no initial length check. Instead, it independently calls v.to_vec() for four bounded-vector conversions before returning None. For any oversized attacker-controlled slice, every conversion must fail by its upper bound, yet the full input is allocated and copied repeatedly.
| /// Produce a minimal `PushValue` that encodes the provided script. This is particularly useful with | ||
| /// P2SH. | ||
| pub fn push_script<T: Into<opcode::PossiblyBad> + opcode::Evaluable + Clone>( | ||
| script: &script::Component<T>, | ||
| ) -> Option<opcode::PushValue> { | ||
| pv::push_value(&script.to_bytes()) | ||
| } |
There was a problem hiding this comment.
🟡 Medium · P2SH constructors accept unrevealable oversized redeem scripts
The P2SH construction APIs commit to arbitrary serialized redeem scripts without enforcing that the script can be revealed as one pushable stack element. pay_to_script_hash and descriptor sh hash their inputs directly, while push_script returns None once the serialized input exceeds the 520-byte PushValue limit. This also affects descriptor-generated multisig scripts: multi_inner and sortedmulti accept up to 20 keys and emit each serialized key, but do not bound the resulting redeem-script size for P2SH use. During P2SH evaluation, the final stack item is selected as the complete redeem script, and parsing a push larger than 520 bytes produces PushSize. The same size validation must therefore be applied before a P2SH locking script is constructed, including along descriptor paths that produce scripts intended for sh.
Motivation
zcash_script0.6.0 is the latest release, from ZcashFoundation/zcash_script. It still requiressha10.10,sha20.10 andripemd0.1, and those keep the wholedigest0.10 RustCrypto family (block-buffer,crypto-common,cpufeatures0.2,generic-array) in Zakura's build, beside the 0.11 family the newer crates use.Measured on Zakura, upgrading Zakura's own
sha2,ripemd,curve25519-dalekanded25519-zebrato their latest versions adds 3–5 crates whilezcash_scriptpins the old line. Owningzcash_scriptlets Common move its digests to 0.11 together withzakura-transparent, whose public API exposes its script types.Solution
Fork. Import
zcash_script0.6.0 as the workspace memberzakura-zcash-script(crates/zcash_script, library targetzcash_script). It uses the reserved crates.io name.Source: upstream commit
8d2fc24. The published 0.6.0 package's sources are byte-identical to it. The package SHA-25610106859…41275is the checksum the registry entry inCargo.lockhad.Changes from upstream:
rustfmtfor edition 2024, which only reorders imports and rewraps lines.lib.rs: gate the test module's signature-validation-only imports onsignature-validation. Upstream fails--all-targets --no-default-featureswithE0432.descriptor.rs: importToStringfromallocinstead ofstd. Upstream failsdescriptorwithoutstd, which CI'stransparent-inputsconfiguration check builds.cfg(linux)libfuzzer-sysdev-dependency, which only its separate fuzz workspace used.After formatting upstream the same way, those two test fixes are the only source diff.
License: upstream's authors and its Apache-2.0-only license are kept. The manifest sets
license = "Apache-2.0"instead of the workspace'sMIT OR Apache-2.0, and the crate shipsLICENSE-APACHEonly.Changelog:
CHANGELOG.mdgets an empty[Unreleased]and a Record of Fork, as in feat(common): own encoding and note encryption #540.Consumers.
zakura-transparent,zakura-primitives, andzakura-keys(dev) now use the workspace fork, so the registryzcash_scriptleavesCargo.lock. This is breaking forzakura-transparent: its public script types (address::Script,TransparentAddress::script, …) now come fromzakura-zcash-script, so they no longer interoperate with upstreamzcash_script0.6 types.zakura-primitivesonly uses the crate internally. Zakura must switch tozakura-zcash-scriptwhen it moves to Common 5.Repository wiring:
[workspace.dependencies]entry, and the scope comment inCargo.toml.supported_features.pyarguments inci.ymlandmaintenance.yml. The crate is#[no_std], so it fits thethumbv7emlane.policy.zakura-zcash-scriptentry, and thezcash_script0.6.0 exemption removed.docs/crates-io-releases.md. Those counts were already stale: main has 32 members and 24 published crates. With this PR it's 33 and 25.Testing
The librustzcash group's CI commands, run locally with the fork included:
cargo testwith every supported feature passes all 268 tests across the 8 crates.--all-targets, the only findings are 2collapsible_ifwarnings in upstream code. This lane doesn't deny warnings, and I left upstream code unchanged.cargo build -p zakura-zcash-script --no-default-features --target thumbv7em-none-eabihfbuilds.The fork on its own:
cargo test -p zakura-zcash-script: 5 pass with default features, 8 with--all-features.-W rust-2024-compatibility, which flags code whose meaning changes under 2024. It produced zero warnings, so the move to edition 2024 changes no behavior.Repository checks:
cargo fmt --all -- --checkpasses, and./scripts/changelog.py checkvalidates the fragment.cargo deny --locked --workspace check bans licenses sourcespasses. Apache-2.0 is already allowed.cargo vet checkreports only the error main already has (zakura-note-encryption5.0.0 andzakura-orchard-legacy5.0.0 match published versions without anaudit-as-crates-iopolicy). The new member has its policy.actionlintshows the same 7 pre-existing shellcheck infos as main. markdownlint passes on the changed docs.Follow-up
zakura-zcash-scripttosha10.11,sha20.11, andripemd0.2, withzakura-transparent,zakura-primitives, andzakura-address. Base58Check would then compute its checksum withsha20.11 and usebs58withoutcheck.zakura-zcash-script. It is currently a 0.0.0 reservation from zakura-core/reserved.zcash_script = "0.6"withzakura-zcash-scriptwhen it adopts Common 5.🤖 Generated with Claude Code
https://claude.ai/code/session_01Gmpc7ZEeScLE6Cb8QMyBEd