From d086c57f2f105f2260670f36452956fa38249edc Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 14 Sep 2022 13:38:00 -0600 Subject: [PATCH] Make init_accounts_table only permit sequential account identifiers. Also, check to ensure that account creation does not exceed the maximum account identifier value. --- zcash_client_backend/src/data_api.rs | 2 +- zcash_client_sqlite/CHANGELOG.md | 4 +++ zcash_client_sqlite/src/error.rs | 9 +++++++ zcash_client_sqlite/src/lib.rs | 4 +++ zcash_client_sqlite/src/wallet/init.rs | 37 ++++++++++++++++++++++++++ 5 files changed, 55 insertions(+), 1 deletion(-) diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 7d188411c..a2eea7f09 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -1,6 +1,5 @@ //! Interfaces for wallet data persistence & low-level wallet utilities. -use secrecy::SecretVec; use std::cmp; use std::collections::HashMap; use std::fmt::Debug; @@ -8,6 +7,7 @@ use std::fmt::Debug; #[cfg(feature = "transparent-inputs")] use std::collections::HashSet; +use secrecy::SecretVec; use zcash_primitives::{ block::BlockHash, consensus::BlockHeight, diff --git a/zcash_client_sqlite/CHANGELOG.md b/zcash_client_sqlite/CHANGELOG.md index 1ec3f1e4c..751eb92eb 100644 --- a/zcash_client_sqlite/CHANGELOG.md +++ b/zcash_client_sqlite/CHANGELOG.md @@ -18,6 +18,10 @@ and this library adheres to Rust's notion of rewinds exceed supported bounds. - `SqliteClientError::DiversifierIndexOutOfRange`, to report when the space of available diversifier indices has been exhausted. + - `SqliteClientError::AccountIdDiscontinuity`, to report when a user attempts + to initialize the accounts table with a noncontiguous set of account identifiers. + - `SqliteClientError::AccountIdOutOfRange`, to report when the maximum account + identifier has been reached. - An `unstable` feature flag; this is added to parts of the API that may change in any release. It enables `zcash_client_backend`'s `unstable` feature flag. - New summary views that may be directly accessed in the sqlite database. diff --git a/zcash_client_sqlite/src/error.rs b/zcash_client_sqlite/src/error.rs index a9afc4a7b..c7315c4cf 100644 --- a/zcash_client_sqlite/src/error.rs +++ b/zcash_client_sqlite/src/error.rs @@ -73,6 +73,13 @@ pub enum SqliteClientError { /// An error occurred deriving a spending key from a seed and an account /// identifier. KeyDerivationError(AccountId), + + /// A caller attempted to initialize the accounts table with a discontinuous + /// set of account identifiers. + AccountIdDiscontinuity, + + /// A caller attempted to construct a new account with an invalid account identifier. + AccountIdOutOfRange, } impl error::Error for SqliteClientError { @@ -113,6 +120,8 @@ impl fmt::Display for SqliteClientError { SqliteClientError::BackendError(e) => write!(f, "{}", e), SqliteClientError::DiversifierIndexOutOfRange => write!(f, "The space of available diversifier indices is exhausted"), SqliteClientError::KeyDerivationError(acct_id) => write!(f, "Key derivation failed for account {:?}", acct_id), + SqliteClientError::AccountIdDiscontinuity => write!(f, "Wallet account identifiers must be sequential."), + SqliteClientError::AccountIdOutOfRange => write!(f, "Wallet account identifiers must be less than 0x7FFFFFFF."), } } } diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 4d8e1707f..8ee156e74 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -412,6 +412,10 @@ impl<'a, P: consensus::Parameters> WalletWrite for DataConnStmtCache<'a, P> { .map(|a| AccountId::from(u32::from(a) + 1)) .unwrap_or_else(|| AccountId::from(0)); + if u32::from(account) >= 0x7FFFFFFF { + return Err(SqliteClientError::AccountIdOutOfRange); + } + let usk = UnifiedSpendingKey::from_seed( &stmts.wallet_db.params, seed.expose_secret(), diff --git a/zcash_client_sqlite/src/wallet/init.rs b/zcash_client_sqlite/src/wallet/init.rs index 37d7141b2..9a8f2720f 100644 --- a/zcash_client_sqlite/src/wallet/init.rs +++ b/zcash_client_sqlite/src/wallet/init.rs @@ -586,6 +586,13 @@ pub fn init_accounts_table( return Err(SqliteClientError::TableNotEmpty); } + // Ensure that the account identifiers are sequential and begin at zero. + if let Some(account_id) = keys.keys().max() { + if usize::try_from(u32::from(*account_id)).unwrap() >= keys.len() { + return Err(SqliteClientError::AccountIdDiscontinuity); + } + } + // Insert accounts atomically wdb.conn.execute("BEGIN IMMEDIATE", NO_PARAMS)?; for (account, key) in keys.iter() { @@ -677,6 +684,7 @@ mod tests { }; use crate::{ + error::SqliteClientError, tests::{self, network}, wallet::get_address, AccountId, WalletDb, @@ -1370,6 +1378,35 @@ mod tests { init_accounts_table(&db_data, &ufvks).unwrap_err(); } + #[test] + fn init_accounts_table_allows_no_gaps() { + let data_file = NamedTempFile::new().unwrap(); + let mut db_data = WalletDb::for_path(data_file.path(), network()).unwrap(); + init_wallet_db(&mut db_data, Some(Secret::new(vec![]))).unwrap(); + + // allow sequential initialization + let seed = [0u8; 32]; + let ufvks = |ids: &[u32]| { + ids.iter() + .map(|a| { + let account = AccountId::from(*a); + UnifiedSpendingKey::from_seed(&network(), &seed, account) + .map(|k| (account, k.to_unified_full_viewing_key())) + .unwrap() + }) + .collect::>() + }; + + // should fail if we have a gap + assert!(matches!( + init_accounts_table(&db_data, &ufvks(&[0, 2])), + Err(SqliteClientError::AccountIdDiscontinuity) + )); + + // should succeed if there are no gaps + assert!(init_accounts_table(&db_data, &ufvks(&[0, 1, 2])).is_ok()); + } + #[test] fn init_blocks_table_only_works_once() { let data_file = NamedTempFile::new().unwrap();