refactor(zcash): remove checked PCZT digest stamp
What the developer wrote
Authored by Adam Tucker
refactor(zcash): remove checked PCZT digest stamp
What changed, and why it matters
This commit removes a security check that verified a Zcash transaction buffer had not been tampered with between the 'check/review' screen and the 'sign' step on a Keystone hardware wallet. Before the change, the device stored a fingerprint (SHA-256 digest) of the reviewed transaction and re-checked it before signing. After the change, it only checks that the buffer pointer is not null. This weakens the defense-in-depth boundary between the C code and the Rust code that handles signing, but the commit does not by itself create a known exploitable bug.
Treat this as a security-relevant design change requiring review. Re-evaluate whether the removed digest check is compensated by another integrity mechanism (e.g., immutable C buffer ownership, secure UI-to-signing IPC, or a higher-level signature over the PCZT). If no compensating control exists, consider restoring a lightweight digest check or documenting the threat-model rationale. Add tests covering C-side tampering scenarios if they are still expected to be rejected.
Security signals we found
Removal of a digest-based integrity check on a signing input buffer
Renaming of verified_bytes() to checked_bytes() with weakened semantics
Cross-language (C/Rust) boundary no longer re-validates buffer contents
Defense-in-depth reduction for Zcash PCZT signing flow
No replacement integrity mechanism introduced in the diff
Evidence from the diff
The patch refactors ZcashCheckedPczt in rust/rust_c/src/zcash/structs.rs: it drops the digest field and the sha256 import, renames verified_bytes() to checked_bytes(), and removes the runtime SHA-256 digest comparison that guarded against the C side handing back a different or corrupted buffer than the one that was checked and displayed. All call sites in mod.rs are updated from verified_bytes() to checked_bytes(). The commit message frames this as a refactor (‘remove checked PCZT digest stamp’). The change reduces tamper-evidence between the UI/check stage and the parse/sign stage, which is a security-relevant design decision, but no exploit path is demonstrated in the diff.
Changed components
rust/rust_c/src/zcash/structs.rsrust/rust_c/src/zcash/mod.rsZcash PCZT single-transaction signing flowZcash PCZT batch signing flowZcashCheckedPczt FFI structInspect captured patch +14 / −36
diff --git a/rust/rust_c/src/zcash/mod.rs b/rust/rust_c/src/zcash/mod.rs
index abfcb23..cacf30d 100644
--- a/rust/rust_c/src/zcash/mod.rs
+++ b/rust/rust_c/src/zcash/mod.rs
@@ -169,7 +169,7 @@ pub unsafe extern "C" fn parse_zcash_tx_cypherpunk(
.c_ptr();
}
let checked = extract_ptr_with_type!(checked_pczt, ZcashCheckedPczt);
- let bytes = match checked.verified_bytes() {
+ let bytes = match checked.checked_bytes() {
Ok(bytes) => bytes,
Err(e) => return TransactionParseResult::from(e).c_ptr(),
};
@@ -195,7 +195,7 @@ pub unsafe extern "C" fn parse_zcash_tx_multi_coins(
.c_ptr();
}
let checked = extract_ptr_with_type!(checked_pczt, ZcashCheckedPczt);
- let bytes = match checked.verified_bytes() {
+ let bytes = match checked.checked_bytes() {
Ok(bytes) => bytes,
Err(e) => return TransactionParseResult::from(e).c_ptr(),
};
@@ -374,7 +374,7 @@ pub unsafe extern "C" fn parse_zcash_batch_tx_cypherpunk(
.c_ptr();
}
let checked = extract_ptr_with_type!(checked_batch, ZcashCheckedPczt);
- let bytes = match checked.verified_bytes() {
+ let bytes = match checked.checked_bytes() {
Ok(bytes) => bytes,
Err(e) => return TransactionParseResult::from(e).c_ptr(),
};
@@ -432,7 +432,7 @@ unsafe fn sign_zcash_batch_tx_cypherpunk_dynamic(
let expected_seed_fingerprint: &[u8; 32] = expected_seed_fingerprint.try_into().unwrap();
let mut seed = extract_array_mut!(seed, u8, seed_len as usize);
- let result = match checked.verified_bytes() {
+ let result = match checked.checked_bytes() {
Ok(bytes) => match ZcashSignBatch::try_from(bytes.to_vec()) {
Ok(batch) => match calculate_seed_fingerprint(seed) {
Ok(seed_fingerprint) => {
@@ -580,7 +580,7 @@ unsafe fn sign_zcash_tx_dynamic(
}
let checked = extract_ptr_with_type!(checked_pczt, ZcashCheckedPczt);
let mut seed = extract_array_mut!(seed, u8, seed_len as usize);
- let result = match checked.verified_bytes() {
+ let result = match checked.checked_bytes() {
Ok(bytes) => match app_zcash::sign_pczt(bytes, seed) {
Ok(pczt) => match ZcashPczt::new(pczt).try_into() {
Err(e) => UREncodeResult::from(e).c_ptr(),
@@ -626,7 +626,7 @@ unsafe fn sign_zcash_tx_cypherpunk_dynamic(
let expected_seed_fingerprint: &[u8; 32] = expected_seed_fingerprint.try_into().unwrap();
let mut seed = extract_array_mut!(seed, u8, seed_len as usize);
- let result = match checked.verified_bytes() {
+ let result = match checked.checked_bytes() {
Ok(pczt_bytes) => match calculate_seed_fingerprint(seed) {
Ok(seed_fingerprint) => {
if &seed_fingerprint != expected_seed_fingerprint {
diff --git a/rust/rust_c/src/zcash/structs.rs b/rust/rust_c/src/zcash/structs.rs
index 3fee735..f3c377c 100644
--- a/rust/rust_c/src/zcash/structs.rs
+++ b/rust/rust_c/src/zcash/structs.rs
@@ -12,7 +12,6 @@ use alloc::{string::ToString, vec::Vec};
use app_zcash::pczt::structs::{
ParsedFrom, ParsedOrchard, ParsedPczt, ParsedTo, ParsedTransparent,
};
-use cryptoxide::hashing::sha256;
use cstr_core;
#[repr(C)]
@@ -221,43 +220,30 @@ impl_c_ptrs!(
/// the check, display, and sign stages (`checked_PCZT` on the C side).
///
/// `data` is opaque to C: the normalized PCZT encoding in the single-transaction
-/// flow, or the normalized `ZcashSignBatch` CBOR in the batch flow. `digest` is
-/// the SHA-256 of those bytes, stamped during check; `verified_bytes` recomputes
-/// and compares it so parse/sign only operate on bytes produced by a successful
-/// check. Construct exclusively from check results.
+/// flow, or the normalized `ZcashSignBatch` CBOR in the batch flow. Construct
+/// exclusively from check results.
#[repr(C)]
pub struct ZcashCheckedPczt {
pub data: Ptr<VecFFI<u8>>,
- pub digest: [u8; 32],
}
impl ZcashCheckedPczt {
- /// Wraps bytes verified during check and stamps their digest.
+ /// Wraps bytes verified during check.
pub fn new(data: Vec<u8>) -> Self {
- let digest = sha256(&data);
Self {
data: VecFFI::from(data).c_ptr(),
- digest,
}
}
- /// Borrows the checked bytes after rechecking the digest stamped during
- /// check, guarding against C handing back a different or corrupted
- /// buffer than the one that was checked and displayed.
- pub unsafe fn verified_bytes(&self) -> Result<&[u8], RustCError> {
+ /// Borrows the bytes produced by a successful check.
+ pub unsafe fn checked_bytes(&self) -> Result<&[u8], RustCError> {
if self.data.is_null() {
return Err(RustCError::InvalidData(
"checked PCZT has no data".to_string(),
));
}
let vec = &*self.data;
- let bytes = slice::from_raw_parts(vec.data, vec.size);
- if sha256(bytes) != self.digest {
- return Err(RustCError::InvalidData(
- "checked PCZT digest mismatch".to_string(),
- ));
- }
- Ok(bytes)
+ Ok(slice::from_raw_parts(vec.data, vec.size))
}
}
@@ -278,18 +264,10 @@ mod tests {
use alloc::vec::Vec;
#[test]
- fn test_checked_pczt_digest_round_trip() {
+ fn test_checked_pczt_bytes_round_trip() {
let checked = ZcashCheckedPczt::new(b"normalized-bytes".to_vec());
- let bytes = unsafe { checked.verified_bytes() }.unwrap();
+ let bytes = unsafe { checked.checked_bytes() }.unwrap();
assert_eq!(bytes, b"normalized-bytes");
unsafe { checked.free() };
}
-
- #[test]
- fn test_checked_pczt_digest_mismatch_is_rejected() {
- let mut checked = ZcashCheckedPczt::new(b"normalized-bytes".to_vec());
- checked.digest[0] ^= 0xff;
- assert!(unsafe { checked.verified_bytes() }.is_err());
- unsafe { checked.free() };
- }
}
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.