fix(lite-backend): validate WIFs, refuse key import while encrypted
import_taddr decoded a WIF with bs58::decode(..).unwrap(), a [2..66] slice of its hex and SecretKey::from_slice(..).unwrap(), all under the wallet write guard that do_import_tk holds. A z-key, a typo or any short string panicked there and poisoned the wallet until restart, and the checksum was never verified, so a mistyped WIF that was still valid base58 imported a different key. A private key imported into an encrypted wallet was stored without an enc_key. The import then failed in do_save's lock(), which had already zeroed the seed while the wallet stayed "unlocked": seed export showed the all-zero-entropy phrase. - decode_taddr_wif replaces the old decode: Base58Check with the checksum verified, version 0xBC (the config prefix) or 0x80, and the 0x01 compressed flag. It decodes into a buffer it owns and wipes it with sodiumoxide::utils::memzero on every path, including a failed checksum. Errors are fixed strings that never contain the key. - import_taddr and add_imported_sk refuse while the wallet is encrypted, as add_taddr and add_zaddr already do. - do_import_tk and do_import_sk check is_encrypted() before the "locked" check, so a locked wallet does not send the user to unlock and retry. Flagged behaviour changes: - Base58Check, version 0xBC/0x80 and the compressed flag are required; - uncompressed and foreign-network WIFs are rejected with fixed messages (an uncompressed key used to import under the compressed address, where its funds are not); - no panic and no poisoned lock; - private-key import (t and z) is refused while the wallet is encrypted, with a reason that says to remove encryption, import, then encrypt again; viewing keys still import; - the key is never echoed. No ABI, symbol or wallet format change. Tests: garbage, z-keys, bad checksums, a transposed or flipped character, a zero scalar, a wrong flag, uncompressed, 0x05 and 0xEF WIFs are rejected with their message and leave tkeys usable; the wallet's own export, a 0x80 WIF and the published key-1 vector still import; the encrypted refusal in the wallet and the client, locked and unlocked, leaves lock, save and seed export intact; a viewing key still imports into an encrypted wallet; a bad timport leaves the wallet lock unpoisoned. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -1120,6 +1120,13 @@ impl LightClient {
|
||||
|
||||
/// Import a new private key
|
||||
pub fn do_import_tk(&self, sk: String, birthday: u64) -> Result<JsonValue, String> {
|
||||
// Checked before "locked": unlocking would not help (s1-D1). No "Error: " prefix here; the console
|
||||
// import/timport commands add it.
|
||||
if self.wallet.read().unwrap().is_encrypted() {
|
||||
let e = "Can't import a private key while the wallet is encrypted - remove encryption, import, then encrypt again".to_string();
|
||||
error!("{}", e);
|
||||
return Err(e);
|
||||
}
|
||||
if !self.wallet.read().unwrap().is_unlocked_for_spending() {
|
||||
error!("Wallet is locked");
|
||||
return Err("Wallet is locked".to_string());
|
||||
@@ -1232,6 +1239,13 @@ pub fn start_mempool_monitor(lc: Arc<LightClient>) -> Result<(), String> {
|
||||
|
||||
/// Import a new private key
|
||||
pub fn do_import_sk(&self, sk: String, birthday: u64) -> Result<JsonValue, String> {
|
||||
// Checked before "locked": unlocking would not help (s1-D1). No "Error: " prefix here; the console
|
||||
// import/timport commands add it.
|
||||
if self.wallet.read().unwrap().is_encrypted() {
|
||||
let e = "Can't import a private key while the wallet is encrypted - remove encryption, import, then encrypt again".to_string();
|
||||
error!("{}", e);
|
||||
return Err(e);
|
||||
}
|
||||
if !self.wallet.read().unwrap().is_unlocked_for_spending() {
|
||||
error!("Wallet is locked");
|
||||
return Err("Wallet is locked".to_string());
|
||||
@@ -2010,4 +2024,68 @@ pub fn test_bad_import() {
|
||||
w.join().unwrap();
|
||||
}
|
||||
|
||||
// C251 (s2): a WIF that cannot be decoded returns Err and leaves the wallet lock usable. Before, import_taddr
|
||||
// panicked under the wallet write guard (bs58 unwrap / hex slice), which poisoned the wallet until restart.
|
||||
#[test]
|
||||
pub fn test_bad_import_does_not_poison() {
|
||||
use std::panic::{catch_unwind, AssertUnwindSafe};
|
||||
let tmp = TempDir::new("lctest").unwrap();
|
||||
let lc = LightClient::new_from_phrase(TEST_SEED.to_string(), &main_config(&tmp), 0, 0, false).unwrap();
|
||||
let before = lc.do_address();
|
||||
|
||||
for bad in &["zs1notawif", "secret-extended-key-main1qqqqqqqqqq", "", "1"] {
|
||||
let r = catch_unwind(AssertUnwindSafe(|| lc.do_import_tk(bad.to_string(), 0)));
|
||||
let e = match r {
|
||||
Ok(Err(e)) => e,
|
||||
Ok(Ok(a)) => panic!("{:?} imported as {}", bad, a),
|
||||
Err(_) => panic!("do_import_tk({:?}) panicked", bad),
|
||||
};
|
||||
assert!(e.contains("Couldn't decode the transparent private key"), "{:?}: {}", bad, e);
|
||||
assert!(bad.len() < 2 || !e.contains(bad), "the error echoes the key: {}", e);
|
||||
// separate statements: a read guard alive in the same expression would deadlock the write()
|
||||
assert!(lc.wallet.read().is_ok(), "the wallet lock is poisoned");
|
||||
assert!(lc.wallet.write().is_ok(), "the wallet lock is poisoned");
|
||||
}
|
||||
assert_eq!(lc.do_address(), before);
|
||||
}
|
||||
|
||||
// C252 (s1 test 8): a private-key import into an encrypted wallet is refused with the "encrypted" reason
|
||||
// whether the wallet is locked or unlocked (never "Wallet is locked", which would send the user to unlock and
|
||||
// retry), and it leaves the wallet able to lock, save and show its real seed. Viewing keys still import.
|
||||
#[test]
|
||||
pub fn test_encrypted_import_is_clean() {
|
||||
let tmp = TempDir::new("lctest").unwrap();
|
||||
let lc = LightClient::new_from_phrase(TEST_SEED.to_string(), &main_config(&tmp), 0, 0, false).unwrap();
|
||||
let tmp2 = TempDir::new("lctest").unwrap();
|
||||
let other = LightClient::new(&main_config(&tmp2), 0).unwrap();
|
||||
let (wif, sk, vk) = {
|
||||
let w = other.wallet.read().unwrap();
|
||||
let z = w.get_z_private_keys()[0].clone();
|
||||
(w.get_t_secret_keys()[0].1.clone(), z.1, z.2)
|
||||
};
|
||||
|
||||
// encrypt() also locks the wallet
|
||||
lc.wallet.write().unwrap().encrypt("password".to_string()).unwrap();
|
||||
for &unlocked in &[false, true] {
|
||||
if unlocked {
|
||||
lc.wallet.write().unwrap().unlock("password".to_string()).unwrap();
|
||||
}
|
||||
for r in vec![lc.do_import_sk(sk.clone(), 0), lc.do_import_tk(wif.clone(), 0), lc.do_import_key(sk.clone(), 0)] {
|
||||
let e = r.err().expect("a private-key import must fail while the wallet is encrypted");
|
||||
assert!(e.starts_with("Can't import a private key while the wallet is encrypted"), "{}", e);
|
||||
assert!(!e.contains("locked") && !e.contains("decode"), "{}", e);
|
||||
assert!(!e.contains(&wif) && !e.contains(&sk), "the error echoes the key");
|
||||
}
|
||||
assert_eq!(lc.wallet.read().unwrap().is_unlocked_for_spending(), unlocked);
|
||||
}
|
||||
|
||||
// A viewing key still imports while encrypted (its save locks the wallet again).
|
||||
assert!(lc.do_import_vk(vk, 0).is_ok());
|
||||
assert!(!lc.wallet.read().unwrap().is_unlocked_for_spending());
|
||||
|
||||
assert!(lc.do_save().is_ok());
|
||||
lc.wallet.write().unwrap().unlock("password".to_string()).unwrap();
|
||||
assert_eq!(lc.do_seed_phrase().unwrap()["seed"], TEST_SEED.to_string());
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user