fix: return InvalidData instead of panicking on crafted wallet bytes

zingolib/src/wallet/disk.rs

@@ -103,6 +103,17 @@ impl<R: Read> Read for CountingReader<R> {
     }
 }
 
+/// Reads a little-endian u32 and rejects values outside the ZIP 32 account id range as `InvalidData`.
+fn read_account_id<R: Read>(reader: &mut R) -> io::Result<zip32::AccountId> {
+    let raw_account_id = reader.read_u32::<LittleEndian>()?;
+    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::<LittleEndian>()?
-                .try_into()
-                .expect("should never overflow"),
-        );
+        let stored_birthday = reader.read_u64::<LittleEndian>()?;
+        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::<LittleEndian>()?)
-                        .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::<BTreeMap<_, _>>()
@@ -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::<LittleEndian>()?)
-                .expect("only valid account ids are stored");
+            let account_id = read_account_id(r)?;
             let address_index = r.read_u32::<LittleEndian>()?;
             let receivers = ReceiverSelection::read(r, ())?;
 
@@ -577,11 +584,18 @@ impl LightWallet {
         .into_iter()
         .collect::<BTreeMap<_, _>>();
         let mut transparent_addresses = Vector::read(&mut reader, |r| {
-            let account_id = zip32::AccountId::try_from(r.read_u32::<LittleEndian>()?)
-                .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::<LittleEndian>()?)
-                .expect("only non-hardened child indexes should be written");
+            let raw_address_index = r.read_u32::<LittleEndian>()?;
+            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::<LittleEndian>()?)
-                    .expect("only valid non-zero u32s stored")
+                let stored_min_confirmations = reader.read_u32::<LittleEndian>()?;
+                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")
             };

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<u8> {
+        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.