keystore: port get_xprv, get_xpub, get_private_key to Rust

By using `bitcoin::bip32`.

The `bip32::Xprv` wrapper is there to try to avoid unzeroed copies of
private data. It is only a best effort attempt, as the `bitcoin` and
`secp256k1` Rust crate maintainers don't support zeroizing and don't
think it's worthwhile to worry about it until the Rust compiler puts
in some guarantees. See
rust-bitcoin/rust-secp256k1#553.

`bitbox02_rust::keystore::tests::test_get_xpub` checks there is no
regression.

`secp256k1_get_private_key_twice` is introduced, as in C, some calls
used `_get_xprv_twice()` to get to the private key, and others only
`_get_xprv()` (the ones where a bitflip or similar would not be
critical).

Author: benma

src/keystore.c

@@ -552,68 +552,6 @@ static bool _get_xprv(const uint32_t* keypath, const size_t keypath_len, struct
     return true;
 }
 
-static bool _ext_key_equal(struct ext_key* one, struct ext_key* two)
-{
-    if (!MEMEQ(one->chain_code, two->chain_code, sizeof(one->chain_code))) {
-        return false;
-    }
-    if (!MEMEQ(one->parent160, two->parent160, sizeof(one->parent160))) {
-        return false;
-    }
-    if (one->depth != two->depth) {
-        return false;
-    }
-    if (!MEMEQ(one->priv_key, two->priv_key, sizeof(one->priv_key))) {
-        return false;
-    }
-    if (one->child_num != two->child_num) {
-        return false;
-    }
-    if (!MEMEQ(one->hash160, two->hash160, sizeof(one->hash160))) {
-        return false;
-    }
-    if (one->version != two->version) {
-        return false;
-    }
-    if (!MEMEQ(one->pub_key, two->pub_key, sizeof(one->pub_key))) {
-        return false;
-    }
-    return true;
-}
-
-static bool _get_xprv_twice(
-    const uint32_t* keypath,
-    const size_t keypath_len,
-    struct ext_key* xprv_out)
-{
-    struct ext_key one __attribute__((__cleanup__(keystore_zero_xkey))) = {0};
-    if (!_get_xprv(keypath, keypath_len, &one)) {
-        return false;
-    }
-    if (!_get_xprv(keypath, keypath_len, xprv_out)) {
-        return false;
-    }
-    if (!_ext_key_equal(&one, xprv_out)) {
-        keystore_zero_xkey(xprv_out);
-        return false;
-    }
-    return true;
-}
-
-bool keystore_get_xpub(
-    const uint32_t* keypath,
-    const size_t keypath_len,
-    struct ext_key* hdkey_neutered_out)
-{
-    struct ext_key xprv __attribute__((__cleanup__(keystore_zero_xkey))) = {0};
-    if (!_get_xprv_twice(keypath, keypath_len, &xprv)) {
-        return false;
-    }
-    bip32_key_strip_private_key(&xprv); // neuter
-    *hdkey_neutered_out = xprv;
-    return true;
-}
-
 void keystore_zero_xkey(struct ext_key* xkey)
 {
     util_zero(xkey, sizeof(struct ext_key));
@@ -739,19 +677,6 @@ bool keystore_get_ed25519_seed(uint8_t* seed_out)
     return true;
 }
 
-USE_RESULT bool keystore_encode_xpub_at_keypath(
-    const uint32_t* keypath,
-    size_t keypath_len,
-    uint8_t* out)
-{
-    struct ext_key derived_xpub __attribute__((__cleanup__(keystore_zero_xkey))) = {0};
-    if (!keystore_get_xpub(keypath, keypath_len, &derived_xpub)) {
-        return false;
-    }
-    return bip32_key_serialize(&derived_xpub, BIP32_FLAG_KEY_PUBLIC, out, BIP32_SERIALIZED_LEN) ==
-           WALLY_OK;
-}
-
 static bool _schnorr_keypair(
     const uint32_t* keypath,
     size_t keypath_len,
@@ -808,19 +733,6 @@ bool keystore_secp256k1_schnorr_sign(
     return secp256k1_schnorrsig_verify(ctx, sig64_out, msg32, 32, &pubkey) == 1;
 }
 
-bool keystore_secp256k1_get_private_key(
-    const uint32_t* keypath,
-    const size_t keypath_len,
-    uint8_t* key_out)
-{
-    struct ext_key xprv __attribute__((__cleanup__(keystore_zero_xkey))) = {0};
-    if (!_get_xprv_twice(keypath, keypath_len, &xprv)) {
-        return false;
-    }
-    memcpy(key_out, xprv.priv_key + 1, 32);
-    return true;
-}
-
 #ifdef TESTING
 void keystore_mock_unlocked(const uint8_t* seed, size_t seed_len, const uint8_t* bip39_seed)
 {

src/keystore.h

@@ -146,18 +146,6 @@ USE_RESULT bool keystore_bip39_mnemonic_to_seed(
     uint8_t* seed_out,
     size_t* seed_len_out);
 
-/**
- * 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.
- * On success, xpub_out must be destroyed using keystore_zero_xkey().
- * @return true on success, false on failure.
- */
-USE_RESULT bool keystore_get_xpub(
-    const uint32_t* keypath,
-    size_t keypath_len,
-    struct ext_key* hdkey_neutered_out);
-
 /**
  * Safely destroy a xpub or xprv.
  */
@@ -240,16 +228,6 @@ USE_RESULT bool keystore_get_u2f_seed(uint8_t* seed_out);
  */
 USE_RESULT bool keystore_get_ed25519_seed(uint8_t* seed_out);
 
-/**
- * Encode an xpub at the given `keypath` as 78 bytes according to BIP32. The version bytes are
- * the ones corresponding to `xpub`, i.e. 0x0488B21E.
- * `out` must be `BIP32_SERIALIZED_LEN` long.
- */
-USE_RESULT bool keystore_encode_xpub_at_keypath(
-    const uint32_t* keypath,
-    size_t keypath_len,
-    uint8_t* out);
-
 /**
  * Sign a message that verifies against the pubkey tweaked using BIP-86.
  *
@@ -267,18 +245,6 @@ USE_RESULT bool keystore_secp256k1_schnorr_sign(
     const uint8_t* tweak,
     uint8_t* sig64_out);
 
-/**
- * Get the private key at the keypath.
- *
- * @param[in] keypath derivation keypath
- * @param[in] keypath_len number of elements in keypath
- * @param[out] key_out resulting private key, must be 32 bytes.
- */
-USE_RESULT bool keystore_secp256k1_get_private_key(
-    const uint32_t* keypath,
-    size_t keypath_len,
-    uint8_t* key_out);
-
 #ifdef TESTING
 /**
  * convenience to mock the keystore state (locked, seed) in tests.

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

@@ -13,12 +13,46 @@
 // limitations under the License.
 
 use super::pb;
+use alloc::boxed::Box;
 use alloc::string::String;
 use alloc::vec::Vec;
 
 pub use pb::btc_pub_request::XPubType;
 
 use bitcoin::hashes::Hash;
+use zeroize::Zeroize;
+
+// Wrapper of `bitcoin::bip32::Xpriv` to imlement zeroizing on drop.
+#[derive(PartialEq)]
+pub struct Xprv {
+    // Boxed so moves don't leave copies.
+    pub xprv: Box<bitcoin::bip32::Xpriv>,
+}
+
+impl Drop for Xprv {
+    fn drop(&mut self) {
+        self.xprv.depth.zeroize();
+        let pf: &mut [u8; 4] = self.xprv.parent_fingerprint.as_mut();
+        pf.zeroize();
+        self.xprv.child_number = bitcoin::bip32::ChildNumber::from(0u32);
+        self.xprv.private_key.non_secure_erase();
+        let chain_code: &mut [u8; 32] = self.xprv.chain_code.as_mut();
+        chain_code.zeroize();
+    }
+}
+
+impl From<bitcoin::bip32::Xpriv> for Xprv {
+    /// Convert from `bitcoin::bip32::Xpriv`, attempting to Box it without leaving copies by using
+    /// [RVO](https://github.com/rust-lang/rfcs/pull/2884).
+    ///
+    /// This is only a best effort attempt, as RVO is not guaranteed, and copies might be left on
+    /// the stack in various places, e.g. in the hmac-sha512 engine inside `new_master()`, etc.
+    fn from(value: bitcoin::bip32::Xpriv) -> Self {
+        let xprv = Box::<bitcoin::bip32::Xpriv>::new_uninit();
+        let xprv = Box::write(xprv, value);
+        Self { xprv }
+    }
+}
 
 #[derive(Clone)]
 pub struct Xpub {

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

@@ -784,7 +784,7 @@ async fn _process(
         if let Some(ref mut silent_payment) = silent_payment {
             let keypair = bitcoin::key::UntweakedKeypair::from_seckey_slice(
                 silent_payment.get_secp(),
-                &bitbox02::keystore::secp256k1_get_private_key(&tx_input.keypath)?,
+                &crate::keystore::secp256k1_get_private_key(&tx_input.keypath)?,
             )
             .unwrap();
             // For Taproot, only key path spends are allowed in silent payments, and we need to

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

@@ -31,10 +31,56 @@ pub fn get_bip39_mnemonic() -> Result<zeroize::Zeroizing<String>, ()> {
     keystore::bip39_mnemonic_from_seed(&keystore::copy_seed()?)
 }
 
-/// Derives an xpub from the keystore seed at the given keypath.
+fn get_xprv(keypath: &[u32]) -> Result<bip32::Xprv, ()> {
+    let bip39_seed = keystore::copy_bip39_seed()?;
+    let xprv: bip32::Xprv =
+        bitcoin::bip32::Xpriv::new_master(bitcoin::NetworkKind::Main, &bip39_seed)
+            .map_err(|_| ())?
+            .into();
+    let secp = bitcoin::secp256k1::Secp256k1::new();
+    Ok(xprv
+        .xprv
+        .derive_priv(&secp, &bip32::keypath_from_slice(keypath))
+        .map_err(|_| ())?
+        .into())
+}
+
+fn get_xprv_twice(keypath: &[u32]) -> Result<bip32::Xprv, ()> {
+    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<zeroize::Zeroizing<Vec<u8>>, ()> {
+    let xprv = get_xprv(keypath)?;
+    Ok(zeroize::Zeroizing::new(
+        xprv.xprv.private_key.secret_bytes().to_vec(),
+    ))
+}
+
+/// Get the private key at the keypath, computed twice to mitigate the risk of bitflips.
+pub fn secp256k1_get_private_key_twice(keypath: &[u32]) -> Result<zeroize::Zeroizing<Vec<u8>>, ()> {
+    let privkey = secp256k1_get_private_key(keypath)?;
+    if privkey == secp256k1_get_private_key(keypath)? {
+        Ok(privkey)
+    } else {
+        Err(())
+    }
+}
+
+/// 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(keypath: &[u32]) -> Result<bip32::Xpub, ()> {
-    // Convert from C keystore to Rust by encoding the xpub in C and decoding it in Rust.
-    bip32::Xpub::from_bytes(&keystore::encode_xpub_at_keypath(keypath)?)
+    let xpriv = get_xprv_twice(keypath)?;
+    let secp = bitcoin::secp256k1::Secp256k1::new();
+    let xpub = bitcoin::bip32::Xpub::from_priv(&secp, &xpriv.xprv);
+
+    Ok(bip32::Xpub::from(xpub))
 }
 
 /// Returns fingerprint of the root public key at m/, which are the first four bytes of its hash160
@@ -45,7 +91,7 @@ pub fn root_fingerprint() -> Result<Vec<u8>, ()> {
 }
 
 fn bip85_entropy(keypath: &[u32]) -> Result<zeroize::Zeroizing<Vec<u8>>, ()> {
-    let priv_key = keystore::secp256k1_get_private_key(keypath)?;
+    let priv_key = secp256k1_get_private_key_twice(keypath)?;
     let mut mac = SimpleHmac::<Sha512>::new_from_slice(b"bip-entropy-from-k").unwrap();
     mac.update(&priv_key);
     let mut out = zeroize::Zeroizing::new(vec![0u8; 64]);
@@ -112,6 +158,40 @@ mod tests {
         mock_memory, mock_unlocked, mock_unlocked_using_mnemonic, TEST_MNEMONIC,
     };
 
+    #[test]
+    fn test_secp256k1_get_private_key() {
+        keystore::lock();
+        let keypath = &[84 + HARDENED, 0 + HARDENED, 0 + HARDENED, 0, 0];
+        assert!(secp256k1_get_private_key(keypath).is_err());
+
+        mock_unlocked_using_mnemonic(
+            "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about",
+            "",
+        );
+
+        assert_eq!(
+            hex::encode(secp256k1_get_private_key(keypath).unwrap()),
+            "4604b4b710fe91f584fff084e1a9159fe4f8408fff380596a604948474ce4fa3"
+        );
+    }
+
+    #[test]
+    fn test_secp256k1_get_private_key_twice() {
+        keystore::lock();
+        let keypath = &[84 + HARDENED, 0 + HARDENED, 0 + HARDENED, 0, 0];
+        assert!(secp256k1_get_private_key_twice(keypath).is_err());
+
+        mock_unlocked_using_mnemonic(
+            "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about",
+            "",
+        );
+
+        assert_eq!(
+            hex::encode(secp256k1_get_private_key_twice(keypath).unwrap()),
+            "4604b4b710fe91f584fff084e1a9159fe4f8408fff380596a604948474ce4fa3"
+        );
+    }
+
     #[test]
     fn test_get_bip39_mnemonic() {
         keystore::lock();
@@ -125,6 +205,8 @@ mod tests {
     #[test]
     fn test_get_xpub() {
         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());
@@ -142,6 +224,10 @@ mod tests {
             get_xpub(keypath).unwrap().serialize_str(bip32::XPubType::Xpub).unwrap(),
             "xpub6Cj6NNCGj2CRPHvkuEG1rbW3nrNCAnLjaoTg1P67FCGoahSsbg9WQ7YaMEEP83QDxt2kZ3hTPAPpGdyEZcfAC1C75HfR66UbjpAb39f4PnG",
         );
+        assert_eq!(
+            get_xpub(keypath_5).unwrap().serialize_str(bip32::XPubType::Xpub).unwrap(),
+            "xpub6HHn1zdtf1RjePopiTV5nxf8jY2xwbJicTQ91jV4cUJZ5EnbvXyBGDhqWt8B9JxxBt9vExi4pdWzrbrM43qSFs747VCGmSy2DPWAhg9MkUg",
+        );
 
         // 18 words
         mock_unlocked_using_mnemonic(

src/rust/bitbox02-sys/build.rs

@@ -71,15 +71,13 @@ const ALLOWLIST_FNS: &[&str] = &[
     "keystore_copy_seed",
     "keystore_copy_bip39_seed",
     "keystore_create_and_store_seed",
-    "keystore_encode_xpub_at_keypath",
     "keystore_encrypt_and_store_seed",
     "keystore_get_bip39_word",
     "keystore_get_ed25519_seed",
     "keystore_get_u2f_seed",
     "keystore_is_locked",
     "keystore_lock",
     "keystore_mock_unlocked",
-    "keystore_secp256k1_get_private_key",
     "keystore_secp256k1_nonce_commit",
     "keystore_secp256k1_schnorr_sign",
     "keystore_secp256k1_sign",