diff --git a/src/rust/bitbox02-rust/src/bip32.rs b/src/rust/bitbox02-rust/src/bip32.rs index 60aeb0d14..b25ddf74c 100644 --- a/src/rust/bitbox02-rust/src/bip32.rs +++ b/src/rust/bitbox02-rust/src/bip32.rs @@ -54,7 +54,7 @@ impl From for Xprv { } } -#[derive(Clone)] +#[derive(PartialEq, Clone)] pub struct Xpub { xpub: pb::XPub, } diff --git a/src/rust/bitbox02-rust/src/hww/api/bitcoin.rs b/src/rust/bitbox02-rust/src/hww/api/bitcoin.rs index ece38b395..7ff1b5324 100644 --- a/src/rust/bitbox02-rust/src/hww/api/bitcoin.rs +++ b/src/rust/bitbox02-rust/src/hww/api/bitcoin.rs @@ -125,7 +125,7 @@ async fn xpub( }) .await? } - let xpub = keystore::get_xpub(keypath) + let xpub = keystore::get_xpub_twice(keypath) .or(Err(Error::InvalidInput))? .serialize_str(xpub_type)?; if display { @@ -163,7 +163,7 @@ pub fn derive_address_simple( ) .or(Err(Error::InvalidInput))?; Ok(common::Payload::from_simple( - &mut crate::xpubcache::XpubCache::new(), + &mut crate::xpubcache::XpubCache::new(crate::xpubcache::Compute::Twice), coin_params, simple_type, keypath, @@ -1083,7 +1083,7 @@ mod tests { root_fingerprint: keystore::root_fingerprint().unwrap(), keypath: KEYPATH_ACCOUNT_TESTNET.to_vec(), xpub: Some( - crate::keystore::get_xpub(KEYPATH_ACCOUNT_TESTNET) + crate::keystore::get_xpub_once(KEYPATH_ACCOUNT_TESTNET) .unwrap() .into(), ), @@ -1092,7 +1092,7 @@ mod tests { root_fingerprint: keystore::root_fingerprint().unwrap(), keypath: KEYPATH_ACCOUNT_MAINNET.to_vec(), xpub: Some( - crate::keystore::get_xpub(KEYPATH_ACCOUNT_MAINNET) + crate::keystore::get_xpub_once(KEYPATH_ACCOUNT_MAINNET) .unwrap() .into(), ), diff --git a/src/rust/bitbox02-rust/src/hww/api/bitcoin/common.rs b/src/rust/bitbox02-rust/src/hww/api/bitcoin/common.rs index 183de5bf8..882204197 100644 --- a/src/rust/bitbox02-rust/src/hww/api/bitcoin/common.rs +++ b/src/rust/bitbox02-rust/src/hww/api/bitcoin/common.rs @@ -577,7 +577,7 @@ mod tests { "sudden tenant fault inject concert weather maid people chunk youth stumble grit", "", ); - let mut xpub_cache = Bip32XpubCache::new(); + let mut xpub_cache = Bip32XpubCache::new(crate::xpubcache::Compute::Once); let coin_params = super::super::params::get(pb::BtcCoin::Btc); // p2wpkh assert_eq!( diff --git a/src/rust/bitbox02-rust/src/hww/api/bitcoin/multisig.rs b/src/rust/bitbox02-rust/src/hww/api/bitcoin/multisig.rs index 4679e50ff..898c259a3 100644 --- a/src/rust/bitbox02-rust/src/hww/api/bitcoin/multisig.rs +++ b/src/rust/bitbox02-rust/src/hww/api/bitcoin/multisig.rs @@ -270,7 +270,7 @@ pub fn validate(multisig: &Multisig, keypath: &[u32]) -> Result<(), Error> { return Err(Error::InvalidInput); } - let our_xpub = crate::keystore::get_xpub(keypath)?.serialize(None)?; + let our_xpub = crate::keystore::get_xpub_once(keypath)?.serialize(None)?; let maybe_our_xpub = bip32::Xpub::from(&multisig.xpubs[multisig.our_xpub_index as usize]).serialize(None)?; if our_xpub != maybe_our_xpub { diff --git a/src/rust/bitbox02-rust/src/hww/api/bitcoin/policies.rs b/src/rust/bitbox02-rust/src/hww/api/bitcoin/policies.rs index 419406384..a9ac61ee2 100644 --- a/src/rust/bitbox02-rust/src/hww/api/bitcoin/policies.rs +++ b/src/rust/bitbox02-rust/src/hww/api/bitcoin/policies.rs @@ -58,7 +58,7 @@ fn is_our_key(key: &pb::KeyOriginInfo, our_root_fingerprint: &[u8]) -> Result { - let our_xpub = crate::keystore::get_xpub(keypath)?.serialize(None)?; + let our_xpub = crate::keystore::get_xpub_once(keypath)?.serialize(None)?; let maybe_our_xpub = bip32::Xpub::from(xpub).serialize(None)?; Ok(our_xpub == maybe_our_xpub) } @@ -787,7 +787,7 @@ mod tests { // Creates a policy for one of our own keys at keypath. fn make_our_key(keypath: &[u32]) -> pb::KeyOriginInfo { - let our_xpub = crate::keystore::get_xpub(keypath).unwrap(); + let our_xpub = crate::keystore::get_xpub_once(keypath).unwrap(); pb::KeyOriginInfo { root_fingerprint: crate::keystore::root_fingerprint().unwrap(), keypath: keypath.to_vec(), diff --git a/src/rust/bitbox02-rust/src/hww/api/bitcoin/script_configs.rs b/src/rust/bitbox02-rust/src/hww/api/bitcoin/script_configs.rs index d5cc6eece..8c1aed050 100644 --- a/src/rust/bitbox02-rust/src/hww/api/bitcoin/script_configs.rs +++ b/src/rust/bitbox02-rust/src/hww/api/bitcoin/script_configs.rs @@ -126,7 +126,7 @@ mod tests { pb::KeyOriginInfo { root_fingerprint: crate::keystore::root_fingerprint().unwrap(), keypath: keypath.to_vec(), - xpub: Some(crate::keystore::get_xpub(keypath).unwrap().into()), + xpub: Some(crate::keystore::get_xpub_once(keypath).unwrap().into()), }, pb::KeyOriginInfo { root_fingerprint: vec![], diff --git a/src/rust/bitbox02-rust/src/hww/api/bitcoin/signtx.rs b/src/rust/bitbox02-rust/src/hww/api/bitcoin/signtx.rs index 504bf767d..1344532f6 100644 --- a/src/rust/bitbox02-rust/src/hww/api/bitcoin/signtx.rs +++ b/src/rust/bitbox02-rust/src/hww/api/bitcoin/signtx.rs @@ -24,7 +24,7 @@ use super::{bip143, bip341, common, keypath}; use crate::hal::Ui; use crate::workflow::{confirm, transaction}; -use crate::xpubcache::Bip32XpubCache; +use crate::xpubcache::{Bip32XpubCache, Compute}; use alloc::string::String; use alloc::vec::Vec; @@ -681,7 +681,7 @@ async fn _process( let validated_output_script_configs = validate_script_configs(coin_params, &request.output_script_configs)?; - let mut xpub_cache = Bip32XpubCache::new(); + let mut xpub_cache = Bip32XpubCache::new(Compute::Once); setup_xpub_cache(&mut xpub_cache, &request.script_configs); // For now we only allow one payment request with one output per transaction. In the future, @@ -1804,7 +1804,7 @@ mod tests { let multisig = pb::btc_script_config::Multisig { threshold: 1, xpubs: vec![ - crate::keystore::get_xpub(keypath).unwrap().into(), + crate::keystore::get_xpub_once(keypath).unwrap().into(), parse_xpub("xpub6ERxBysTYfQyY4USv6c6J1HNVv9hpZFN9LHVPu47Ac4rK8fLy6NnAeeAHyEsMvG4G66ay5aFZii2VM7wT3KxLKX8Q8keZPd67kRGmrD1WJj").unwrap(), ], our_xpub_index: 0, @@ -3160,7 +3160,7 @@ mod tests { pb::KeyOriginInfo { root_fingerprint: crate::keystore::root_fingerprint().unwrap(), keypath: keypath_account.to_vec(), - xpub: Some(crate::keystore::get_xpub(keypath_account).unwrap().into()), + xpub: Some(crate::keystore::get_xpub_once(keypath_account).unwrap().into()), }, pb::KeyOriginInfo { root_fingerprint: vec![], @@ -3280,7 +3280,7 @@ mod tests { pb::KeyOriginInfo { root_fingerprint: crate::keystore::root_fingerprint().unwrap(), keypath: keypath_account.to_vec(), - xpub: Some(crate::keystore::get_xpub(keypath_account).unwrap().into()), + xpub: Some(crate::keystore::get_xpub_once(keypath_account).unwrap().into()), }, pb::KeyOriginInfo { root_fingerprint: vec![], @@ -3345,7 +3345,7 @@ mod tests { pb::KeyOriginInfo { root_fingerprint: crate::keystore::root_fingerprint().unwrap(), keypath: keypath_account.to_vec(), - xpub: Some(crate::keystore::get_xpub(keypath_account).unwrap().into()), + xpub: Some(crate::keystore::get_xpub_once(keypath_account).unwrap().into()), }, ], }; @@ -3452,7 +3452,7 @@ mod tests { pb::KeyOriginInfo { root_fingerprint: crate::keystore::root_fingerprint().unwrap(), keypath: keypath_account.to_vec(), - xpub: Some(crate::keystore::get_xpub(keypath_account).unwrap().into()), + xpub: Some(crate::keystore::get_xpub_once(keypath_account).unwrap().into()), }, pb::KeyOriginInfo { root_fingerprint: vec![], @@ -3504,7 +3504,7 @@ mod tests { pb::KeyOriginInfo { root_fingerprint: crate::keystore::root_fingerprint().unwrap(), keypath: keypath_account.to_vec(), - xpub: Some(crate::keystore::get_xpub(keypath_account).unwrap().into()), + xpub: Some(crate::keystore::get_xpub_once(keypath_account).unwrap().into()), }, pb::KeyOriginInfo { root_fingerprint: vec![], @@ -3553,7 +3553,7 @@ mod tests { pb::KeyOriginInfo { root_fingerprint: crate::keystore::root_fingerprint().unwrap(), keypath: keypath_account.to_vec(), - xpub: Some(crate::keystore::get_xpub(keypath_account).unwrap().into()), + xpub: Some(crate::keystore::get_xpub_once(keypath_account).unwrap().into()), }, pb::KeyOriginInfo { root_fingerprint: vec![], diff --git a/src/rust/bitbox02-rust/src/hww/api/electrum.rs b/src/rust/bitbox02-rust/src/hww/api/electrum.rs index e4cd42912..e2caffb29 100644 --- a/src/rust/bitbox02-rust/src/hww/api/electrum.rs +++ b/src/rust/bitbox02-rust/src/hww/api/electrum.rs @@ -39,7 +39,7 @@ pub async fn process( { return Err(Error::InvalidInput); } - let xpub = keystore::get_xpub(keypath) + let xpub = keystore::get_xpub_twice(keypath) .or(Err(Error::InvalidInput))? .serialize_str(bip32::XPubType::Xpub)?; diff --git a/src/rust/bitbox02-rust/src/hww/api/ethereum/pubrequest.rs b/src/rust/bitbox02-rust/src/hww/api/ethereum/pubrequest.rs index d5c90bcfd..da8c7fa0d 100644 --- a/src/rust/bitbox02-rust/src/hww/api/ethereum/pubrequest.rs +++ b/src/rust/bitbox02-rust/src/hww/api/ethereum/pubrequest.rs @@ -45,7 +45,7 @@ async fn process_address( if !super::keypath::is_valid_keypath_address(&request.keypath) { return Err(Error::InvalidInput); } - let pubkey = crate::keystore::get_xpub(&request.keypath) + let pubkey = crate::keystore::get_xpub_twice(&request.keypath) .or(Err(Error::InvalidInput))? .pubkey_uncompressed()?; let address = super::address::from_pubkey(&pubkey); @@ -79,7 +79,7 @@ fn process_xpub(request: &pb::EthPubRequest) -> Result { if !super::keypath::is_valid_keypath_xpub(&request.keypath) { return Err(Error::InvalidInput); } - let xpub = keystore::get_xpub(&request.keypath) + let xpub = keystore::get_xpub_twice(&request.keypath) .or(Err(Error::InvalidInput))? .serialize_str(bip32::XPubType::Xpub)?; diff --git a/src/rust/bitbox02-rust/src/keystore.rs b/src/rust/bitbox02-rust/src/keystore.rs index 37d89beee..293230319 100644 --- a/src/rust/bitbox02-rust/src/keystore.rs +++ b/src/rust/bitbox02-rust/src/keystore.rs @@ -45,15 +45,6 @@ fn get_xprv(keypath: &[u32]) -> Result { .into()) } -fn get_xprv_twice(keypath: &[u32]) -> Result { - let xprv = get_xprv(keypath)?; - if xprv == get_xprv(keypath)? { - Ok(xprv) - } else { - Err(()) - } -} - /// Get the private key at the keypath. pub fn secp256k1_get_private_key(keypath: &[u32]) -> Result>, ()> { let xprv = get_xprv(keypath)?; @@ -75,19 +66,35 @@ pub fn secp256k1_get_private_key_twice(keypath: &[u32]) -> Result Result { - let xpriv = get_xprv_twice(keypath)?; +pub fn get_xpub_once(keypath: &[u32]) -> Result { + let xpriv = get_xprv(keypath)?; let secp = bitcoin::secp256k1::Secp256k1::new(); let xpub = bitcoin::bip32::Xpub::from_priv(&secp, &xpriv.xprv); Ok(bip32::Xpub::from(xpub)) } +/// Can be used only if the keystore is unlocked. Returns the derived xpub, +/// using bip32 derivation. Derivation is done from the xprv master, so hardened +/// derivation is allowed. +pub fn get_xpub_twice(keypath: &[u32]) -> Result { + let res1 = get_xpub_once(keypath)?; + let res2 = get_xpub_once(keypath)?; + if res1 != res2 { + return Err(()); + } + Ok(res1) +} + /// Returns fingerprint of the root public key at m/, which are the first four bytes of its hash160 /// according to: /// https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#serialization-format pub fn root_fingerprint() -> Result, ()> { - Ok(get_xpub(&[])?.pubkey_hash160().get(..4).ok_or(())?.to_vec()) + Ok(get_xpub_twice(&[])? + .pubkey_hash160() + .get(..4) + .ok_or(())? + .to_vec()) } fn bip85_entropy(keypath: &[u32]) -> Result>, ()> { @@ -203,13 +210,13 @@ mod tests { } #[test] - fn test_get_xpub() { + fn test_get_xpub_twice() { let keypath = &[44 + HARDENED, 0 + HARDENED, 0 + HARDENED]; // Also test with unhardened and non-zero elements. let keypath_5 = &[44 + HARDENED, 1 + HARDENED, 10 + HARDENED, 1, 100]; keystore::lock(); - assert!(get_xpub(keypath).is_err()); + assert!(get_xpub_twice(keypath).is_err()); // 24 words mock_unlocked_using_mnemonic( @@ -217,15 +224,15 @@ mod tests { "", ); assert_eq!( - get_xpub(&[]).unwrap().serialize_str(bip32::XPubType::Xpub).unwrap(), + get_xpub_twice(&[]).unwrap().serialize_str(bip32::XPubType::Xpub).unwrap(), "xpub661MyMwAqRbcEhX8d9WJh78SZrxusAzWFoykz4n5CF75uYRzixw5FZPUSoWyhaaJ1bpiPFdzdHSQqJN38PcTkyrLmxT4J2JDYfoGJQ4ioE2", ); assert_eq!( - get_xpub(keypath).unwrap().serialize_str(bip32::XPubType::Xpub).unwrap(), + get_xpub_twice(keypath).unwrap().serialize_str(bip32::XPubType::Xpub).unwrap(), "xpub6Cj6NNCGj2CRPHvkuEG1rbW3nrNCAnLjaoTg1P67FCGoahSsbg9WQ7YaMEEP83QDxt2kZ3hTPAPpGdyEZcfAC1C75HfR66UbjpAb39f4PnG", ); assert_eq!( - get_xpub(keypath_5).unwrap().serialize_str(bip32::XPubType::Xpub).unwrap(), + get_xpub_twice(keypath_5).unwrap().serialize_str(bip32::XPubType::Xpub).unwrap(), "xpub6HHn1zdtf1RjePopiTV5nxf8jY2xwbJicTQ91jV4cUJZ5EnbvXyBGDhqWt8B9JxxBt9vExi4pdWzrbrM43qSFs747VCGmSy2DPWAhg9MkUg", ); @@ -235,7 +242,7 @@ mod tests { "", ); assert_eq!( - get_xpub(keypath).unwrap().serialize_str(bip32::XPubType::Xpub).unwrap(), + get_xpub_twice(keypath).unwrap().serialize_str(bip32::XPubType::Xpub).unwrap(), "xpub6C7fKxGtTzEVxCC22U2VHx4GpaVy77DzU6KdZ1CLuHgoUGviBMWDc62uoQVxqcRa5RQbMPnffjpwxve18BG81VJhJDXnSpRe5NGKwVpXiAb", ); @@ -245,7 +252,7 @@ mod tests { "", ); assert_eq!( - get_xpub(keypath).unwrap().serialize_str(bip32::XPubType::Xpub).unwrap(), + get_xpub_twice(keypath).unwrap().serialize_str(bip32::XPubType::Xpub).unwrap(), "xpub6DLvpzjKpJ8k4xYrWYPmZQkUe9dkG1eRig2v6Jz4iYgo8hcpHWx87gGoCGDaB2cHFZ3ExUfe1jDiMu7Ch6gA4ULCBhvwZj29mHCPYSux3YV", ) } @@ -401,7 +408,7 @@ mod tests { test.expected_mnemonic, ); let keypath = &[44 + HARDENED, 0 + HARDENED, 0 + HARDENED]; - let xpub = get_xpub(keypath).unwrap(); + let xpub = get_xpub_once(keypath).unwrap(); assert_eq!( xpub.serialize_str(crate::bip32::XPubType::Xpub).unwrap(), test.expected_xpub, diff --git a/src/rust/bitbox02-rust/src/xpubcache.rs b/src/rust/bitbox02-rust/src/xpubcache.rs index 5f1cf93ca..97cac0f7a 100644 --- a/src/rust/bitbox02-rust/src/xpubcache.rs +++ b/src/rust/bitbox02-rust/src/xpubcache.rs @@ -17,17 +17,27 @@ use super::keystore; use crate::bip32; use alloc::vec::Vec; +#[derive(Copy, Clone)] +pub enum Compute { + /// The xpubs are derived once. Use for non-critical operations like signing a transaction, + /// where a compute error will lead to an invalid signature only. + Once, + /// The xpubs are derive twice, to mitigate the risk of bitflips or similar compute corruption. + /// Used for critical operations, like delivering xpubs to the host. + Twice, +} + pub trait Xpub: Sized { /// Derives a child xpub using the provided keypath. - fn derive(&self, keypath: &[u32]) -> Result; + fn derive(&self, keypath: &[u32], compute: Compute) -> Result; /// Derives an xpub from the root xpub using the provided keypath. - fn from_keypath(keypath: &[u32]) -> Result; + fn from_keypath(keypath: &[u32], compute: Compute) -> Result; } /// Implements a cache for xpubs. Cached intermediate xpubs are used to derive child xpubs. /// -/// The cache must be configured using `cache_keypath()`, otherwise no caching occurs. The reason +/// The cache must be configured using `add_keypath()`, otherwise no caching occurs. The reason /// for this is that automatic caching is harder to get right and reason about, e.g. in a BTC tx, we /// shouldn't cache xpubs at the address level (e.g. m/84/0'/0'/0/0), as they don't repeat and there /// can be many of them. @@ -37,13 +47,15 @@ pub struct XpubCache { // Cached xpubs. First tuple element is the keypath for which the xpub was cached, the second // element is the cached xpub. xpubs: Vec<(Vec, X)>, + compute: Compute, } impl XpubCache { - pub fn new() -> Self { + pub fn new(compute: Compute) -> Self { XpubCache { keypaths: Vec::new(), xpubs: Vec::new(), + compute, } } @@ -71,9 +83,9 @@ impl XpubCache { // from an xpub (hardened elements require the xprv). const UNHARDENED_LAST: u32 = util::bip32::HARDENED - 1; let xpub = if let [prefix @ .., last @ 0..=UNHARDENED_LAST] = keypath { - self.get_xpub(prefix)?.derive(&[*last])? + self.get_xpub(prefix)?.derive(&[*last], self.compute)? } else { - X::from_keypath(keypath)? + X::from_keypath(keypath, self.compute)? }; self.xpubs.push((keypath.to_vec(), xpub.clone())); Ok(xpub) @@ -95,19 +107,32 @@ impl XpubCache { .max_by_key(|(kp, _)| kp.len()); if let Some((cached_prefix, suffix)) = search_result { let xpub = self.cache_get_set(&cached_prefix.clone())?; - return xpub.derive(suffix); + return xpub.derive(suffix, self.compute); } - X::from_keypath(keypath) + X::from_keypath(keypath, self.compute) } } impl Xpub for bip32::Xpub { - fn derive(&self, keypath: &[u32]) -> Result { - bip32::Xpub::derive(self, keypath) + fn derive(&self, keypath: &[u32], compute: Compute) -> Result { + match compute { + Compute::Once => bip32::Xpub::derive(self, keypath), + Compute::Twice => { + let res1 = bip32::Xpub::derive(self, keypath)?; + let res2 = bip32::Xpub::derive(self, keypath)?; + if res1 != res2 { + return Err(()); + } + Ok(res2) + } + } } - fn from_keypath(keypath: &[u32]) -> Result { - keystore::get_xpub(keypath) + fn from_keypath(keypath: &[u32], compute: Compute) -> Result { + match compute { + Compute::Once => keystore::get_xpub_once(keypath), + Compute::Twice => keystore::get_xpub_twice(keypath), + } } } @@ -133,7 +158,7 @@ mod tests { bitbox02::testing::UnsafeSyncRefCell::new(0); impl Xpub for MockXpub { - fn derive(&self, keypath: &[u32]) -> Result { + fn derive(&self, keypath: &[u32], _compute: Compute) -> Result { let mut kp = Vec::new(); kp.extend_from_slice(&self.0); kp.extend_from_slice(keypath); @@ -141,7 +166,7 @@ mod tests { Ok(MockXpub(kp)) } - fn from_keypath(keypath: &[u32]) -> Result { + fn from_keypath(keypath: &[u32], _compute: Compute) -> Result { *ROOT_DERIVATIONS.borrow_mut() += 1; Ok(MockXpub(keypath.to_vec())) } @@ -149,7 +174,7 @@ mod tests { type MockCache = XpubCache; - let mut cache = MockCache::new(); + let mut cache = MockCache::new(crate::xpubcache::Compute::Once); assert_eq!(cache.get_xpub(&[]).unwrap().0.as_slice(), &[]); assert_eq!(*CHILD_DERIVATIONS.borrow(), 0u32); @@ -210,7 +235,7 @@ mod tests { #[test] fn test_bip32_xpub_cache() { - let mut cache = Bip32XpubCache::new(); + let mut cache = Bip32XpubCache::new(crate::xpubcache::Compute::Twice); cache.add_keypath(&[84 + HARDENED, 0 + HARDENED, 0 + HARDENED, 1]); cache.add_keypath(&[84 + HARDENED, 0 + HARDENED, 0 + HARDENED]);