What the developer wrote
Authored by soralit
fix: zcash checks
What changed, and why it matters
This commit fixes a missing security check in the Zcash PCZT (Partially Created Zcash Transaction) verifier for the Keystone 3 hardware wallet. Previously, when a transaction output could be decrypted with an internal outgoing viewing key (a key meant for change/internal addresses), the code did not verify that the output actually belonged to the wallet. This could allow a maliciously crafted transaction to trick the wallet into accepting an external output as if it were internal/change. The patch now checks that any output recoverable with an internal OVK is actually a wallet-owned Orchard address, and also considers the transparent internal OVK. The commit message is just 'fix: zcash checks' and does not disclose this as a security issue.
Review whether this fix addresses a complete attack path or if additional output ownership checks are needed for Sapling and transparent components. Consider whether the change warrants a security advisory or CVE given the potential for a malicious PCZT to mislead the wallet about output ownership. Users should update to a firmware release containing this commit once available.
Security signals we found
Missing output ownership validation for internal OVK decryption
Potential acceptance of non-wallet outputs as internal/change
Addition of wallet-address ownership check after internal-OVK output recovery
Inclusion of transparent internal OVK in decryption attempts
Test case added to verify rejection of non-wallet internal-OVK outputs
Evidence from the diff
In rust/apps/zcash/src/pczt/check.rs, the check_action_output function previously only verified the Orchard note commitment (cmx) and had a TODO noting that output decryption/ownership checks were implicit elsewhere. The patch adds explicit validation: it attempts to decrypt the output encryption ciphertext using external OVK, internal OVK, and transparent internal OVK. If decryption succeeds with an internal OVK, it verifies the resulting address is a wallet-owned Orchard address (either external or internal scope) via is_wallet_orchard_address. If not, it returns InvalidPczt with the message ‘Orchard output was recoverable with an internal OVK but does not belong to this wallet’. The test in lib.rs is updated to assert this failure case. The change is gated behind the ‘cypherpunk’ feature and affects Orchard bundle verification only.
Changed components
rust/apps/zcash/src/pczt/check.rsrust/apps/zcash/src/lib.rsZcash PCZT verifier (cypherpunk feature)Orchard action output validationInspect captured patch +59 / −17
diff --git a/rust/apps/zcash/src/lib.rs b/rust/apps/zcash/src/lib.rs
index 6fefeb0..7dacd23 100644
--- a/rust/apps/zcash/src/lib.rs
+++ b/rust/apps/zcash/src/lib.rs
@@ -392,7 +392,6 @@ mod tests {
.unwrap();
let pczt = Creator::build_from_parts(pczt_parts).unwrap();
let pczt_bytes = pczt.serialize();
-
let seed_fingerprint = calculate_seed_fingerprint(&victim_seed).unwrap();
let result =
@@ -407,6 +406,16 @@ mod tests {
panic!("unexpected success: orchard={orchard:?}");
}
}
+
+ let check_result =
+ check_pczt_cypherpunk(¶ms, &pczt_bytes, &ufvk_text, &seed_fingerprint, 0);
+ match check_result {
+ Err(ZcashError::InvalidPczt(_)) => {}
+ Err(ZcashError::InvalidDataError(msg))
+ if msg.contains("Orchard output was recoverable with an internal OVK but does not belong to this wallet") => {}
+ Err(e) => panic!("unexpected check error: {e:?}"),
+ Ok(()) => panic!("unexpected check success"),
+ }
}
#[test]
diff --git a/rust/apps/zcash/src/pczt/check.rs b/rust/apps/zcash/src/pczt/check.rs
index 791a070..bdde3f5 100644
--- a/rust/apps/zcash/src/pczt/check.rs
+++ b/rust/apps/zcash/src/pczt/check.rs
@@ -1,11 +1,11 @@
// checking logic for PCZT
-use alloc::string::ToString;
+use alloc::{string::ToString, vec};
use super::*;
#[cfg(feature = "cypherpunk")]
-use orchard::{keys::FullViewingKey, value::ValueSum};
+use orchard::{keys::FullViewingKey, value::ValueSum, Address};
use zcash_vendor::{
pczt::{self, roles::verifier::Verifier, Pczt},
@@ -50,13 +50,9 @@ pub fn check_pczt_orchard<P: consensus::Parameters>(
pczt: &Pczt,
) -> Result<(), ZcashError> {
validate_sapling_bundle_consistency(pczt)?;
- // checking orchard keys.
- let orchard = ufvk.orchard().ok_or(ZcashError::InvalidDataError(
- "orchard fvk is not present".to_string(),
- ))?;
Verifier::new(pczt.clone())
.with_orchard(|bundle| {
- check_orchard(params, seed_fingerprint, account_index, orchard, bundle)
+ check_orchard(params, seed_fingerprint, account_index, ufvk, bundle)
.map_err(pczt::roles::verifier::OrchardError::Custom)
})
.map_err(|e| ZcashError::InvalidDataError(alloc::format!("{e:?}")))?;
@@ -306,11 +302,11 @@ fn check_orchard<P: consensus::Parameters>(
params: &P,
seed_fingerprint: &[u8; 32],
account_index: zip32::AccountId,
- fvk: &FullViewingKey,
+ ufvk: &UnifiedFullViewingKey,
bundle: &orchard::pczt::Bundle,
) -> Result<(), ZcashError> {
bundle.actions().iter().try_for_each(|action| {
- check_action(params, seed_fingerprint, account_index, fvk, action)?;
+ check_action(params, seed_fingerprint, account_index, ufvk, action)?;
Ok::<_, ZcashError>(())
})?;
@@ -338,7 +334,7 @@ fn check_action<P: consensus::Parameters>(
params: &P,
seed_fingerprint: &[u8; 32],
account_index: zip32::AccountId,
- fvk: &FullViewingKey,
+ ufvk: &UnifiedFullViewingKey,
action: &orchard::pczt::Action,
) -> Result<(), ZcashError> {
// Check `cv_net` first so we know that the `value` fields for both the spend and the
@@ -347,8 +343,11 @@ fn check_action<P: consensus::Parameters>(
ZcashError::InvalidPczt(alloc::format!("invalid cv_net in Orchard action: {e:?}"))
})?;
+ let fvk = ufvk.orchard().ok_or(ZcashError::InvalidDataError(
+ "orchard fvk is not present".to_string(),
+ ))?;
check_action_spend(params, seed_fingerprint, account_index, fvk, action.spend())?;
- check_action_output(action)
+ check_action_output(ufvk, action)
}
#[cfg(feature = "cypherpunk")]
@@ -394,8 +393,20 @@ fn check_action_spend<P: consensus::Parameters>(
}
#[cfg(feature = "cypherpunk")]
-//check output cmx
-fn check_action_output(action: &orchard::pczt::Action) -> Result<(), ZcashError> {
+fn is_wallet_orchard_address(fvk: &FullViewingKey, address: &Address) -> bool {
+ let external_ivk = fvk.to_ivk(zcash_vendor::zip32::Scope::External);
+ let internal_ivk = fvk.to_ivk(zcash_vendor::zip32::Scope::Internal);
+
+ external_ivk.diversifier_index(address).is_some()
+ || internal_ivk.diversifier_index(address).is_some()
+}
+
+#[cfg(feature = "cypherpunk")]
+// check output cmx and internal-ovk output ownership constraints
+fn check_action_output(
+ ufvk: &UnifiedFullViewingKey,
+ action: &orchard::pczt::Action,
+) -> Result<(), ZcashError> {
action
.output()
.verify_note_commitment(action.spend())
@@ -403,9 +414,31 @@ fn check_action_output(action: &orchard::pczt::Action) -> Result<(), ZcashError>
ZcashError::InvalidPczt(alloc::format!("invalid Orchard action cmx: {e:?}"))
})?;
- // TODO: Currently the "can decrypt output" check is performed implicitly by
- // `parse_orchard_output`. If desired, that code could be called from here and
- // checked; then in `parse_orchard_output` it would never error if reached.
+ let fvk = ufvk.orchard().ok_or(ZcashError::InvalidDataError(
+ "orchard fvk is not present".to_string(),
+ ))?;
+ let external_ovk = fvk.to_ovk(zcash_vendor::zip32::Scope::External).clone();
+ let internal_ovk = fvk.to_ovk(zcash_vendor::zip32::Scope::Internal).clone();
+ let transparent_internal_ovk = ufvk
+ .transparent()
+ .map(|k| orchard::keys::OutgoingViewingKey::from(k.internal_ovk().as_bytes()));
+
+ let mut keys = vec![(Some(external_ovk), false), (Some(internal_ovk), true)];
+ if let Some(ovk) = transparent_internal_ovk {
+ keys.push((Some(ovk), true));
+ }
+
+ for (vk, is_internal_ovk) in keys {
+ if let Some((_, address, _)) = super::parse::decode_output_enc_ciphertext(action, vk.as_ref())?
+ {
+ if is_internal_ovk && !is_wallet_orchard_address(fvk, &address) {
+ return Err(ZcashError::InvalidPczt(
+ "Orchard output was recoverable with an internal OVK but does not belong to this wallet".into(),
+ ));
+ }
+ break;
+ }
+ }
Ok(())
}
Why this scored 67/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.