Skip to content

Commit 8cdc363

Browse files
benmabenma
committed
xpubcache: allow choosing if xpub computation should be repeated
End-goal: reduce the number of secure chip ops when signing a BTC transaction, to reduce the chance of going over the Optiga chip's "rate limit", which induces throttling. By default keystore::get_xpub computed the xpub twice, to mitigate potential bitflips, which could be bad when delivering the wrong xpub (or derivatives) to the host. When signing a transaction however, one does not need the extra protection - if there is a bit flip, the resulting signature will be invalid. This commit reduces the number of secure chip ops needed when the bitflip mitigation is not required. The existing method `get_xpub` was renamed so the compiler can tell us all the instances where we need to decide between one or the other.
1 parent a6f1c2e commit 8cdc363

File tree

11 files changed

+85
-57
lines changed

11 files changed

+85
-57
lines changed

‎src/rust/bitbox02-rust/src/bip32.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ impl From<bitcoin::bip32::Xpriv> for Xprv {
5454
}
5555
}
5656

57-
#[derive(Clone)]
57+
#[derive(PartialEq, Clone)]
5858
pub struct Xpub {
5959
xpub: pb::XPub,
6060
}

‎src/rust/bitbox02-rust/src/hww/api/bitcoin.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ async fn xpub(
125125
})
126126
.await?
127127
}
128-
let xpub = keystore::get_xpub(keypath)
128+
let xpub = keystore::get_xpub_twice(keypath)
129129
.or(Err(Error::InvalidInput))?
130130
.serialize_str(xpub_type)?;
131131
if display {
@@ -163,7 +163,7 @@ pub fn derive_address_simple(
163163
)
164164
.or(Err(Error::InvalidInput))?;
165165
Ok(common::Payload::from_simple(
166-
&mut crate::xpubcache::XpubCache::new(),
166+
&mut crate::xpubcache::XpubCache::new(crate::xpubcache::Compute::Twice),
167167
coin_params,
168168
simple_type,
169169
keypath,
@@ -1083,7 +1083,7 @@ mod tests {
10831083
root_fingerprint: keystore::root_fingerprint().unwrap(),
10841084
keypath: KEYPATH_ACCOUNT_TESTNET.to_vec(),
10851085
xpub: Some(
1086-
crate::keystore::get_xpub(KEYPATH_ACCOUNT_TESTNET)
1086+
crate::keystore::get_xpub_once(KEYPATH_ACCOUNT_TESTNET)
10871087
.unwrap()
10881088
.into(),
10891089
),
@@ -1092,7 +1092,7 @@ mod tests {
10921092
root_fingerprint: keystore::root_fingerprint().unwrap(),
10931093
keypath: KEYPATH_ACCOUNT_MAINNET.to_vec(),
10941094
xpub: Some(
1095-
crate::keystore::get_xpub(KEYPATH_ACCOUNT_MAINNET)
1095+
crate::keystore::get_xpub_once(KEYPATH_ACCOUNT_MAINNET)
10961096
.unwrap()
10971097
.into(),
10981098
),

‎src/rust/bitbox02-rust/src/hww/api/bitcoin/common.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -577,7 +577,7 @@ mod tests {
577577
"sudden tenant fault inject concert weather maid people chunk youth stumble grit",
578578
"",
579579
);
580-
let mut xpub_cache = Bip32XpubCache::new();
580+
let mut xpub_cache = Bip32XpubCache::new(crate::xpubcache::Compute::Once);
581581
let coin_params = super::super::params::get(pb::BtcCoin::Btc);
582582
// p2wpkh
583583
assert_eq!(

‎src/rust/bitbox02-rust/src/hww/api/bitcoin/multisig.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,7 @@ pub fn validate(multisig: &Multisig, keypath: &[u32]) -> Result<(), Error> {
270270
return Err(Error::InvalidInput);
271271
}
272272

273-
let our_xpub = crate::keystore::get_xpub(keypath)?.serialize(None)?;
273+
let our_xpub = crate::keystore::get_xpub_once(keypath)?.serialize(None)?;
274274
let maybe_our_xpub =
275275
bip32::Xpub::from(&multisig.xpubs[multisig.our_xpub_index as usize]).serialize(None)?;
276276
if our_xpub != maybe_our_xpub {

‎src/rust/bitbox02-rust/src/hww/api/bitcoin/policies.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ fn is_our_key(key: &pb::KeyOriginInfo, our_root_fingerprint: &[u8]) -> Result<bo
5858
xpub: Some(xpub),
5959
..
6060
} if root_fingerprint.as_slice() == our_root_fingerprint => {
61-
let our_xpub = crate::keystore::get_xpub(keypath)?.serialize(None)?;
61+
let our_xpub = crate::keystore::get_xpub_once(keypath)?.serialize(None)?;
6262
let maybe_our_xpub = bip32::Xpub::from(xpub).serialize(None)?;
6363
Ok(our_xpub == maybe_our_xpub)
6464
}
@@ -787,7 +787,7 @@ mod tests {
787787

788788
// Creates a policy for one of our own keys at keypath.
789789
fn make_our_key(keypath: &[u32]) -> pb::KeyOriginInfo {
790-
let our_xpub = crate::keystore::get_xpub(keypath).unwrap();
790+
let our_xpub = crate::keystore::get_xpub_once(keypath).unwrap();
791791
pb::KeyOriginInfo {
792792
root_fingerprint: crate::keystore::root_fingerprint().unwrap(),
793793
keypath: keypath.to_vec(),

‎src/rust/bitbox02-rust/src/hww/api/bitcoin/script_configs.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ mod tests {
126126
pb::KeyOriginInfo {
127127
root_fingerprint: crate::keystore::root_fingerprint().unwrap(),
128128
keypath: keypath.to_vec(),
129-
xpub: Some(crate::keystore::get_xpub(keypath).unwrap().into()),
129+
xpub: Some(crate::keystore::get_xpub_once(keypath).unwrap().into()),
130130
},
131131
pb::KeyOriginInfo {
132132
root_fingerprint: vec![],

‎src/rust/bitbox02-rust/src/hww/api/bitcoin/signtx.rs

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ use super::{bip143, bip341, common, keypath};
2424

2525
use crate::hal::Ui;
2626
use crate::workflow::{confirm, transaction};
27-
use crate::xpubcache::Bip32XpubCache;
27+
use crate::xpubcache::{Bip32XpubCache, Compute};
2828

2929
use alloc::string::String;
3030
use alloc::vec::Vec;
@@ -681,7 +681,7 @@ async fn _process(
681681
let validated_output_script_configs =
682682
validate_script_configs(coin_params, &request.output_script_configs)?;
683683

684-
let mut xpub_cache = Bip32XpubCache::new();
684+
let mut xpub_cache = Bip32XpubCache::new(Compute::Once);
685685
setup_xpub_cache(&mut xpub_cache, &request.script_configs);
686686

687687
// For now we only allow one payment request with one output per transaction. In the future,
@@ -1799,7 +1799,7 @@ mod tests {
17991799
let multisig = pb::btc_script_config::Multisig {
18001800
threshold: 1,
18011801
xpubs: vec![
1802-
crate::keystore::get_xpub(keypath).unwrap().into(),
1802+
crate::keystore::get_xpub_once(keypath).unwrap().into(),
18031803
parse_xpub("xpub6ERxBysTYfQyY4USv6c6J1HNVv9hpZFN9LHVPu47Ac4rK8fLy6NnAeeAHyEsMvG4G66ay5aFZii2VM7wT3KxLKX8Q8keZPd67kRGmrD1WJj").unwrap(),
18041804
],
18051805
our_xpub_index: 0,
@@ -3155,7 +3155,7 @@ mod tests {
31553155
pb::KeyOriginInfo {
31563156
root_fingerprint: crate::keystore::root_fingerprint().unwrap(),
31573157
keypath: keypath_account.to_vec(),
3158-
xpub: Some(crate::keystore::get_xpub(keypath_account).unwrap().into()),
3158+
xpub: Some(crate::keystore::get_xpub_once(keypath_account).unwrap().into()),
31593159
},
31603160
pb::KeyOriginInfo {
31613161
root_fingerprint: vec![],
@@ -3275,7 +3275,7 @@ mod tests {
32753275
pb::KeyOriginInfo {
32763276
root_fingerprint: crate::keystore::root_fingerprint().unwrap(),
32773277
keypath: keypath_account.to_vec(),
3278-
xpub: Some(crate::keystore::get_xpub(keypath_account).unwrap().into()),
3278+
xpub: Some(crate::keystore::get_xpub_once(keypath_account).unwrap().into()),
32793279
},
32803280
pb::KeyOriginInfo {
32813281
root_fingerprint: vec![],
@@ -3340,7 +3340,7 @@ mod tests {
33403340
pb::KeyOriginInfo {
33413341
root_fingerprint: crate::keystore::root_fingerprint().unwrap(),
33423342
keypath: keypath_account.to_vec(),
3343-
xpub: Some(crate::keystore::get_xpub(keypath_account).unwrap().into()),
3343+
xpub: Some(crate::keystore::get_xpub_once(keypath_account).unwrap().into()),
33443344
},
33453345
],
33463346
};
@@ -3447,7 +3447,7 @@ mod tests {
34473447
pb::KeyOriginInfo {
34483448
root_fingerprint: crate::keystore::root_fingerprint().unwrap(),
34493449
keypath: keypath_account.to_vec(),
3450-
xpub: Some(crate::keystore::get_xpub(keypath_account).unwrap().into()),
3450+
xpub: Some(crate::keystore::get_xpub_once(keypath_account).unwrap().into()),
34513451
},
34523452
pb::KeyOriginInfo {
34533453
root_fingerprint: vec![],
@@ -3499,7 +3499,7 @@ mod tests {
34993499
pb::KeyOriginInfo {
35003500
root_fingerprint: crate::keystore::root_fingerprint().unwrap(),
35013501
keypath: keypath_account.to_vec(),
3502-
xpub: Some(crate::keystore::get_xpub(keypath_account).unwrap().into()),
3502+
xpub: Some(crate::keystore::get_xpub_once(keypath_account).unwrap().into()),
35033503
},
35043504
pb::KeyOriginInfo {
35053505
root_fingerprint: vec![],
@@ -3548,7 +3548,7 @@ mod tests {
35483548
pb::KeyOriginInfo {
35493549
root_fingerprint: crate::keystore::root_fingerprint().unwrap(),
35503550
keypath: keypath_account.to_vec(),
3551-
xpub: Some(crate::keystore::get_xpub(keypath_account).unwrap().into()),
3551+
xpub: Some(crate::keystore::get_xpub_once(keypath_account).unwrap().into()),
35523552
},
35533553
pb::KeyOriginInfo {
35543554
root_fingerprint: vec![],

‎src/rust/bitbox02-rust/src/hww/api/electrum.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ pub async fn process(
3939
{
4040
return Err(Error::InvalidInput);
4141
}
42-
let xpub = keystore::get_xpub(keypath)
42+
let xpub = keystore::get_xpub_twice(keypath)
4343
.or(Err(Error::InvalidInput))?
4444
.serialize_str(bip32::XPubType::Xpub)?;
4545

‎src/rust/bitbox02-rust/src/hww/api/ethereum/pubrequest.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ async fn process_address(
4545
if !super::keypath::is_valid_keypath_address(&request.keypath) {
4646
return Err(Error::InvalidInput);
4747
}
48-
let pubkey = crate::keystore::get_xpub(&request.keypath)
48+
let pubkey = crate::keystore::get_xpub_twice(&request.keypath)
4949
.or(Err(Error::InvalidInput))?
5050
.pubkey_uncompressed()?;
5151
let address = super::address::from_pubkey(&pubkey);
@@ -79,7 +79,7 @@ fn process_xpub(request: &pb::EthPubRequest) -> Result<Response, Error> {
7979
if !super::keypath::is_valid_keypath_xpub(&request.keypath) {
8080
return Err(Error::InvalidInput);
8181
}
82-
let xpub = keystore::get_xpub(&request.keypath)
82+
let xpub = keystore::get_xpub_twice(&request.keypath)
8383
.or(Err(Error::InvalidInput))?
8484
.serialize_str(bip32::XPubType::Xpub)?;
8585

‎src/rust/bitbox02-rust/src/keystore.rs

Lines changed: 27 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -45,15 +45,6 @@ fn get_xprv(keypath: &[u32]) -> Result<bip32::Xprv, ()> {
4545
.into())
4646
}
4747

48-
fn get_xprv_twice(keypath: &[u32]) -> Result<bip32::Xprv, ()> {
49-
let xprv = get_xprv(keypath)?;
50-
if xprv == get_xprv(keypath)? {
51-
Ok(xprv)
52-
} else {
53-
Err(())
54-
}
55-
}
56-
5748
/// Get the private key at the keypath.
5849
pub fn secp256k1_get_private_key(keypath: &[u32]) -> Result<zeroize::Zeroizing<Vec<u8>>, ()> {
5950
let xprv = get_xprv(keypath)?;
@@ -75,19 +66,35 @@ pub fn secp256k1_get_private_key_twice(keypath: &[u32]) -> Result<zeroize::Zeroi
7566
/// Can be used only if the keystore is unlocked. Returns the derived xpub,
7667
/// using bip32 derivation. Derivation is done from the xprv master, so hardened
7768
/// derivation is allowed.
78-
pub fn get_xpub(keypath: &[u32]) -> Result<bip32::Xpub, ()> {
79-
let xpriv = get_xprv_twice(keypath)?;
69+
pub fn get_xpub_once(keypath: &[u32]) -> Result<bip32::Xpub, ()> {
70+
let xpriv = get_xprv(keypath)?;
8071
let secp = bitcoin::secp256k1::Secp256k1::new();
8172
let xpub = bitcoin::bip32::Xpub::from_priv(&secp, &xpriv.xprv);
8273

8374
Ok(bip32::Xpub::from(xpub))
8475
}
8576

77+
/// Can be used only if the keystore is unlocked. Returns the derived xpub,
78+
/// using bip32 derivation. Derivation is done from the xprv master, so hardened
79+
/// derivation is allowed.
80+
pub fn get_xpub_twice(keypath: &[u32]) -> Result<bip32::Xpub, ()> {
81+
let res1 = get_xpub_once(keypath)?;
82+
let res2 = get_xpub_once(keypath)?;
83+
if res1 != res2 {
84+
return Err(());
85+
}
86+
Ok(res1)
87+
}
88+
8689
/// Returns fingerprint of the root public key at m/, which are the first four bytes of its hash160
8790
/// according to:
8891
/// https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#serialization-format
8992
pub fn root_fingerprint() -> Result<Vec<u8>, ()> {
90-
Ok(get_xpub(&[])?.pubkey_hash160().get(..4).ok_or(())?.to_vec())
93+
Ok(get_xpub_twice(&[])?
94+
.pubkey_hash160()
95+
.get(..4)
96+
.ok_or(())?
97+
.to_vec())
9198
}
9299

93100
fn bip85_entropy(keypath: &[u32]) -> Result<zeroize::Zeroizing<Vec<u8>>, ()> {
@@ -203,29 +210,29 @@ mod tests {
203210
}
204211

205212
#[test]
206-
fn test_get_xpub() {
213+
fn test_get_xpub_twice() {
207214
let keypath = &[44 + HARDENED, 0 + HARDENED, 0 + HARDENED];
208215
// Also test with unhardened and non-zero elements.
209216
let keypath_5 = &[44 + HARDENED, 1 + HARDENED, 10 + HARDENED, 1, 100];
210217

211218
keystore::lock();
212-
assert!(get_xpub(keypath).is_err());
219+
assert!(get_xpub_twice(keypath).is_err());
213220

214221
// 24 words
215222
mock_unlocked_using_mnemonic(
216223
"sleep own lobster state clean thrive tail exist cactus bitter pass soccer clinic riot dream turkey before sport action praise tunnel hood donate man",
217224
"",
218225
);
219226
assert_eq!(
220-
get_xpub(&[]).unwrap().serialize_str(bip32::XPubType::Xpub).unwrap(),
227+
get_xpub_twice(&[]).unwrap().serialize_str(bip32::XPubType::Xpub).unwrap(),
221228
"xpub661MyMwAqRbcEhX8d9WJh78SZrxusAzWFoykz4n5CF75uYRzixw5FZPUSoWyhaaJ1bpiPFdzdHSQqJN38PcTkyrLmxT4J2JDYfoGJQ4ioE2",
222229
);
223230
assert_eq!(
224-
get_xpub(keypath).unwrap().serialize_str(bip32::XPubType::Xpub).unwrap(),
231+
get_xpub_twice(keypath).unwrap().serialize_str(bip32::XPubType::Xpub).unwrap(),
225232
"xpub6Cj6NNCGj2CRPHvkuEG1rbW3nrNCAnLjaoTg1P67FCGoahSsbg9WQ7YaMEEP83QDxt2kZ3hTPAPpGdyEZcfAC1C75HfR66UbjpAb39f4PnG",
226233
);
227234
assert_eq!(
228-
get_xpub(keypath_5).unwrap().serialize_str(bip32::XPubType::Xpub).unwrap(),
235+
get_xpub_twice(keypath_5).unwrap().serialize_str(bip32::XPubType::Xpub).unwrap(),
229236
"xpub6HHn1zdtf1RjePopiTV5nxf8jY2xwbJicTQ91jV4cUJZ5EnbvXyBGDhqWt8B9JxxBt9vExi4pdWzrbrM43qSFs747VCGmSy2DPWAhg9MkUg",
230237
);
231238

@@ -235,7 +242,7 @@ mod tests {
235242
"",
236243
);
237244
assert_eq!(
238-
get_xpub(keypath).unwrap().serialize_str(bip32::XPubType::Xpub).unwrap(),
245+
get_xpub_twice(keypath).unwrap().serialize_str(bip32::XPubType::Xpub).unwrap(),
239246
"xpub6C7fKxGtTzEVxCC22U2VHx4GpaVy77DzU6KdZ1CLuHgoUGviBMWDc62uoQVxqcRa5RQbMPnffjpwxve18BG81VJhJDXnSpRe5NGKwVpXiAb",
240247
);
241248

@@ -245,7 +252,7 @@ mod tests {
245252
"",
246253
);
247254
assert_eq!(
248-
get_xpub(keypath).unwrap().serialize_str(bip32::XPubType::Xpub).unwrap(),
255+
get_xpub_twice(keypath).unwrap().serialize_str(bip32::XPubType::Xpub).unwrap(),
249256
"xpub6DLvpzjKpJ8k4xYrWYPmZQkUe9dkG1eRig2v6Jz4iYgo8hcpHWx87gGoCGDaB2cHFZ3ExUfe1jDiMu7Ch6gA4ULCBhvwZj29mHCPYSux3YV",
250257
)
251258
}
@@ -401,7 +408,7 @@ mod tests {
401408
test.expected_mnemonic,
402409
);
403410
let keypath = &[44 + HARDENED, 0 + HARDENED, 0 + HARDENED];
404-
let xpub = get_xpub(keypath).unwrap();
411+
let xpub = get_xpub_once(keypath).unwrap();
405412
assert_eq!(
406413
xpub.serialize_str(crate::bip32::XPubType::Xpub).unwrap(),
407414
test.expected_xpub,

0 commit comments

Comments
 (0)