fix(zcash): validate transparent PCZT formats
What the developer wrote
Authored by Adam Tucker
fix(zcash): validate transparent PCZT formats
What changed, and why it matters
This commit tightens validation for Zcash transaction formats (PCZT) on the Keystone 3 hardware wallet. Previously, the firmware only checked that a transaction was version 6 or higher and not v6 specifically, plus that it had no Sapling or Orchard shielded components. The new code also verifies the exact transaction version, version group ID, and the consensus branch's Orchard protocol revision. A new test confirms that a v6 transaction on the Nu6 branch is rejected for transparent-only operations. The change appears to prevent unsupported or ambiguous transaction formats from being signed or parsed, which could reduce the risk of signing a transaction the wallet does not fully understand.
Review whether any additional consensus branch IDs or version group IDs should be included in the allow-list, and confirm that the new rejection paths are reachable from all user-facing signing flows. Consider adding tests for version/group ID mismatches and for v5 on non-Orchard branches.
Security signals we found
Input-validation hardening for transaction parsing/signing
Explicit allow-listing of supported (tx_version, version_group_id, orchard_revision) tuples
New negative test for v6 + Nu6 branch rejection
Potential prior under-validation of transparent-only PCZT formats
Evidence from the diff
The patch modifies rust/apps/zcash/src/pczt/mod.rs so that pczt_is_unsupported_by_transparent_only() no longer relies solely on tx_version >= 6 && !pczt_is_v6(). Instead it explicitly matches the tuple (tx_version, version_group_id, orchard_revision) against known-good constants: (V5_TX_VERSION, V5_VERSION_GROUP_ID, Some(_)) and (V6_TX_VERSION, V6_VERSION_GROUP_ID, Some(OrchardProtocolRevision::V3)). Anything else is rejected. The test file adds a helper to mutate a sample PCZT’s consensus_branch_id to BranchId::Nu6 and asserts that check_pczt_multi_coins, parse_pczt_multi_coins, and sign_pczt all reject it with transparent-only error messages. This implies the prior check could have accepted v6 transactions on branches other than the expected Orchard v3 revision, or with mismatched version/group IDs.
Changed components
rust/apps/zcash/src/pczt/mod.rsrust/apps/zcash/src/lib.rsZcash PCZT signing, checking, and parsing functionsInspect captured patch +80 / −2
diff --git a/rust/apps/zcash/src/lib.rs b/rust/apps/zcash/src/lib.rs
index 56010dd..ac02316 100644
--- a/rust/apps/zcash/src/lib.rs
+++ b/rust/apps/zcash/src/lib.rs
@@ -718,8 +718,39 @@ pub fn sign_pczt(pczt: &[u8], seed: &[u8]) -> Result<Vec<u8>> {
#[cfg(all(test, feature = "multi_coins", not(feature = "cypherpunk")))]
mod legacy_tests {
+ use alloc::{collections::BTreeMap, string::String, vec::Vec};
+
use super::*;
- use zcash_vendor::zcash_protocol::consensus::MainNetwork;
+ use serde::{Deserialize, Serialize};
+ use zcash_vendor::zcash_protocol::consensus::{BranchId, MainNetwork};
+
+ #[derive(Serialize, Deserialize)]
+ struct PcztGlobalPrefix {
+ global: GlobalMirror,
+ }
+
+ #[derive(Serialize, Deserialize)]
+ struct GlobalMirror {
+ tx_version: u32,
+ version_group_id: u32,
+ consensus_branch_id: u32,
+ fallback_lock_time: Option<u32>,
+ expiry_height: u32,
+ coin_type: u32,
+ tx_modifiable: u8,
+ proprietary: BTreeMap<String, Vec<u8>>,
+ }
+
+ fn pczt_with_consensus_branch_id(bytes: &[u8], branch_id: BranchId) -> Vec<u8> {
+ let (mut prefix, rest) =
+ postcard::take_from_bytes::<PcztGlobalPrefix>(&bytes[8..]).unwrap();
+ prefix.global.consensus_branch_id = branch_id.into();
+
+ let mut encoded = bytes[..8].to_vec();
+ encoded = postcard::to_extend(&prefix, encoded).unwrap();
+ encoded.extend_from_slice(rest);
+ encoded
+ }
fn assert_invalid_pczt_message<T: core::fmt::Debug>(result: Result<T>, expected: &str) {
match result {
@@ -798,6 +829,35 @@ mod legacy_tests {
)
.expect("transparent-only v6 PCZT should pass checking");
}
+
+ #[test]
+ fn legacy_rejects_v6_before_nu6_3() {
+ let sample = pczt::legacy_test_support::legacy_transparent_v6_sample();
+ let bytes = pczt_with_consensus_branch_id(&sample.bytes, BranchId::Nu6);
+ let pczt = Pczt::parse(&bytes).unwrap();
+ assert!(pczt::pczt_is_v6(&pczt));
+
+ assert_invalid_pczt_message(
+ check_pczt_multi_coins(
+ &MainNetwork,
+ &bytes,
+ &sample.xpub,
+ &sample.seed_fingerprint,
+ 0,
+ ),
+ "PCZT is not supported by transparent-only checking",
+ );
+ assert_invalid_pczt_message(
+ parse_pczt_multi_coins(&MainNetwork, &bytes, &sample.seed_fingerprint),
+ "PCZT is not supported by transparent-only parsing",
+ );
+ assert_eq!(
+ sign_pczt(&bytes, &sample.seed),
+ Err(ZcashError::SigningError(
+ "PCZT is not supported by transparent-only signing".to_string()
+ ))
+ );
+ }
}
#[cfg(feature = "cypherpunk")]
diff --git a/rust/apps/zcash/src/pczt/mod.rs b/rust/apps/zcash/src/pczt/mod.rs
index 014ee3c..6a1db8f 100644
--- a/rust/apps/zcash/src/pczt/mod.rs
+++ b/rust/apps/zcash/src/pczt/mod.rs
@@ -240,7 +240,25 @@ pub(crate) fn matching_seed_supported_orchard_account_parts(
/// v6 is supported by the shared ZIP 244 sighash implementation.
#[cfg(any(feature = "multi_coins", not(feature = "cypherpunk")))]
pub(crate) fn pczt_is_unsupported_by_transparent_only(pczt: &zcash_vendor::pczt::Pczt) -> bool {
- (*pczt.global().tx_version() >= 6 && !pczt_is_v6(pczt))
+ use zcash_vendor::zcash_protocol::{
+ consensus::{BranchId, OrchardProtocolRevision},
+ constants::{V5_TX_VERSION, V5_VERSION_GROUP_ID, V6_TX_VERSION, V6_VERSION_GROUP_ID},
+ };
+
+ let orchard_revision = BranchId::try_from(*pczt.global().consensus_branch_id())
+ .ok()
+ .and_then(|branch_id| branch_id.orchard_protocol_revision());
+ let supported_transaction_format = match (
+ *pczt.global().tx_version(),
+ *pczt.global().version_group_id(),
+ orchard_revision,
+ ) {
+ (V5_TX_VERSION, V5_VERSION_GROUP_ID, Some(_)) => true,
+ (V6_TX_VERSION, V6_VERSION_GROUP_ID, Some(OrchardProtocolRevision::V3)) => true,
+ _ => false,
+ };
+
+ !supported_transaction_format
|| !pczt.sapling().spends().is_empty()
|| !pczt.sapling().outputs().is_empty()
|| !pczt.orchard().actions().is_empty()
Why this scored 60/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.