From 4f599a223fc65c55717398dac0efafc8d35c5464 Mon Sep 17 00:00:00 2001 From: zancas Date: Fri, 28 Aug 2026 07:47:10 -0700 Subject: [PATCH] fix: return InvalidData instead of panicking on crafted wallet bytes LightWallet::validate promises an io::Result whose error names the byte offset reached, but the read path it wraps carried seven expect() calls that panic on crafted-but-reachable inputs: a pre-32 birthday above u32::MAX, account ids with the hardened bit set in the unified key store and in both address maps, a hardened transparent address index, a version 35 file whose key store vector holds no account 0, and a stored min_confirmations of zero. Each site now returns an InvalidData error instead, and the three identical account id reads collapse into one read_account_id helper. Five regression tests craft minimal wallet prefixes that reach the five shallow sites and assert validate returns an error rather than aborting. The birthday and min_confirmations sites sit behind fields whose grammars are impractical to hand-craft, so they are fixed by the same mechanical transformation without dedicated fixtures. Co-Authored-By: Claude Fable 5 --- zingolib/src/wallet/disk.rs | 64 ++++++++----- zingolib/src/wallet/disk/testing/tests.rs | 105 +++++++++++++++++++++- 2 files changed, 148 insertions(+), 21 deletions(-) diff --git a/zingolib/src/wallet/disk.rs b/zingolib/src/wallet/disk.rs index 4876e1fb3..32a87dcac 100644 --- a/zingolib/src/wallet/disk.rs +++ b/zingolib/src/wallet/disk.rs @@ -103,6 +103,17 @@ impl Read for CountingReader { } } +/// Reads a little-endian u32 and rejects values outside the ZIP 32 account id range as `InvalidData`. +fn read_account_id(reader: &mut R) -> io::Result { + let raw_account_id = reader.read_u32::()?; + zip32::AccountId::try_from(raw_account_id).map_err(|_| { + Error::new( + ErrorKind::InvalidData, + format!("invalid account id {raw_account_id} stored in wallet file"), + ) + }) +} + fn check_saved_chain(saved_network: &str, chain_type: &ChainType) -> io::Result<()> { if saved_network == chain_type.to_string() { Ok(()) @@ -309,12 +320,13 @@ impl LightWallet { } else { WalletOptions::read(&mut reader)? }; - let birthday = BlockHeight::from_u32( - reader - .read_u64::()? - .try_into() - .expect("should never overflow"), - ); + let stored_birthday = reader.read_u64::()?; + let birthday = BlockHeight::from_u32(stored_birthday.try_into().map_err(|_| { + Error::new( + ErrorKind::InvalidData, + format!("stored birthday {stored_birthday} exceeds the maximum block height"), + ) + })?); if version <= 22 { let _sapling_tree_verified = if version <= 12 { @@ -533,11 +545,7 @@ impl LightWallet { let unified_key_store = if version >= 35 { Vector::read(&mut reader, |r| { - Ok(( - zip32::AccountId::try_from(r.read_u32::()?) - .expect("only valid account ids are stored"), - UnifiedKeyStore::read(r, chain_type)?, - )) + Ok((read_account_id(r)?, UnifiedKeyStore::read(r, chain_type)?)) })? .into_iter() .collect::>() @@ -551,8 +559,7 @@ impl LightWallet { }; let mut unified_addresses = Vector::read(&mut reader, |r| { - let account_id = zip32::AccountId::try_from(r.read_u32::()?) - .expect("only valid account ids are stored"); + let account_id = read_account_id(r)?; let address_index = r.read_u32::()?; let receivers = ReceiverSelection::read(r, ())?; @@ -577,11 +584,18 @@ impl LightWallet { .into_iter() .collect::>(); let mut transparent_addresses = Vector::read(&mut reader, |r| { - let account_id = zip32::AccountId::try_from(r.read_u32::()?) - .expect("only valid account ids are stored"); + let account_id = read_account_id(r)?; let scope = TransparentScope::try_from(r.read_u8()?)?; - let address_index = NonHardenedChildIndex::from_index(r.read_u32::()?) - .expect("only non-hardened child indexes should be written"); + let raw_address_index = r.read_u32::()?; + let address_index = + NonHardenedChildIndex::from_index(raw_address_index).ok_or_else(|| { + Error::new( + ErrorKind::InvalidData, + format!( + "hardened transparent address index {raw_address_index} stored in wallet file" + ), + ) + })?; Ok(( TransparentAddressId::new(account_id, scope, address_index), @@ -608,7 +622,12 @@ impl LightWallet { if version < 36 { let unified_key = unified_key_store .get(&zip32::AccountId::ZERO) - .expect("account 0 must exist"); + .ok_or_else(|| { + Error::new( + ErrorKind::InvalidData, + "wallet file stores no key for account 0", + ) + })?; unified_addresses = BTreeMap::new(); if let Some(receivers) = unified_key.default_receivers() { let unified_address_id = UnifiedAddressId { @@ -687,8 +706,13 @@ impl LightWallet { let wallet_settings = if version >= 33 { let sync_config = SyncConfig::read(&mut reader)?; let min_confirmations = if version >= 38 { - NonZeroU32::try_from(reader.read_u32::()?) - .expect("only valid non-zero u32s stored") + let stored_min_confirmations = reader.read_u32::()?; + NonZeroU32::try_from(stored_min_confirmations).map_err(|_| { + Error::new( + ErrorKind::InvalidData, + "min_confirmations of zero stored in wallet file", + ) + })? } else { NonZeroU32::try_from(3).expect("hard-coded non-zero integer") }; diff --git a/zingolib/src/wallet/disk/testing/tests.rs b/zingolib/src/wallet/disk/testing/tests.rs index 2b88cc40a..9d96712d8 100644 --- a/zingolib/src/wallet/disk/testing/tests.rs +++ b/zingolib/src/wallet/disk/testing/tests.rs @@ -660,8 +660,10 @@ mod validation { use zingo_common_components::protocol::ActivationHeights; + use pepper_sync::keys::transparent::TransparentScope; + use crate::config::ChainType; - use crate::wallet::LightWallet; + use crate::wallet::{LightWallet, utils}; use super::current_version_wallet_bytes; use super::{ @@ -848,6 +850,107 @@ mod validation { assert_eq!(error.kind(), std::io::ErrorKind::InvalidData); } + /// The lowest u32 with the ZIP 32 hardened-derivation bit set, valid neither as an account id nor as a non-hardened child index. + const FIRST_HARDENED_INDEX: u32 = 1 << 31; + + /// The chain tag byte the current wallet version stores for mainnet. + const MAINNET_CHAIN_TAG: u8 = 0; + + /// The filler byte the crafted fixtures use as seed entropy. + const SEED_ENTROPY_FILL: u8 = 0x55; + + /// The oldest wallet version that stores the unified key store as a vector of account entries. + const FIRST_VECTORED_KEY_STORE_VERSION: u64 = 35; + + /// Builds a current-version wallet prefix through the birthday field, ready for a crafted tail. + fn current_version_prefix_through_birthday() -> Vec { + let mut bytes = LightWallet::serialized_version().to_le_bytes().to_vec(); + bytes.push(MAINNET_CHAIN_TAG); + let seed_entropy = [SEED_ENTROPY_FILL; 32]; + bytes.push(seed_entropy.len() as u8); + bytes.extend_from_slice(&seed_entropy); + bytes.extend_from_slice(&0u32.to_le_bytes()); + bytes + } + + /// [`LightWallet::validate`] returns an error, rather than panicking, on a key store entry whose account id has the hardened bit set. + #[test] + fn validate_rejects_a_hardened_key_store_account_id() { + let mut bytes = current_version_prefix_through_birthday(); + bytes.push(1); + bytes.extend_from_slice(&FIRST_HARDENED_INDEX.to_le_bytes()); + + let error = LightWallet::validate(bytes.as_slice(), ChainType::Mainnet) + .expect_err("a hardened account id must be rejected, not panicked on"); + assert_eq!(error.kind(), std::io::ErrorKind::InvalidData); + assert!( + error + .to_string() + .contains(&FIRST_HARDENED_INDEX.to_string()), + "the error must name the rejected account id: {error}" + ); + } + + /// [`LightWallet::validate`] returns an error, rather than panicking, on a unified address whose account id has the hardened bit set. + #[test] + fn validate_rejects_a_hardened_unified_address_account_id() { + let mut bytes = current_version_prefix_through_birthday(); + bytes.push(0); + bytes.push(1); + bytes.extend_from_slice(&FIRST_HARDENED_INDEX.to_le_bytes()); + + let error = LightWallet::validate(bytes.as_slice(), ChainType::Mainnet) + .expect_err("a hardened account id must be rejected, not panicked on"); + assert_eq!(error.kind(), std::io::ErrorKind::InvalidData); + } + + /// [`LightWallet::validate`] returns an error, rather than panicking, on a transparent address whose account id has the hardened bit set. + #[test] + fn validate_rejects_a_hardened_transparent_address_account_id() { + let mut bytes = current_version_prefix_through_birthday(); + bytes.push(0); + bytes.push(0); + bytes.push(1); + bytes.extend_from_slice(&FIRST_HARDENED_INDEX.to_le_bytes()); + + let error = LightWallet::validate(bytes.as_slice(), ChainType::Mainnet) + .expect_err("a hardened account id must be rejected, not panicked on"); + assert_eq!(error.kind(), std::io::ErrorKind::InvalidData); + } + + /// [`LightWallet::validate`] returns an error, rather than panicking, on a transparent address index with the hardened bit set. + #[test] + fn validate_rejects_a_hardened_transparent_address_index() { + let mut bytes = current_version_prefix_through_birthday(); + bytes.push(0); + bytes.push(0); + bytes.push(1); + bytes.extend_from_slice(&0u32.to_le_bytes()); + bytes.push(TransparentScope::External as u8); + bytes.extend_from_slice(&FIRST_HARDENED_INDEX.to_le_bytes()); + + let error = LightWallet::validate(bytes.as_slice(), ChainType::Mainnet) + .expect_err("a hardened address index must be rejected, not panicked on"); + assert_eq!(error.kind(), std::io::ErrorKind::InvalidData); + } + + /// [`LightWallet::validate`] returns an error, rather than panicking, on a version 35 wallet whose key store vector holds no account 0. + #[test] + fn validate_rejects_an_account_zero_free_version_thirty_five_wallet() { + let mut bytes = FIRST_VECTORED_KEY_STORE_VERSION.to_le_bytes().to_vec(); + utils::write_string(&mut bytes, &"main".to_string()) + .expect("writing to a vector cannot fail"); + bytes.push(0); + bytes.extend_from_slice(&0u32.to_le_bytes()); + bytes.push(0); + bytes.push(0); + bytes.push(0); + + let error = LightWallet::validate(bytes.as_slice(), ChainType::Mainnet) + .expect_err("a wallet with no account 0 key must be rejected, not panicked on"); + assert_eq!(error.kind(), std::io::ErrorKind::InvalidData); + } + proptest! { /// Any byte string whose version word falls outside the accepted /// range is rejected by both functions.