fix(zcash): require an owned singleton input
What the developer wrote
Authored by Adam Tucker
fix(zcash): require an owned singleton input
What changed, and why it matters
This commit changes how the Keystone hardware wallet validates Zcash PCZT (partially-created transaction) files before signing. Previously, under a 'batch' policy it required at least one shielded input belonging to the wallet. After the change, the wallet also accepts a transaction as 'mine' if it contains an owned transparent input, even when there are no signable shielded actions. The title says the fix 'requires an owned singleton input.' The practical effect is to broaden the cases where the wallet will proceed with signing, while still rejecting transactions that have no inputs controlled by the wallet at all. The commit does not describe this as a security fix in its message, but the change is in a security-sensitive signing-check path.
Treat this as a security-relevant correctness fix in the Zcash signing path. Review the full policy around 'singleton' ownership to ensure the new transparent-input exception cannot be combined with foreign shielded inputs to trick the device into co-signing a partially-owned transaction. Verify that check_pczt_transparent correctly identifies inputs truly controlled by the wallet and that has_my_transparent_input cannot be set by outputs or non-owned inputs. Consider requesting a CVE only if further analysis shows an exploitable bypass of the 'no my inputs' guard.
Security signals we found
Change in transaction ownership validation before signing
New condition combines empty shielded actions with absence of transparent inputs to reject PCZT
Test renamed to expect rejection at check phase for foreign seed
New test verifies owned transparent input alone is accepted
Title frames change as a fix: 'require an owned singleton input'
Evidence from the diff
In rust/apps/zcash/src/lib.rs, check_pczt_cypherpunk_with_policy now captures a boolean has_my_transparent_input from check_pczt_transparent and then calls signable_shielded_actions for both Single and Batch policies. It only returns ZcashError::PcztNoMyInputs when signable shielded actions are empty AND there is no owned transparent input. Previously, Single policy skipped the signable-actions check entirely, and Batch policy required at least one shielded action. The commit also adds/adjusts tests: a renamed test now expects the check phase (not only the sign phase) to reject a foreign seed, and a new test asserts that an owned transparent input alone satisfies singleton ownership. Supporting test code in pczt/mod.rs is made available under the cypherpunk feature and conditionally compiles multi_coins-only fields.
Changed components
rust/apps/zcash/src/lib.rsrust/apps/zcash/src/pczt/mod.rsZcash PCZT signing/validation flowShieldedActionPolicy handling (Single/Batch)Inspect captured patch +49 / −26
diff --git a/rust/apps/zcash/src/lib.rs b/rust/apps/zcash/src/lib.rs
index a0a97ff..6f52e67 100644
--- a/rust/apps/zcash/src/lib.rs
+++ b/rust/apps/zcash/src/lib.rs
@@ -145,7 +145,7 @@ fn check_pczt_cypherpunk_with_policy<P: consensus::Parameters>(
"transparent xpub is not present".to_string(),
))?;
pczt::check::check_pczt_orchard(params, seed_fingerprint, account_index, &ufvk, &pczt)?;
- pczt::check::check_pczt_transparent(
+ let has_my_transparent_input = pczt::check::check_pczt_transparent(
params,
seed_fingerprint,
account_index,
@@ -153,18 +153,11 @@ fn check_pczt_cypherpunk_with_policy<P: consensus::Parameters>(
&pczt,
false,
)?;
-
- let pczt = match policy {
- ShieldedActionPolicy::Single => pczt,
- ShieldedActionPolicy::Batch => {
- let (actions, pczt) =
- signable_shielded_actions(params, pczt, seed_fingerprint, account_index, policy)?;
- if actions.is_empty() {
- return Err(ZcashError::PcztNoMyInputs);
- }
- pczt
- }
- };
+ let (signable_actions, pczt) =
+ signable_shielded_actions(params, pczt, seed_fingerprint, account_index, policy)?;
+ if signable_actions.is_empty() && !has_my_transparent_input {
+ return Err(ZcashError::PcztNoMyInputs);
+ }
pczt.serialize()
.map_err(|e| ZcashError::InvalidPczt(alloc::format!("serialize normalized PCZT: {e:?}")))
@@ -2372,8 +2365,20 @@ mod tests {
}
#[test]
- fn test_sign_checked_pczt_rejects_foreign_seed() {
+ fn test_check_and_sign_pczt_reject_foreign_seed() {
let sample = pczt::test_support::sample_orchard_change_pczt();
+ let foreign_seed = [9u8; 32];
+ let foreign_fingerprint = calculate_seed_fingerprint(&foreign_seed).unwrap();
+
+ let check_result = check_pczt_cypherpunk(
+ &pczt::test_support::Nu6_3Network,
+ &sample.bytes,
+ &sample.ufvk_text,
+ &foreign_fingerprint,
+ 0,
+ );
+ assert!(matches!(check_result, Err(ZcashError::PcztNoMyInputs)));
+
let normalized = check_pczt_cypherpunk(
&pczt::test_support::Nu6_3Network,
&sample.bytes,
@@ -2382,8 +2387,6 @@ mod tests {
0,
)
.unwrap();
- let foreign_seed = [9u8; 32];
- let foreign_fingerprint = calculate_seed_fingerprint(&foreign_seed).unwrap();
let result = sign_checked_pczt(
&pczt::test_support::Nu6_3Network,
@@ -2395,6 +2398,21 @@ mod tests {
assert!(matches!(result, Err(ZcashError::PcztNoMyInputs)));
}
+ #[test]
+ fn test_check_pczt_accepts_owned_transparent_input() {
+ let sample = pczt::legacy_test_support::legacy_transparent_sample();
+ let ufvk = derive_ufvk(&MainNetwork, &sample.seed, "m/32'/133'/0'").unwrap();
+
+ check_pczt_cypherpunk(
+ &MainNetwork,
+ &sample.bytes,
+ &ufvk,
+ &sample.seed_fingerprint,
+ 0,
+ )
+ .expect("an owned transparent input satisfies singleton ownership");
+ }
+
#[test]
fn test_sign_checked_batch_pczt_signs_and_rejects_sapling() {
let sample = pczt::test_support::sample_orchard_change_pczt();
diff --git a/rust/apps/zcash/src/pczt/mod.rs b/rust/apps/zcash/src/pczt/mod.rs
index 5e236b2..56b78ee 100644
--- a/rust/apps/zcash/src/pczt/mod.rs
+++ b/rust/apps/zcash/src/pczt/mod.rs
@@ -834,26 +834,25 @@ pub(crate) mod test_support {
}
}
-#[cfg(all(test, feature = "multi_coins", not(feature = "cypherpunk")))]
+#[cfg(all(test, any(feature = "multi_coins", feature = "cypherpunk")))]
pub(crate) mod legacy_test_support {
- use alloc::{
- string::{String, ToString},
- vec,
- vec::Vec,
- };
+ #[cfg(feature = "multi_coins")]
+ use alloc::string::{String, ToString};
+ use alloc::{vec, vec::Vec};
use ::pczt::roles::{creator::Creator, updater::Updater};
use bitcoin::secp256k1::Secp256k1;
- use keystore::algorithms::{
- secp256k1::get_extended_public_key_by_seed, zcash::calculate_seed_fingerprint,
- };
+ #[cfg(feature = "multi_coins")]
+ use keystore::algorithms::secp256k1::get_extended_public_key_by_seed;
+ use keystore::algorithms::zcash::calculate_seed_fingerprint;
use rand_core::OsRng;
use zcash_primitives::transaction::{
builder::{BuildConfig, Builder, PcztResult},
fees::zip317,
};
+ #[cfg(feature = "multi_coins")]
+ use zcash_vendor::pczt::Pczt;
use zcash_vendor::{
- pczt::Pczt,
transparent::{
bundle as transparent,
keys::{AccountPrivKey, IncomingViewingKey},
@@ -869,7 +868,9 @@ pub(crate) mod legacy_test_support {
pub(crate) bytes: Vec<u8>,
pub(crate) seed: Vec<u8>,
pub(crate) seed_fingerprint: [u8; 32],
+ #[cfg(feature = "multi_coins")]
pub(crate) xpub: String,
+ #[cfg(feature = "multi_coins")]
pub(crate) input_pubkey: [u8; 33],
}
@@ -883,6 +884,7 @@ pub(crate) mod legacy_test_support {
]
}
+ #[cfg(feature = "multi_coins")]
pub(crate) fn legacy_transparent_pczt_with_input_derivation(
bytes: &[u8],
seed_fingerprint: [u8; 32],
@@ -978,6 +980,7 @@ pub(crate) mod legacy_test_support {
.unwrap()
.finish();
+ #[cfg(feature = "multi_coins")]
let xpub = get_extended_public_key_by_seed(&seed, &"M/44'/133'/0'".into())
.unwrap()
.to_string();
@@ -986,7 +989,9 @@ pub(crate) mod legacy_test_support {
bytes: pczt.serialize().unwrap(),
seed: seed.to_vec(),
seed_fingerprint,
+ #[cfg(feature = "multi_coins")]
xpub,
+ #[cfg(feature = "multi_coins")]
input_pubkey,
}
}
Why this scored 57/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.