From 31304a1a6303bd0510b3abce6abf289a0f575973 Mon Sep 17 00:00:00 2001 From: DanS Date: Mon, 12 Oct 2026 13:22:00 -0500 Subject: [PATCH] fix(lite-backend): durable wallet saves, non-destructive backup recovery do_save flushed the temp wallet and renamed it over the wallet with no fsync of the temp file, the .bak or the directory, so after a power cut the rename could be on disk while the data was not, leaving a torn wallet. The readers then panicked on malformed bytes instead of returning an error (the gzip Decoder and WalletTx unwraps in LightWallet::read, the version asserts in data.rs and walletzkey.rs, from_utf8/diversifier unwraps, and vec![0; len] on a corrupt length prefix, which can abort the process outright), so read_from_disk's .bak fallback was never reached. And the fallback itself was destructive: it copied the .bak over the wallet before reading it, on ANY error, so the damaged file was lost, a transient I/O error replaced a good newer wallet with an older .bak, and a wallet written by a newer app ("Don't know how to read wallet version") was overwritten too, dropping imported keys that are not in the seed. - do_save: sync_all the temp file (removed on error), copy the .bak, tighten and sync it through a write handle (FlushFileBuffers needs write access on Windows), rename, sync the copy-fallback destination, then fsync the directory on unix. No MoveFileEx write-through on Windows. - read_from_disk: each parse runs in read_wallet_file under catch_unwind (panic = "unwind" is kept for release builds). The .bak is used only when the wallet fails with InvalidData, UnexpectedEof or InvalidInput, and it is read first, so a bad backup leaves both files as they were. A damaged wallet whose header says encrypted is not replaced by a plaintext .bak (only an older build leaves one): the open fails with an explanation and nothing changes. Otherwise the damaged wallet is moved (or, failing that, copied) to .dat.corrupt-, with a -N suffix while that name is taken (a dangling symlink counts as taken), and is never deleted. The recovered wallet is tightened as before. - read_from_disk_reporting returns the same client plus "recovered from backup; damaged file kept as " after a recovery, for the FFI; read_from_disk wraps it. Both stages are also written to stderr, since the logger is not initialised yet when the wallet is opened. - Readers return Err: a newer wallet version is ErrorKind::Unsupported (was InvalidData), and so is a newer WalletTx or key version; Decoder::new and WalletTx::read propagate with ?; a bad Utxo version, a negative or non-UTF-8 or non-R Utxo address, non-UTF-8 metadata addresses, an invalid diversifier and a viewing key with no default address are InvalidData. Every length-prefixed string and address is read through read_bytes_bounded (take(len).read_to_end, no cap, std's own short-read error), so a corrupt length is never allocated up front. Flagged behaviour changes: - fsync order: tmp, then .bak, then rename, then the dir (unix); saves at exit, encrypt and decrypt still run on the UI thread and get slower on slow disks; - the readers return Err instead of panicking; - the .bak fallback runs only for corrupt files, and the damaged file is kept as .dat.corrupt- (every copy is kept; one from a plaintext era keeps its seed, inside the 0700 dir); - a wallet with a newer format errors (Unsupported) and is never replaced; - a damaged encrypted wallet whose backup predates encryption is not opened. Wallet format v9 is unchanged; valid files read and write byte for byte as before. No ABI or symbol change. Tests: a wallet whose parse panicked recovers from the .bak, the garbage is kept as .dat.corrupt- and the report names it, and the next open reports nothing; a newer-version wallet is Unsupported and both files are unchanged (this fails when only the version error kind is reverted); an unreadable (0000) wallet is a PermissionDenied error that changes nothing either; a corrupt wallet with a corrupt or no .bak changes nothing; a damaged encrypted wallet with a pre-encryption plaintext .bak is refused with both files unchanged and nothing moved aside; a recovery never overwrites an earlier .dat.corrupt- or follows a dangling symlink; every truncation of a wallet and of a WalletTx is an Err, crafted Utxo, metadata, note and key records are the expected Err kinds, and a u64::MAX length returns at once. Co-Authored-By: Claude Opus 5.5 --- .../lib/src/lightclient.rs | 307 ++++++++++++++++-- .../lib/src/lightwallet.rs | 6 +- .../lib/src/lightwallet/data.rs | 45 ++- .../lib/src/lightwallet/tests.rs | 190 +++++++++++ .../lib/src/lightwallet/utils.rs | 16 +- .../lib/src/lightwallet/walletzkey.rs | 13 +- 6 files changed, 535 insertions(+), 42 deletions(-) diff --git a/third_party/silentdragonxlite/silentdragonxlite-cli/lib/src/lightclient.rs b/third_party/silentdragonxlite/silentdragonxlite-cli/lib/src/lightclient.rs index ddbcfab..4fe9c7e 100644 --- a/third_party/silentdragonxlite/silentdragonxlite-cli/lib/src/lightclient.rs +++ b/third_party/silentdragonxlite/silentdragonxlite-cli/lib/src/lightclient.rs @@ -587,6 +587,12 @@ impl LightClient { } pub fn read_from_disk(config: &LightClientConfig) -> io::Result { + Self::read_from_disk_reporting(config).map(|(lc, _)| lc) + } + + /// read_from_disk, plus what the open did to the files (s2-D14): Some("recovered from backup; damaged file kept as + /// ") when a corrupt wallet was replaced by its backup, for the FFI to pass on. + pub fn read_from_disk_reporting(config: &LightClientConfig) -> io::Result<(Self, Option)> { if !config.wallet_exists() { // Try to recover from backup let bak_path = config.get_wallet_path().with_extension("dat.bak"); @@ -601,25 +607,43 @@ impl LightClient { } } - // Try to open the wallet file; if it fails (corrupted/truncated), try the backup - let wallet = match File::open(config.get_wallet_path()) - .and_then(|f| { - let mut file_buffer = BufReader::new(f); - LightWallet::read(&mut file_buffer, config) - }) { - Ok(w) => w, - Err(e) => { - warn!("Failed to read wallet file: {}, trying backup", e); - let bak_path = config.get_wallet_path().with_extension("dat.bak"); - if bak_path.exists() { - std::fs::copy(&bak_path, config.get_wallet_path())?; - tighten(&config.get_wallet_path(), false); - let mut file_buffer = BufReader::new(File::open(config.get_wallet_path())?); - LightWallet::read(&mut file_buffer, config)? - } else { + // C244: fall back to the backup ONLY for a corrupt or torn file: a parser error, or a parser panic, which + // read_wallet_file turns into InvalidData. Never for a transient I/O error (a sharing violation, EACCES) or a + // wallet written by a newer app (Unsupported, s2-D4): both would replace good data with an older .bak. + let wallet_path = config.get_wallet_path(); + let bak_path = wallet_path.with_extension("dat.bak"); + let (wallet, recovered) = match Self::read_wallet_file(&wallet_path, config) { + Ok(w) => (w, None), + Err(e) if matches!(e.kind(), ErrorKind::InvalidData | ErrorKind::UnexpectedEof | ErrorKind::InvalidInput) => { + // The logger is not set up yet (the FFI initialises it after the open), so this goes to stderr. + let _ = writeln!(io::stderr(), "Wallet file unreadable ({}); trying the backup", e); + if !bak_path.exists() { return Err(e); } + // Read the backup FIRST: if it is bad too, both files are left exactly as they were. + let w = Self::read_wallet_file(&bak_path, config)?; + // s2-D12: never silently drop wallet encryption. Only a .bak last saved by Lite <= 1.1.1 can predate + // the encryption of its wallet (since s1-D6 the first encrypted save backs up the encrypted file). + if !w.is_encrypted() && wallet_header_encrypted(&wallet_path) == Some(true) { + return Err(io::Error::new(ErrorKind::InvalidData, + "The wallet file is damaged and its backup was made before the wallet was encrypted. \ + Nothing was changed. Restore from your seed phrase, or move the damaged wallet file aside \ + to open the older, unencrypted backup.")); + } + // s2-D13: keep the damaged file, and never overwrite an earlier one: move it aside, or copy it aside + // when it cannot be moved, before the backup replaces it. + let corrupt = Self::unused_corrupt_path(&wallet_path); + if let Err(re) = std::fs::rename(&wallet_path, &corrupt) { + std::fs::copy(&wallet_path, &corrupt).map_err(|ce| io::Error::new(ce.kind(), + format!("Couldn't keep the damaged wallet file: {} / {}", re, ce)))?; + } + std::fs::copy(&bak_path, &wallet_path)?; + tighten(&wallet_path, false); + let name = corrupt.file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_default(); + let _ = writeln!(io::stderr(), "Wallet restored from backup; damaged file kept as {}", corrupt.display()); + (w, Some(format!("recovered from backup; damaged file kept as {}", name))) } + Err(e) => return Err(e), }; // C254: a wallet or .bak written before owner-only saves (0644 under the default umask) is healed on every @@ -643,7 +667,32 @@ impl LightClient { info!("Read wallet with birthday {}", lc.wallet.read().unwrap().get_first_tx_block()); info!("Created LightClient to {}", &config.server); - Ok(lc) + Ok((lc, recovered)) + } + + /// C244: one parse of a wallet file. A parser panic on a corrupt file becomes InvalidData (lib/Cargo.toml keeps + /// panic = "unwind" for release builds, so the catch is effective). + fn read_wallet_file(path: &Path, config: &LightClientConfig) -> io::Result { + let f = File::open(path)?; + match std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| LightWallet::read(BufReader::new(f), config))) { + Ok(r) => r, + Err(_) => Err(io::Error::new(ErrorKind::InvalidData, "wallet file is corrupt (parser panic)")), + } + } + + /// s2-D13: .dat.corrupt-, with a -1, -2, ... suffix while that name is taken. Any entry counts + /// as taken, a dangling symlink too (symlink_metadata), so an earlier damaged file is never overwritten and a + /// symlink is never followed. + fn unused_corrupt_path(wallet_path: &Path) -> PathBuf { + let ts = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).map(|d| d.as_secs()).unwrap_or(0); + let taken = |p: &Path| std::fs::symlink_metadata(p).is_ok(); + let mut p = wallet_path.with_extension(format!("dat.corrupt-{}", ts)); + let mut i: u64 = 1; + while taken(&p) { + p = wallet_path.with_extension(format!("dat.corrupt-{}-{}", ts, i)); + i += 1; + } + p } pub fn init_logging(&self) -> io::Result<()> { @@ -869,6 +918,14 @@ impl LightClient { // Only proceed with rename if the write succeeded if r.is_ok() { + // C244: make the new bytes durable BEFORE they can replace the old file (FlushFileBuffers on Windows, + // F_FULLFSYNC on macOS). Order: the tmp file, the .bak, the rename, then the directory (unix), so every + // crash point leaves a complete wallet or a complete old wallet. + file_buffer.get_ref().sync_all().map_err(|e| { + let _ = std::fs::remove_file(&tmp_path); + format!("Failed to sync wallet file: {}", e) + })?; + // Create backup of existing wallet (if it exists) if wallet_path.exists() { // s1-D6: at the plaintext -> encrypted transition, back up the NEW (encrypted) file, so no @@ -876,8 +933,12 @@ impl LightClient { let bak_src: &Path = if wallet.is_encrypted() && wallet_header_encrypted(&wallet_path) == Some(false) { tmp_path.as_path() } else { &*wallet_path }; match std::fs::copy(bak_src, &bak_path) { - // fs::copy gives the .bak its source's mode: strip the group/other bits (C254) - Ok(_) => tighten(&bak_path, false), + // fs::copy gives the .bak its source's mode: strip the group/other bits (C254). Then make it + // durable (C244) through a write handle: FlushFileBuffers fails on a read-only one on Windows. + Ok(_) => { + tighten(&bak_path, false); + if let Ok(f) = std::fs::OpenOptions::new().write(true).open(&bak_path) { let _ = f.sync_all(); } + } Err(e) => { warn!("Failed to create wallet backup: {}", e); // Non-fatal: continue with the save @@ -893,8 +954,17 @@ impl LightClient { let _ = std::fs::remove_file(&tmp_path); return Err(format!("Failed to save wallet: {} / {}", e, e2)); } + if let Ok(f) = std::fs::OpenOptions::new().write(true).open(&wallet_path) { let _ = f.sync_all(); } let _ = std::fs::remove_file(&tmp_path); } + // C244: the rename is durable once its directory is. Unix only: a directory cannot be fsynced on + // Windows, and the rename is not write-through there (s2-D3). + #[cfg(unix)] + { + if let Some(dir) = wallet_path.parent() { + if let Ok(d) = File::open(dir) { let _ = d.sync_all(); } + } + } } } @@ -2609,4 +2679,203 @@ pub fn test_bad_import() { } } + // C244 (s2 tests 1-3, the X5 test for s2-D12 and the s2-D13 no-overwrite test): read_from_disk falls back to the + // .bak only for a corrupt wallet, keeps the damaged file, and changes nothing in every other case. + fn corrupt_files(dir: &std::path::Path) -> Vec { + let mut v: Vec = std::fs::read_dir(dir).unwrap().map(|e| e.unwrap().path()) + .filter(|p| p.file_name().unwrap().to_string_lossy().contains(".dat.corrupt-")).collect(); + v.sort(); + v + } + + // A saved wallet and its .bak (the second save makes the .bak). + fn saved_pair(tmp: &TempDir) -> (LightClientConfig, std::path::PathBuf, std::path::PathBuf) { + let config = main_config(tmp); + let wallet = config.get_wallet_path().into_path_buf(); + let bak = wallet.with_extension("dat.bak"); + let lc = LightClient::new_from_phrase(TEST_SEED.to_string(), &config, 0, 0, false).unwrap(); + lc.do_save().unwrap(); + lc.do_save().unwrap(); + assert!(bak.exists()); + (config, wallet, bak) + } + + // Version 5 selects the gzip path, and 0xAB is no gzip header: Decoder::new(..).unwrap() (LW:323) panicked on it. + fn gzip_garbage() -> Vec { + let mut b = 5u64.to_le_bytes().to_vec(); + b.extend_from_slice(&[0xAB; 32]); + b + } + + fn read_err(config: &LightClientConfig) -> std::io::Error { + match LightClient::read_from_disk(config) { + Ok(_) => panic!("the wallet opened"), + Err(e) => e, + } + } + + // s2 C244 test 1: a parse that panicked recovers from the .bak, and the damaged file is kept as .dat.corrupt-. + #[test] + pub fn test_read_from_disk_recovers_when_parse_would_panic() { + let tmp = TempDir::new("lctest").unwrap(); + let (config, wallet, bak) = saved_pair(&tmp); + let bak_bytes = std::fs::read(&bak).unwrap(); + std::fs::write(&wallet, gzip_garbage()).unwrap(); + + let (lc, note) = LightClient::read_from_disk_reporting(&config).unwrap(); + assert_eq!(lc.do_seed_phrase().unwrap()["seed"], TEST_SEED.to_string()); + let kept = corrupt_files(tmp.path()); + assert_eq!(kept.len(), 1, "the damaged file is kept"); + assert_eq!(std::fs::read(&kept[0]).unwrap(), gzip_garbage()); + assert_eq!(std::fs::read(&wallet).unwrap(), bak_bytes, "the wallet is the backup now"); + assert_eq!(std::fs::read(&bak).unwrap(), bak_bytes, "the backup is untouched"); + let name = kept[0].file_name().unwrap().to_string_lossy().into_owned(); + assert!(name.starts_with("silentdragonxlite-wallet.dat.corrupt-"), "{}", name); + assert_eq!(note, Some(format!("recovered from backup; damaged file kept as {}", name))); + drop(lc); + + // The next open is a plain one: nothing to report, nothing moved. + let (lc, note) = LightClient::read_from_disk_reporting(&config).unwrap(); + assert_eq!(note, None); + assert_eq!(corrupt_files(tmp.path()), kept); + assert_eq!(lc.do_seed_phrase().unwrap()["seed"], TEST_SEED.to_string()); + } + + // s2 C244 test 2 (s2-D4): a wallet written by a newer app is an Unsupported error that changes nothing; the older + // .bak never replaces it. A transient I/O error (EACCES here) does not fall back either. + #[test] + pub fn test_read_from_disk_keeps_newer_version_wallet() { + let tmp = TempDir::new("lctest").unwrap(); + let (config, wallet, bak) = saved_pair(&tmp); + let bak_bytes = std::fs::read(&bak).unwrap(); + let mut newer = u64::MAX.to_le_bytes().to_vec(); + newer.extend_from_slice(b"a wallet from a newer app"); + std::fs::write(&wallet, &newer).unwrap(); + + let e = read_err(&config); + assert_eq!(e.kind(), std::io::ErrorKind::Unsupported, "{}", e); + assert!(e.to_string().contains("Don't know how to read wallet version"), "{}", e); + assert_eq!(std::fs::read(&wallet).unwrap(), newer, "the newer wallet was replaced"); + assert_eq!(std::fs::read(&bak).unwrap(), bak_bytes, "the backup was changed"); + assert!(corrupt_files(tmp.path()).is_empty()); + + #[cfg(unix)] + { + std::fs::write(&wallet, gzip_garbage()).unwrap(); + chmod(&wallet, 0o000); + if std::fs::File::open(&wallet).is_err() { // not when the tests run as root + let e = read_err(&config); + chmod(&wallet, 0o600); + assert_eq!(e.kind(), std::io::ErrorKind::PermissionDenied, "{}", e); + assert_eq!(std::fs::read(&wallet).unwrap(), gzip_garbage(), "an unreadable wallet was replaced"); + assert_eq!(std::fs::read(&bak).unwrap(), bak_bytes); + assert!(corrupt_files(tmp.path()).is_empty()); + } + chmod(&wallet, 0o600); + } + } + + // s2 C244 test 3: a corrupt wallet with a corrupt (or no) .bak is an error that leaves the files as they were. + #[test] + pub fn test_read_from_disk_both_bad_leaves_files() { + let tmp = TempDir::new("lctest").unwrap(); + let (config, wallet, bak) = saved_pair(&tmp); + let mut main_bad = std::fs::read(&wallet).unwrap(); + main_bad.truncate(main_bad.len() / 2); + std::fs::write(&wallet, &main_bad).unwrap(); + std::fs::write(&bak, gzip_garbage()).unwrap(); + + let _ = read_err(&config); + assert_eq!(std::fs::read(&wallet).unwrap(), main_bad, "the wallet was changed"); + assert_eq!(std::fs::read(&bak).unwrap(), gzip_garbage(), "the backup was changed"); + assert!(corrupt_files(tmp.path()).is_empty()); + + std::fs::remove_file(&bak).unwrap(); + let e = read_err(&config); + assert!(matches!(e.kind(), std::io::ErrorKind::InvalidData | std::io::ErrorKind::UnexpectedEof + | std::io::ErrorKind::InvalidInput), "{:?}", e); + assert_eq!(std::fs::read(&wallet).unwrap(), main_bad); + assert!(!bak.exists()); + assert!(corrupt_files(tmp.path()).is_empty()); + } + + // X5 (s2-D12): a damaged encrypted wallet whose .bak predates the encryption (a .bak last saved by Lite <= 1.1.1; + // s1-D6 no longer writes one, so it is built by hand) is not opened: Err, both files unchanged, nothing kept aside. + #[test] + pub fn test_read_from_disk_refuses_plaintext_bak_for_encrypted_main() { + use crate::lightwallet::LightWallet; + use super::wallet_header_encrypted; + let tmp = TempDir::new("lctest").unwrap(); + let config = main_config(&tmp); + let wallet = config.get_wallet_path().into_path_buf(); + let bak = wallet.with_extension("dat.bak"); + let lc = LightClient::new_from_phrase(TEST_SEED.to_string(), &config, 0, 0, false).unwrap(); + lc.do_save().unwrap(); + let plaintext = std::fs::read(&wallet).unwrap(); + assert_eq!(plaintext[8], 0, "precondition: a plaintext wallet"); + lc.wallet.write().unwrap().encrypt("password".to_string()).unwrap(); + lc.do_save().unwrap(); + drop(lc); + + std::fs::write(&bak, &plaintext).unwrap(); + let enc = std::fs::read(&wallet).unwrap(); + assert_eq!(enc[8], 1, "precondition: an encrypted wallet"); + let damaged = enc[..enc.len() / 2].to_vec(); // the 9-byte header is intact + std::fs::write(&wallet, &damaged).unwrap(); + assert_eq!(wallet_header_encrypted(&wallet), Some(true)); + let from_bak = LightWallet::read(std::io::BufReader::new(std::fs::File::open(&bak).unwrap()), &config).unwrap(); + assert!(!from_bak.is_encrypted(), "precondition: the .bak opens, unencrypted"); + assert_eq!(from_bak.get_seed_phrase(), TEST_SEED.to_string()); + + let e = read_err(&config); + assert_eq!(e.kind(), std::io::ErrorKind::InvalidData, "{}", e); + assert!(e.to_string().contains("its backup was made before the wallet was encrypted"), "{}", e); + assert_eq!(std::fs::read(&wallet).unwrap(), damaged, "the damaged encrypted wallet was changed"); + assert_eq!(std::fs::read(&bak).unwrap(), plaintext, "the backup was changed"); + assert!(corrupt_files(tmp.path()).is_empty(), "a damaged file was moved aside"); + } + + // s2-D13: a recovery never overwrites an earlier .dat.corrupt-; it takes the next free - suffix. Any entry + // counts as taken, a dangling symlink too (unix), whose target is never created. + #[test] + pub fn test_recovery_never_overwrites_previous_corrupt_file() { + let tmp = TempDir::new("lctest").unwrap(); + let (config, wallet, _bak) = saved_pair(&tmp); + let now = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap().as_secs(); + let mut sentinels = vec![]; + for t in now..now + 10 { // the recovery may run a few seconds later + let p = wallet.with_extension(format!("dat.corrupt-{}", t)); + let s = format!("an earlier damaged wallet {}", t).into_bytes(); + std::fs::write(&p, &s).unwrap(); + sentinels.push((p, s)); + } + #[cfg(unix)] + let (want_suffix, target) = { + let target = tmp.path().join("symlink-target"); + for t in now..now + 10 { + std::os::unix::fs::symlink(&target, wallet.with_extension(format!("dat.corrupt-{}-1", t))).unwrap(); + } + ("-2", Some(target)) + }; + #[cfg(not(unix))] + let (want_suffix, target): (&str, Option) = ("-1", None); + + std::fs::write(&wallet, gzip_garbage()).unwrap(); + let (_lc, note) = LightClient::read_from_disk_reporting(&config).unwrap(); + for (p, s) in &sentinels { + assert_eq!(&std::fs::read(p).unwrap(), s, "{:?} was overwritten", p); + } + let new: Vec = corrupt_files(tmp.path()).into_iter() + .filter(|p| std::fs::symlink_metadata(p).unwrap().file_type().is_file() && !sentinels.iter().any(|(q, _)| q == p)) + .collect(); + assert_eq!(new.len(), 1, "{:?}", new); + let name = new[0].file_name().unwrap().to_string_lossy().into_owned(); + assert!(name.ends_with(want_suffix), "{}", name); + assert_eq!(std::fs::read(&new[0]).unwrap(), gzip_garbage()); + assert_eq!(note, Some(format!("recovered from backup; damaged file kept as {}", name))); + if let Some(t) = target { + assert!(!t.exists(), "a dangling symlink was followed"); + } + } + } diff --git a/third_party/silentdragonxlite/silentdragonxlite-cli/lib/src/lightwallet.rs b/third_party/silentdragonxlite/silentdragonxlite-cli/lib/src/lightwallet.rs index e8a988e..e2a3585 100644 --- a/third_party/silentdragonxlite/silentdragonxlite-cli/lib/src/lightwallet.rs +++ b/third_party/silentdragonxlite/silentdragonxlite-cli/lib/src/lightwallet.rs @@ -309,7 +309,7 @@ impl LightWallet { if version > LightWallet::serialized_version() { let e = format!("Don't know how to read wallet version {}. Do you have the latest version?", version); error!("{}", e); - return Err(io::Error::new(ErrorKind::InvalidData, e)); + return Err(io::Error::new(ErrorKind::Unsupported, e)); } println!("Reading wallet version {}", version); info!("Reading wallet version {}", version); @@ -320,7 +320,7 @@ impl LightWallet { Box::new(inp) } else { info!("Reading libflat"); - Box::new(Decoder::new(inp).unwrap()) + Box::new(Decoder::new(inp)?) }; let encrypted = if version >= 4 { @@ -431,7 +431,7 @@ impl LightWallet { let mut txid_bytes = [0u8; 32]; r.read_exact(&mut txid_bytes)?; - Ok((TxId{0: txid_bytes}, WalletTx::read(r).unwrap())) + Ok((TxId{0: txid_bytes}, WalletTx::read(r)?)) })?; let txs = txs_tuples.into_iter().collect::>(); diff --git a/third_party/silentdragonxlite/silentdragonxlite-cli/lib/src/lightwallet/data.rs b/third_party/silentdragonxlite/silentdragonxlite-cli/lib/src/lightwallet/data.rs index 72157ed..29d7b80 100644 --- a/third_party/silentdragonxlite/silentdragonxlite-cli/lib/src/lightwallet/data.rs +++ b/third_party/silentdragonxlite/silentdragonxlite-cli/lib/src/lightwallet/data.rs @@ -157,7 +157,9 @@ impl SaplingNoteData { // to recreate the note let (value, r) = read_note(&mut reader)?; // TODO: This method is in a different package, because of some fields that are private - let maybe_note = extfvk.fvk.vk.into_payment_address(diversifier, &JUBJUB).unwrap().create_note(value, r, &JUBJUB); + let maybe_note = extfvk.fvk.vk.into_payment_address(diversifier, &JUBJUB) + .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidData, "invalid diversifier"))? + .create_note(value, r, &JUBJUB); let note = match maybe_note { Some(n) => Ok(n), @@ -262,13 +264,21 @@ impl Utxo { pub fn read(mut reader: R) -> io::Result { let version = reader.read_u64::()?; - assert_eq!(version, Utxo::serialized_version()); + if version != Utxo::serialized_version() { + return Err(io::Error::new(io::ErrorKind::InvalidData, format!("Unknown Utxo version {}", version))); + } let address_len = reader.read_i32::()?; - let mut address_bytes = vec![0; address_len as usize]; - reader.read_exact(&mut address_bytes)?; - let address = String::from_utf8(address_bytes).unwrap(); - assert_eq!(address.chars().take(1).collect::>()[0], 'R'); + // C244: a negative i32 would sign-extend to a ~2^64 length. + if address_len < 0 { + return Err(io::Error::new(io::ErrorKind::InvalidData, "negative Utxo address length")); + } + let address_bytes = super::utils::read_bytes_bounded(&mut reader, address_len as u64)?; + let address = String::from_utf8(address_bytes) + .map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e.to_string()))?; + if !address.starts_with('R') { + return Err(io::Error::new(io::ErrorKind::InvalidData, "Utxo address is not a transparent address")); + } let mut txid_bytes = [0; 32]; reader.read_exact(&mut txid_bytes)?; @@ -335,15 +345,16 @@ pub struct OutgoingTxMetadata { impl OutgoingTxMetadata { pub fn read(mut reader: R) -> io::Result { let address_len = reader.read_u64::()?; - let mut address_bytes = vec![0; address_len as usize]; - reader.read_exact(&mut address_bytes)?; - let address = String::from_utf8(address_bytes).unwrap(); + let address_bytes = super::utils::read_bytes_bounded(&mut reader, address_len)?; + let address = String::from_utf8(address_bytes) + .map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e.to_string()))?; let value = reader.read_u64::()?; let mut memo_bytes = [0u8; 512]; reader.read_exact(&mut memo_bytes)?; - let memo = Memo::from_bytes(&memo_bytes).unwrap(); + let memo = Memo::from_bytes(&memo_bytes) + .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidData, "invalid memo"))?; Ok(OutgoingTxMetadata{ address, @@ -373,9 +384,9 @@ pub struct IncomingTxMetadata { impl IncomingTxMetadata { pub fn read(mut reader: R) -> io::Result { let address_len = reader.read_u64::()?; - let mut address_bytes = vec![0; address_len as usize]; - reader.read_exact(&mut address_bytes)?; - let address = String::from_utf8(address_bytes).unwrap(); + let address_bytes = super::utils::read_bytes_bounded(&mut reader, address_len)?; + let address = String::from_utf8(address_bytes) + .map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e.to_string()))?; let value = reader.read_u64::()?; let incoming_mempool = true; @@ -383,7 +394,8 @@ impl IncomingTxMetadata { let mut memo_bytes = [0u8; 512]; reader.read_exact(&mut memo_bytes)?; - let memo = Memo::from_bytes(&memo_bytes).unwrap(); + let memo = Memo::from_bytes(&memo_bytes) + .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidData, "invalid memo"))?; Ok(IncomingTxMetadata{ address, @@ -461,7 +473,10 @@ impl WalletTx { pub fn read(mut reader: R) -> io::Result { let version = reader.read_u64::()?; - assert!(version <= WalletTx::serialized_version(), "Version mismatch. Please restore with your Seed"); + if version > WalletTx::serialized_version() { + return Err(io::Error::new(io::ErrorKind::Unsupported, + "Wallet transaction version is newer than this app. Please update, or restore with your Seed")); + } let block = reader.read_i32::()?; let datetime = if version >= 4 { diff --git a/third_party/silentdragonxlite/silentdragonxlite-cli/lib/src/lightwallet/tests.rs b/third_party/silentdragonxlite/silentdragonxlite-cli/lib/src/lightwallet/tests.rs index 3d910d0..b94f59e 100644 --- a/third_party/silentdragonxlite/silentdragonxlite-cli/lib/src/lightwallet/tests.rs +++ b/third_party/silentdragonxlite/silentdragonxlite-cli/lib/src/lightwallet/tests.rs @@ -3793,3 +3793,193 @@ fn test_scan_full_tx_mixed_order_epk_no_panic() { assert!(wallet.incoming_mempool_txs.read().unwrap().contains_key(&TxId(txid))); }); } + +// C244 Fix 2: the wallet readers return Err on corrupt bytes instead of panicking, and a huge length prefix is never +// allocated (an allocation abort cannot be caught). Each case names the old panic it replaces. +#[test] +fn test_wallet_read_never_panics_on_corrupt_input() { + use std::io::{ErrorKind, Read}; + use std::panic::{catch_unwind, AssertUnwindSafe}; + use std::time::{Duration, Instant}; + use super::data::{OutgoingTxMetadata, IncomingTxMetadata, Utxo}; + use super::walletzkey::{WalletTKey, WalletZKey}; + use super::utils; + + // The reader returns, Ok or Err, without panicking, and promptly. + fn run std::io::Result>(what: &str, f: F) -> std::io::Result { + let t0 = Instant::now(); + let r = catch_unwind(AssertUnwindSafe(f)); + assert!(t0.elapsed() < Duration::from_secs(5), "{}: took {:?}", what, t0.elapsed()); + match r { + Ok(r) => r, + Err(_) => panic!("{}: the reader panicked", what), + } + } + fn kind(what: &str, r: std::io::Result) -> ErrorKind { + match r { + Ok(_) => panic!("{}: read Ok", what), + Err(e) => e.kind(), + } + } + fn find(hay: &[u8], needle: &[u8]) -> usize { + let at: Vec = hay.windows(needle.len()).enumerate().filter(|(_, w)| *w == needle).map(|(i, _)| i).collect(); + assert_eq!(at.len(), 1, "pattern found {} times", at.len()); + at[0] + } + + // A wallet with a received note, a utxo, and a spend with change (as in test_serialization). + let secp = Secp256k1::new(); + let config = get_test_config(); + let wallet = LightWallet::new(None, &config, 0, 0).unwrap(); + let mut cb1 = FakeCompactBlock::new(0, BlockHash([0; 32])); + let (nf1, txid1) = cb1.add_tx_paying(wallet.zkeys.read().unwrap()[0].extfvk.clone(), 5); + wallet.scan_block(&cb1.as_bytes()).unwrap(); + let pk = PublicKey::from_secret_key(&secp, &wallet.tkeys.read().unwrap()[0].tkey.unwrap()); + let taddr = wallet.address_from_sk(&wallet.tkeys.read().unwrap()[0].tkey.unwrap()); + let mut tx = FakeTransaction::new_with_txid(txid1); + tx.add_t_output(&pk, 20); + wallet.scan_full_tx(&tx.get_tx(), 0, 0); + let addr2 = ExtendedFullViewingKey::from(&ExtendedSpendingKey::master(&[0u8; 32])).default_address().unwrap().1; + let mut cb2 = FakeCompactBlock::new(1, cb1.hash()); + cb2.add_tx_spending((nf1, 5), wallet.zkeys.read().unwrap()[0].extfvk.clone(), addr2, 2); + wallet.scan_block(&cb2.as_bytes()).unwrap(); + assert_eq!(wallet.txs.read().unwrap()[&txid1].utxos.len(), 1); + + // A valid wallet reads, and re-writes byte for byte, as before. + let mut full = vec![]; + wallet.write(&mut full).unwrap(); + let back = run("the wallet", || LightWallet::read(&full[..], &config)).unwrap(); + let mut again = vec![]; + back.write(&mut again).unwrap(); + assert_eq!(again, full); + + // The whole-wallet prefix stride: every truncation is an Err (LW:434 WalletTx::read(r).unwrap() panicked when the + // cut fell inside a tx). The birthday is the last field, so no proper prefix reads. + let mut cuts: Vec = (0..full.len()).step_by(13).collect(); + cuts.extend(full.len().saturating_sub(64)..full.len()); + for &n in &cuts { + let what = format!("the wallet cut at {} of {}", n, full.len()); + assert!(run(&what, || LightWallet::read(&full[..n], &config)).is_err(), "{}", what); + } + + // Whole-wallet cases. The WalletTx of txid1: its txid as the map key, then its version word (DATA:464 assert). + let wtx_at = find(&full, &[&txid1.0[..], &5u64.to_le_bytes()[..]].concat()) + 32; + // The utxo: version 1, an i32 address length, the address (DATA:265/268/270/271). + let utxo_at = find(&full, &[&1u64.to_le_bytes()[..], &(taddr.len() as i32).to_le_bytes()[..], taddr.as_bytes()].concat()); + // The chain name's u64 length: the file ends with it, the name and the u64 birthday (utils::read_string). + let chain_at = full.len() - 8 - config.chain_name.len() - 8; + assert_eq!(&full[chain_at..chain_at + 8], &(config.chain_name.len() as u64).to_le_bytes()[..]); + for (what, at, bytes, want) in vec![ + ("a WalletTx of a newer version", wtx_at, 6u64.to_le_bytes().to_vec(), ErrorKind::Unsupported), + ("a Utxo of version 2", utxo_at, 2u64.to_le_bytes().to_vec(), ErrorKind::InvalidData), + ("a negative Utxo address length", utxo_at + 8, (-1i32).to_le_bytes().to_vec(), ErrorKind::InvalidData), + ("an i32::MIN Utxo address length", utxo_at + 8, i32::MIN.to_le_bytes().to_vec(), ErrorKind::InvalidData), + ("a Utxo address that is not a t-address", utxo_at + 12, b"X".to_vec(), ErrorKind::InvalidData), + ("a Utxo address that is not UTF-8", utxo_at + 12, vec![0xFF], ErrorKind::InvalidData), + ("a u64::MAX chain name length", chain_at, u64::MAX.to_le_bytes().to_vec(), ErrorKind::UnexpectedEof), + ] { + let mut f = full.clone(); + f[at..at + bytes.len()].copy_from_slice(&bytes); + assert_eq!(kind(what, run(what, || LightWallet::read(&f[..], &config))), want, "{}", what); + } + + // The record readers on crafted bytes: WalletTx (every truncation, a newer version). + let txs = wallet.txs.read().unwrap(); + let wtx = &txs[&txid1]; + let mut b = vec![]; + wtx.write(&mut b).unwrap(); + assert!(run("a WalletTx", || WalletTx::read(&b[..])).is_ok()); + for n in (0..b.len()).step_by(7) { + let what = format!("a WalletTx cut at {} of {}", n, b.len()); + assert!(run(&what, || WalletTx::read(&b[..n])).is_err(), "{}", what); + } + let mut v = b.clone(); + v[..8].copy_from_slice(&6u64.to_le_bytes()); + assert_eq!(kind("a newer WalletTx", run("a newer WalletTx", || WalletTx::read(&v[..]))), ErrorKind::Unsupported); + + // SaplingNoteData: a diversifier with no payment address (DATA:160 into_payment_address(..).unwrap()). + let nd = &wtx.notes[0]; + let mut b = vec![]; + nd.write(&mut b).unwrap(); + let mut fvk = vec![]; + nd.extfvk.write(&mut fvk).unwrap(); + let at = 8 + 8 + fvk.len(); + assert_eq!(&b[at..at + 11], &nd.diversifier.0[..]); + let bad = (0u8..=255).map(|i| [i, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10]) + .find(|d| nd.extfvk.fvk.vk.into_payment_address(zcash_primitives::primitives::Diversifier(*d), &JUBJUB).is_none()) + .unwrap(); + let mut v = b.clone(); + v[at..at + 11].copy_from_slice(&bad); + assert!(run("a note", || SaplingNoteData::read(&b[..])).is_ok()); + assert_eq!(kind("an invalid diversifier", run("an invalid diversifier", || SaplingNoteData::read(&v[..]))), + ErrorKind::InvalidData); + + // Utxo (DATA:265 version assert, DATA:268 vec![0; address_len as usize], DATA:270 from_utf8 unwrap, DATA:271 [0]). + let mut b = vec![]; + wtx.utxos[0].write(&mut b).unwrap(); + assert_eq!(run("a Utxo", || Utxo::read(&b[..])).unwrap().address, taddr); + let tail = b[12 + taddr.len()..].to_vec(); + let utxo_with = |ver: u64, len: i32, addr: &[u8]| -> Vec { + [&ver.to_le_bytes()[..], &len.to_le_bytes()[..], addr, &tail[..]].concat() + }; + for (what, bytes, want) in vec![ + ("a Utxo of version 2", utxo_with(2, taddr.len() as i32, taddr.as_bytes()), ErrorKind::InvalidData), + ("a Utxo address length of -1", utxo_with(1, -1, taddr.as_bytes()), ErrorKind::InvalidData), + ("a Utxo address length of i32::MIN", utxo_with(1, i32::MIN, taddr.as_bytes()), ErrorKind::InvalidData), + ("an empty Utxo address", utxo_with(1, 0, b""), ErrorKind::InvalidData), + ("a Utxo address that is not UTF-8", utxo_with(1, 2, &[0xC3, 0x28]), ErrorKind::InvalidData), + ("a Utxo address length of i32::MAX", utxo_with(1, i32::MAX, taddr.as_bytes()), ErrorKind::UnexpectedEof), + ] { + assert_eq!(kind(what, run(what, || Utxo::read(&bytes[..]))), want, "{}", what); + } + + // Outgoing/IncomingTxMetadata (DATA:338/376 vec![0; address_len as usize], DATA:340/378 from_utf8 unwrap). Their + // Memo::from_bytes(..).unwrap() (DATA:346/386) cannot fail on 512 bytes; it is an Err now all the same. + let om = OutgoingTxMetadata { address: "zs1test".to_string(), value: 7, memo: Memo::default() }; + let mut b = vec![]; + om.write(&mut b).unwrap(); + let meta_with = |len: u64, addr: &[u8]| -> Vec { [&len.to_le_bytes()[..], addr, &b[8 + 7..]].concat() }; + assert_eq!(run("OutgoingTxMetadata", || OutgoingTxMetadata::read(&b[..])).unwrap().address, "zs1test"); + assert_eq!(run("IncomingTxMetadata", || IncomingTxMetadata::read(&b[..])).unwrap().address, "zs1test"); + for (what, bytes, want) in vec![ + ("a u64::MAX metadata address length", meta_with(u64::MAX, b"zs1test"), ErrorKind::UnexpectedEof), + ("a metadata address that is not UTF-8", meta_with(2, &[0xC3, 0x28]), ErrorKind::InvalidData), + ] { + assert_eq!(kind(what, run(what, || OutgoingTxMetadata::read(&bytes[..]))), want, "outgoing: {}", what); + assert_eq!(kind(what, run(what, || IncomingTxMetadata::read(&bytes[..]))), want, "incoming: {}", what); + } + + // WalletTKey / WalletZKey (WZK:69/275 version asserts); the t-key's address string (utils::read_string). + let mut b = vec![]; + wallet.tkeys.read().unwrap()[0].write(&mut b).unwrap(); + assert!(run("a WalletTKey", || WalletTKey::read(&b[..])).is_ok()); + let mut v = b.clone(); + v[0] = 2; + assert_eq!(kind("a newer WalletTKey", run("a newer WalletTKey", || WalletTKey::read(&v[..]))), ErrorKind::Unsupported); + assert_eq!(&b[6..14], &(taddr.len() as u64).to_le_bytes()[..]); + let mut v = b.clone(); + v[6..14].copy_from_slice(&u64::MAX.to_le_bytes()); + assert_eq!(kind("a u64::MAX t-address length", run("a u64::MAX t-address length", || WalletTKey::read(&v[..]))), + ErrorKind::UnexpectedEof); + let mut b = vec![]; + wallet.zkeys.read().unwrap()[0].write(&mut b).unwrap(); + assert!(run("a WalletZKey", || WalletZKey::read(&b[..])).is_ok()); + let mut v = b.clone(); + v[0] = 2; + assert_eq!(kind("a newer WalletZKey", run("a newer WalletZKey", || WalletZKey::read(&v[..]))), ErrorKind::Unsupported); + + // read_bytes_bounded: exact lengths read; a short field is std's read_exact error word for word (so a truncated + // file reads exactly as it did); a u64::MAX length is never allocated. + let src = [1u8, 2, 3]; + assert_eq!(utils::read_bytes_bounded(&src[..], 3).unwrap(), vec![1, 2, 3]); + assert!(utils::read_bytes_bounded(&src[..], 0).unwrap().is_empty()); + let mut four = [0u8; 4]; + let std_err = (&src[..]).read_exact(&mut four).unwrap_err(); + let ours = run("a short field", || utils::read_bytes_bounded(&src[..], 4)).unwrap_err(); + assert_eq!((ours.kind(), ours.to_string()), (std_err.kind(), std_err.to_string())); + let huge = run("a u64::MAX field", || utils::read_bytes_bounded(&src[..], u64::MAX)).unwrap_err(); + assert_eq!(huge.kind(), ErrorKind::UnexpectedEof); + let s = [&u64::MAX.to_le_bytes()[..], &b"main"[..]].concat(); + assert_eq!(kind("a u64::MAX string length", run("a u64::MAX string length", || utils::read_string(&s[..]))), + ErrorKind::UnexpectedEof); +} diff --git a/third_party/silentdragonxlite/silentdragonxlite-cli/lib/src/lightwallet/utils.rs b/third_party/silentdragonxlite/silentdragonxlite-cli/lib/src/lightwallet/utils.rs index 5b16b8f..336bdc3 100644 --- a/third_party/silentdragonxlite/silentdragonxlite-cli/lib/src/lightwallet/utils.rs +++ b/third_party/silentdragonxlite/silentdragonxlite-cli/lib/src/lightwallet/utils.rs @@ -1,11 +1,23 @@ use std::io::{self, Read, Write}; use byteorder::{LittleEndian, ReadBytesExt, WriteBytesExt}; +// C244: read a length-prefixed field of `len` bytes without allocating `len` up front. The buffer grows only with the +// bytes really read, so a corrupt length (u64::MAX) is an UnexpectedEof instead of a capacity-overflow panic or an +// allocation abort (which catch_unwind cannot catch). No cap: every length a valid file holds reads as before. A short +// field gives std's read_exact error word for word, so a truncated file reads exactly as it did. +pub fn read_bytes_bounded(mut r: R, len: u64) -> io::Result> { + let mut buf = Vec::new(); + r.by_ref().take(len).read_to_end(&mut buf)?; + if buf.len() as u64 != len { + return Err(io::Error::new(io::ErrorKind::UnexpectedEof, "failed to fill whole buffer")); + } + Ok(buf) +} + pub fn read_string(mut reader: R) -> io::Result { // Strings are written as len + bytes let str_len = reader.read_u64::()?; - let mut str_bytes = vec![0; str_len as usize]; - reader.read_exact(&mut str_bytes)?; + let str_bytes = read_bytes_bounded(&mut reader, str_len)?; let str = String::from_utf8(str_bytes).map_err(|e| { io::Error::new(io::ErrorKind::InvalidData, e.to_string()) diff --git a/third_party/silentdragonxlite/silentdragonxlite-cli/lib/src/lightwallet/walletzkey.rs b/third_party/silentdragonxlite/silentdragonxlite-cli/lib/src/lightwallet/walletzkey.rs index 5ef42e4..cbdccef 100644 --- a/third_party/silentdragonxlite/silentdragonxlite-cli/lib/src/lightwallet/walletzkey.rs +++ b/third_party/silentdragonxlite/silentdragonxlite-cli/lib/src/lightwallet/walletzkey.rs @@ -75,7 +75,10 @@ impl WalletTKey { pub fn read(mut inp: R) -> io::Result { let version = inp.read_u8()?; - assert!(version <= Self::serialized_version()); + if version > Self::serialized_version() { + return Err(io::Error::new(ErrorKind::Unsupported, + format!("Wallet key version {} is newer than this app. Please update, or restore with your Seed", version))); + } let keytype: WalletTKeyType = match inp.read_u32::()? { 0 => Ok(WalletTKeyType::HdKey), @@ -286,7 +289,10 @@ impl WalletZKey { pub fn read(mut inp: R) -> io::Result { let version = inp.read_u8()?; - assert!(version <= Self::serialized_version()); + if version > Self::serialized_version() { + return Err(io::Error::new(ErrorKind::Unsupported, + format!("Wallet key version {} is newer than this app. Please update, or restore with your Seed", version))); + } let keytype: WalletZKeyType = match inp.read_u32::()? { 0 => Ok(WalletZKeyType::HdKey), @@ -299,7 +305,8 @@ impl WalletZKey { let extsk = Optional::read(&mut inp, |r| ExtendedSpendingKey::read(r))?; let extfvk = ExtendedFullViewingKey::read(&mut inp)?; - let zaddress = extfvk.default_address().unwrap().1; + let zaddress = extfvk.default_address() + .map_err(|_| io::Error::new(ErrorKind::InvalidData, "invalid viewing key"))?.1; let hdkey_num = Optional::read(&mut inp, |r| r.read_u32::())?;