← Watch feed
refactor(zcash): drop ufvk from sign FFI and retire byte-level postflights
Public commit record
62/100 · Adequate
What the developer wrote
Authored by Adam Tucker
refactor(zcash): drop ufvk from sign FFI and retire byte-level postflights
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Uses a recognizable type or scope! No meaningful explanatory body
What changed, and why it matters
This commit refactors how Keystone's Zcash signing code handles shielded transactions. It removes several old byte-level 'preflight' and 'postflight' checks and no longer passes a full viewing key (ufvk) into the final signing functions. The change appears to be a cleanup that moves verification earlier in the workflow, but the commit message and diff alone do not clearly state whether any security bug is being fixed. Without external references, it is hard to tell if this is a hardening change or just routine refactoring.
Recommended action
Treat this as a refactor requiring follow-up review. Verify that the newer preflight_batch_pczt_cypherpunk / preflight_pczt_cypherpunk and sign_checked_batch_pczt / sign_checked_pczt paths enforce equivalent or stronger checks for (1) existence of signable shielded inputs, (2) unsupported Sapling outputs, and (3) presence of spend authorization signatures after signing. If the replacement functions already perform these checks, the change is safe. If not, the removed checks should be reintroduced or the signing flow should be updated to retain them. No immediate patch is indicated by the diff alone.
Security signals we found
01
Removal of explicit byte-level postflight signature verification for shielded Zcash actions
02
Removal of explicit preflight check that at least one signable shielded action exists before batch signing
03
FFI signature change drops unified full viewing key (ufvk) from signing path
04
Comments state ufvk is now consumed during preflight, implying trust shift to earlier stage
05
No new input validation or error handling added in the visible diff
Evidence from the diff
The patch deletes three Rust helper functions from rust/apps/zcash/src/lib.rs: ensure_pczt_has_signable_shielded_action, ensure_signable_shielded_actions_are_signed, and ensure_owned_supported_shielded_actions_are_signed. These were guarded by #[cfg(feature = “cypherpunk”)] and performed byte-level PCZT parsing to confirm signable shielded actions existed and were signed. The FFI layer in rust/rust_c/src/zcash/mod.rs drops the ufvk (unified full viewing key) parameter from sign_zcash_batch_tx_cypherpunk, sign_zcash_tx_cypherpunk, and their unlimited variants, with comments noting that ufvk is now consumed by preflight. The C UI layer in src/ui/gui_chain/multi/gui_zcash.c stops fetching the ufvk and stops passing it. Tests are updated to use newer preflight/sign_checked functions. The diff shows a refactor, not a direct vulnerability fix, and the security implications depend on whether the newer preflight path already covers the removed checks.
Changed components
rust/apps/zcash/src/lib.rsrust/rust_c/src/zcash/mod.rssrc/ui/gui_chain/multi/gui_zcash.csrc/ui/gui_chain/multi/gui_zcash.hInspect captured patch +14 / −234
diff --git a/rust/apps/zcash/src/lib.rs b/rust/apps/zcash/src/lib.rs
index 03d0ab7..71ded25 100644
--- a/rust/apps/zcash/src/lib.rs
+++ b/rust/apps/zcash/src/lib.rs
@@ -663,98 +663,6 @@ fn ensure_shielded_actions_are_signed(
Ok(verifier.finish())
}
-/// Checks whether the PCZT contains at least one non-dummy supported shielded
-/// action that can be signed by the account identified by `seed_fingerprint` and
-/// `account_index`.
-///
-/// `sign_pczt` intentionally returns a redacted PCZT even when no key matched.
-/// Batch signing needs this explicit preflight so one approval cannot silently
-/// produce a result with zero shielded signatures for an entry.
-#[cfg(feature = "cypherpunk")]
-pub fn ensure_pczt_has_signable_shielded_action<P: consensus::Parameters>(
- params: &P,
- pczt: &[u8],
- seed_fingerprint: &[u8; 32],
- account_index: u32,
-) -> Result<()> {
- let pczt = pczt::parse_pczt(pczt)?;
- let account_index = zip32::AccountId::try_from(account_index)
- .map_err(|_e| ZcashError::InvalidDataError("invalid account index".to_string()))?;
-
- let (signable_actions, _pczt) = signable_shielded_actions(
- params,
- pczt,
- seed_fingerprint,
- account_index,
- ShieldedActionPolicy::Batch,
- )?;
- if signable_actions.is_empty() {
- Err(ZcashError::PcztNoMyInputs)
- } else {
- Ok(())
- }
-}
-
-/// Confirms that every signable supported shielded action in `unsigned_pczt`
-/// has a spend authorization signature in the same position in `signed_pczt`.
-#[cfg(feature = "cypherpunk")]
-pub fn ensure_signable_shielded_actions_are_signed<P: consensus::Parameters>(
- params: &P,
- unsigned_pczt: &[u8],
- signed_pczt: &[u8],
- seed_fingerprint: &[u8; 32],
- account_index: u32,
-) -> Result<()> {
- let unsigned_pczt = pczt::parse_pczt(unsigned_pczt)?;
- let account_index = zip32::AccountId::try_from(account_index)
- .map_err(|_e| ZcashError::InvalidDataError("invalid account index".to_string()))?;
- let (signable_actions, _pczt) = signable_shielded_actions(
- params,
- unsigned_pczt,
- seed_fingerprint,
- account_index,
- ShieldedActionPolicy::Batch,
- )?;
- if signable_actions.is_empty() {
- Err(ZcashError::PcztNoMyInputs)
- } else {
- let signed_pczt = pczt::parse_pczt(signed_pczt)
- .map_err(|_| ZcashError::InvalidPczt("invalid signed pczt data".to_string()))?;
- ensure_shielded_actions_are_signed(signed_pczt, &signable_actions)?;
- Ok(())
- }
-}
-
-/// Confirms that supported shielded actions owned by this account were signed
-/// without applying the batch-only shielded input policy to ordinary PCZTs.
-#[cfg(feature = "cypherpunk")]
-pub fn ensure_owned_supported_shielded_actions_are_signed<P: consensus::Parameters>(
- params: &P,
- unsigned_pczt: &[u8],
- signed_pczt: &[u8],
- seed_fingerprint: &[u8; 32],
- account_index: u32,
-) -> Result<()> {
- let unsigned_pczt = pczt::parse_pczt(unsigned_pczt)?;
- let account_index = zip32::AccountId::try_from(account_index)
- .map_err(|_e| ZcashError::InvalidDataError("invalid account index".to_string()))?;
- let (signable_actions, _pczt) = signable_shielded_actions(
- params,
- unsigned_pczt,
- seed_fingerprint,
- account_index,
- ShieldedActionPolicy::Single,
- )?;
- if signable_actions.is_empty() {
- Ok(())
- } else {
- let signed_pczt = pczt::parse_pczt(signed_pczt)
- .map_err(|_| ZcashError::InvalidPczt("invalid signed pczt data".to_string()))?;
- ensure_shielded_actions_are_signed(signed_pczt, &signable_actions)?;
- Ok(())
- }
-}
-
/// Signs a preflight-checked, normalized PCZT and confirms in memory that every
/// supported shielded action owned by (`seed_fingerprint`, `account_index`)
/// received a spend authorization signature. Single-transaction policy: a PCZT
@@ -1417,137 +1325,30 @@ mod tests {
#[cfg(zcash_unstable = "nu6.3")]
#[test]
- fn test_batch_preflight_accepts_orchard_spend() {
- let sample = pczt::test_support::sample_orchard_change_pczt();
-
- ensure_pczt_has_signable_shielded_action(
- &pczt::test_support::Nu6_3Network,
- &sample.bytes,
- &sample.seed_fingerprint,
- 0,
- )
- .unwrap();
- assert_eq!(
- ensure_pczt_has_signable_shielded_action(
- &pczt::test_support::Nu6_3Network,
- &sample.bytes,
- &sample.seed_fingerprint,
- 1,
- )
- .unwrap_err(),
- ZcashError::PcztNoMyInputs
- );
- }
-
- #[cfg(zcash_unstable = "nu6.3")]
- #[test]
- fn test_batch_postflight_confirms_orchard_signature() {
- let sample = pczt::test_support::sample_orchard_change_pczt();
- let signed = sign_pczt(&sample.bytes, &sample.seed).expect("Orchard PCZT should sign");
-
- ensure_signable_shielded_actions_are_signed(
- &pczt::test_support::Nu6_3Network,
- &sample.bytes,
- &signed,
- &sample.seed_fingerprint,
- 0,
- )
- .unwrap();
- }
-
- #[cfg(zcash_unstable = "nu6.3")]
- #[test]
- fn test_single_postflight_confirms_orchard_signature_when_present() {
- let sample = pczt::test_support::sample_orchard_change_pczt();
-
- assert!(matches!(
- ensure_owned_supported_shielded_actions_are_signed(
- &pczt::test_support::Nu6_3Network,
- &sample.bytes,
- &sample.bytes,
- &sample.seed_fingerprint,
- 0,
- ),
- Err(ZcashError::SigningError(message))
- if message == "signed PCZT is missing an Orchard spend authorization signature"
- ));
-
- let signed = sign_pczt(&sample.bytes, &sample.seed).expect("Orchard PCZT should sign");
- ensure_owned_supported_shielded_actions_are_signed(
- &pczt::test_support::Nu6_3Network,
- &sample.bytes,
- &signed,
- &sample.seed_fingerprint,
- 0,
- )
- .unwrap();
- }
-
- #[cfg(zcash_unstable = "nu6.3")]
- #[test]
- fn test_batch_preflight_rejects_sapling_outputs() {
- let sample = pczt_with_sapling_output();
-
- assert_batch_unsupported_sapling_error(ensure_pczt_has_signable_shielded_action(
- &pczt::test_support::Nu6_3Network,
- &sample.bytes,
- &sample.seed_fingerprint,
- 0,
- ));
- }
-
- #[cfg(zcash_unstable = "nu6.3")]
- #[test]
- fn test_batch_postflight_rejects_sapling_outputs() {
- let sample = pczt_with_sapling_output();
-
- assert_batch_unsupported_sapling_error(ensure_signable_shielded_actions_are_signed(
- &pczt::test_support::Nu6_3Network,
- &sample.bytes,
- &sample.bytes,
- &sample.seed_fingerprint,
- 0,
- ));
- }
-
- #[cfg(zcash_unstable = "nu6.3")]
- #[test]
- fn test_batch_preflight_accepts_ironwood_spend() {
+ fn test_sign_checked_batch_pczt_signs_ironwood_spend() {
let sample = pczt::test_support::sample_ironwood_pczt();
-
- ensure_pczt_has_signable_shielded_action(
+ let normalized = preflight_batch_pczt_cypherpunk(
&pczt::test_support::Nu6_3Network,
&sample.bytes,
+ &sample.ufvk_text,
&sample.seed_fingerprint,
0,
)
.unwrap();
- assert_eq!(
- ensure_pczt_has_signable_shielded_action(
- &pczt::test_support::Nu6_3Network,
- &sample.bytes,
- &sample.seed_fingerprint,
- 1,
- )
- .unwrap_err(),
- ZcashError::PcztNoMyInputs
- );
- }
-
- #[cfg(zcash_unstable = "nu6.3")]
- #[test]
- fn test_batch_postflight_confirms_ironwood_signature() {
- let sample = pczt::test_support::sample_ironwood_pczt();
- let signed = sign_pczt(&sample.bytes, &sample.seed).expect("Ironwood PCZT should sign");
-
- ensure_signable_shielded_actions_are_signed(
+ let signed = sign_checked_batch_pczt(
&pczt::test_support::Nu6_3Network,
- &sample.bytes,
- &signed,
+ &normalized,
+ &sample.seed,
&sample.seed_fingerprint,
0,
)
.unwrap();
+ assert!(Pczt::parse(&signed)
+ .unwrap()
+ .ironwood()
+ .actions()
+ .iter()
+ .any(|action| action.spend().spend_auth_sig().is_some()));
}
#[cfg(zcash_unstable = "nu6.3")]
diff --git a/rust/rust_c/src/zcash/mod.rs b/rust/rust_c/src/zcash/mod.rs
index 6a4fa4b..b1f5def 100644
--- a/rust/rust_c/src/zcash/mod.rs
+++ b/rust/rust_c/src/zcash/mod.rs
@@ -408,7 +408,6 @@ pub unsafe extern "C" fn parse_zcash_batch_tx_cypherpunk(
#[cfg(feature = "cypherpunk")]
unsafe fn sign_zcash_batch_tx_cypherpunk_dynamic(
checked_batch: Ptr<ZcashCheckedPczt>,
- ufvk: PtrString,
seed_fingerprint: PtrBytes,
account_index: u32,
disabled: bool,
@@ -417,8 +416,6 @@ unsafe fn sign_zcash_batch_tx_cypherpunk_dynamic(
max_fragment_length: usize,
allow_multipart: bool,
) -> *mut UREncodeResult {
- // ufvk is consumed by preflight now; dropped in the final cleanup task.
- let _ = ufvk;
if disabled {
return UREncodeResult::from(RustCError::UnsupportedTransaction(
"Zcash requires at least 256-bit entropy (use 33-word Shamir shares)".to_string(),
@@ -514,7 +511,6 @@ unsafe fn sign_zcash_batch_tx_cypherpunk_dynamic(
#[no_mangle]
pub unsafe extern "C" fn sign_zcash_batch_tx_cypherpunk(
checked_batch: Ptr<ZcashCheckedPczt>,
- ufvk: PtrString,
seed_fingerprint: PtrBytes,
account_index: u32,
disabled: bool,
@@ -523,7 +519,6 @@ pub unsafe extern "C" fn sign_zcash_batch_tx_cypherpunk(
) -> *mut UREncodeResult {
sign_zcash_batch_tx_cypherpunk_dynamic(
checked_batch,
- ufvk,
seed_fingerprint,
account_index,
disabled,
@@ -538,7 +533,6 @@ pub unsafe extern "C" fn sign_zcash_batch_tx_cypherpunk(
#[no_mangle]
pub unsafe extern "C" fn sign_zcash_batch_tx_cypherpunk_unlimited(
checked_batch: Ptr<ZcashCheckedPczt>,
- ufvk: PtrString,
seed_fingerprint: PtrBytes,
account_index: u32,
disabled: bool,
@@ -547,7 +541,6 @@ pub unsafe extern "C" fn sign_zcash_batch_tx_cypherpunk_unlimited(
) -> *mut UREncodeResult {
sign_zcash_batch_tx_cypherpunk_dynamic(
checked_batch,
- ufvk,
seed_fingerprint,
account_index,
disabled,
@@ -612,7 +605,6 @@ unsafe fn sign_zcash_tx_dynamic(
#[cfg(feature = "cypherpunk")]
unsafe fn sign_zcash_tx_cypherpunk_dynamic(
checked_pczt: Ptr<ZcashCheckedPczt>,
- ufvk: PtrString,
seed_fingerprint: PtrBytes,
account_index: u32,
disabled: bool,
@@ -620,9 +612,6 @@ unsafe fn sign_zcash_tx_cypherpunk_dynamic(
seed_len: u32,
max_fragment_length: usize,
) -> *mut UREncodeResult {
- // ufvk is consumed by preflight now; the parameter is dropped together with
- // the batch signature in the final cleanup task.
- let _ = ufvk;
if disabled {
return UREncodeResult::from(RustCError::UnsupportedTransaction(
"Zcash requires at least 256-bit entropy (use 33-word Shamir shares)".to_string(),
@@ -678,7 +667,6 @@ unsafe fn sign_zcash_tx_cypherpunk_dynamic(
#[no_mangle]
pub unsafe extern "C" fn sign_zcash_tx_cypherpunk(
checked_pczt: Ptr<ZcashCheckedPczt>,
- ufvk: PtrString,
seed_fingerprint: PtrBytes,
account_index: u32,
disabled: bool,
@@ -687,7 +675,6 @@ pub unsafe extern "C" fn sign_zcash_tx_cypherpunk(
) -> *mut UREncodeResult {
sign_zcash_tx_cypherpunk_dynamic(
checked_pczt,
- ufvk,
seed_fingerprint,
account_index,
disabled,
@@ -701,7 +688,6 @@ pub unsafe extern "C" fn sign_zcash_tx_cypherpunk(
#[no_mangle]
pub unsafe extern "C" fn sign_zcash_tx_cypherpunk_unlimited(
checked_pczt: Ptr<ZcashCheckedPczt>,
- ufvk: PtrString,
seed_fingerprint: PtrBytes,
account_index: u32,
disabled: bool,
@@ -710,7 +696,6 @@ pub unsafe extern "C" fn sign_zcash_tx_cypherpunk_unlimited(
) -> *mut UREncodeResult {
sign_zcash_tx_cypherpunk_dynamic(
checked_pczt,
- ufvk,
seed_fingerprint,
account_index,
disabled,
diff --git a/src/ui/gui_chain/multi/gui_zcash.c b/src/ui/gui_chain/multi/gui_zcash.c
index 9516bef..8bfe9f7 100644
--- a/src/ui/gui_chain/multi/gui_zcash.c
+++ b/src/ui/gui_chain/multi/gui_zcash.c
@@ -362,14 +362,13 @@ UREncodeResult *GuiSignZcashCypherpunkWithSeed(void *data,
uint8_t seed[SEED_LEN] = {0};
uint8_t sfp[32] = {0};
uint32_t zcashAccountIndex = 0;
- char ufvk[ZCASH_UFVK_MAX_LEN + 1] = {0};
bool disabled = !IsZcashSupportedForCurrentMnemonic();
int ret = 0;
do {
ZcashCypherpunkSignFunc selectedSignFunc = unlimited ? unlimitedSignFunc : signFunc;
if (disabled) {
- encodeResult = selectedSignFunc(data, ufvk, sfp, zcashAccountIndex, true, seed, 0);
+ encodeResult = selectedSignFunc(data, sfp, zcashAccountIndex, true, seed, 0);
CHECK_CHAIN_BREAK(encodeResult);
break;
}
@@ -382,13 +381,9 @@ UREncodeResult *GuiSignZcashCypherpunkWithSeed(void *data,
if (ret != 0) {
break;
}
- ret = GetZcashUFVK(GetCurrentAccountIndex(), ufvk);
- if (ret != 0) {
- break;
- }
int len = GetMnemonicType() == MNEMONIC_TYPE_BIP39 ? sizeof(seed) : GetCurrentAccountEntropyLen();
- encodeResult = selectedSignFunc(data, ufvk, sfp, zcashAccountIndex, false, seed, len);
+ encodeResult = selectedSignFunc(data, sfp, zcashAccountIndex, false, seed, len);
CHECK_CHAIN_BREAK(encodeResult);
} while (0);
diff --git a/src/ui/gui_chain/multi/gui_zcash.h b/src/ui/gui_chain/multi/gui_zcash.h
index 7fcd8cf..bb25c55 100644
--- a/src/ui/gui_chain/multi/gui_zcash.h
+++ b/src/ui/gui_chain/multi/gui_zcash.h
@@ -10,7 +10,6 @@ void GuiZcashOverview(lv_obj_t *parent, void *totalData);
PtrT_TransactionCheckResult GuiGetZcashCheckResult(void);
#ifdef CYPHERPUNK_VERSION
typedef UREncodeResult *(*ZcashCypherpunkSignFunc)(void *data,
- PtrString ufvk,
PtrBytes seedFingerprint,
uint32_t accountIndex,
bool disabled,
Risk score
Our methodology →Why this scored 27/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.