keystore: encode xpub to bytes, not string

It is used as an intermediate representation when getting an xpub in
Rust. Bytes are enough, no need to base58Check encode/decode.

keystore_encode_xpub then becomes unused (only used in
test_keystore_functional.c).

Author: benma

src/CMakeLists.txt

@@ -323,6 +323,7 @@ add_custom_target(rust-bindgen
     --allowlist-function keystore_encode_xpub_at_keypath
     --allowlist-function keystore_encrypt_and_store_seed
     --allowlist-var XPUB_ENCODED_LEN
+    --allowlist-var BIP32_SERIALIZED_LEN
     --allowlist-function lock_animation_start
     --allowlist-function lock_animation_stop
     --allowlist-function delay_us

src/keystore.c

@@ -681,27 +681,17 @@ bool keystore_get_ed25519_seed(uint8_t* seed_out)
     return true;
 }
 
-bool keystore_encode_xpub(const struct ext_key* xpub, char* out, size_t out_len)
-{
-    uint8_t bytes[BIP32_SERIALIZED_LEN] = {0};
-    if (bip32_key_serialize(xpub, BIP32_FLAG_KEY_PUBLIC, bytes, sizeof(bytes)) != WALLY_OK) {
-        return false;
-    }
-    return rust_base58_encode_check(
-        rust_util_bytes(bytes, sizeof(bytes)), rust_util_cstr_mut(out, out_len));
-}
-
 USE_RESULT bool keystore_encode_xpub_at_keypath(
     const uint32_t* keypath,
     size_t keypath_len,
-    char* out,
-    size_t out_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 keystore_encode_xpub(&derived_xpub, out, out_len);
+    return bip32_key_serialize(&derived_xpub, BIP32_FLAG_KEY_PUBLIC, out, BIP32_SERIALIZED_LEN) ==
+           WALLY_OK;
 }
 
 static void _tagged_hash(const char* tag, const uint8_t* msg, size_t msg_len, uint8_t* hash_out)

src/keystore.h

@@ -249,24 +249,14 @@ 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 as a base58 string.
- * @param[in] xpub the xpub to encode.
- * @param[out] out resulting string, must be at least of size `XPUB_ENCODED_LEN` (including the null
- * terminator).
- * @param[in] out_len size of `out`.
- * @return false on failure, true on success.
- */
-USE_RESULT bool keystore_encode_xpub(const struct ext_key* xpub, char* out, size_t out_len);
-
-/**
- * Encode an xpub as a base58 string at the given `keypath`.
- * Args the same as `keystore_encode_xpub`.
+ * 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,
-    char* out,
-    size_t out_len);
+    uint8_t* out);
 
 /**
  * Return the tweaked taproot pubkey at the given keypath.

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

@@ -61,21 +61,28 @@ pub fn serialize_xpub_str(xpub: &pb::XPub, xpub_type: XPubType) -> Result<String
         .into_string())
 }
 
-/// Parses a Base58Check-encoded xpub string. The 4 version bytes are not checked and discarded.
-pub fn parse_xpub(xpub: &str) -> Result<pb::XPub, ()> {
-    let decoded = bs58::decode(xpub).with_check(None).into_vec().or(Err(()))?;
-    if decoded.len() != 78 {
+/// Parses an 78-ytes xpub bytestring, encoded according to BIP32. The 4 version bytes are not
+/// checked and discarded.
+pub fn parse_xpub_bytes(xpub: &[u8]) -> Result<pb::XPub, ()> {
+    if xpub.len() != 78 {
         return Err(());
     }
     Ok(pb::XPub {
-        depth: decoded[4..5].to_vec(),
-        parent_fingerprint: decoded[5..9].to_vec(),
-        child_num: u32::from_be_bytes(core::convert::TryInto::try_into(&decoded[9..13]).unwrap()),
-        chain_code: decoded[13..45].to_vec(),
-        public_key: decoded[45..78].to_vec(),
+        depth: xpub[4..5].to_vec(),
+        parent_fingerprint: xpub[5..9].to_vec(),
+        child_num: u32::from_be_bytes(core::convert::TryInto::try_into(&xpub[9..13]).unwrap()),
+        chain_code: xpub[13..45].to_vec(),
+        public_key: xpub[45..78].to_vec(),
     })
 }
 
+/// Parses a Base58Check-encoded xpub string. The 4 version bytes are not checked and discarded.
+#[cfg(test)]
+pub fn parse_xpub(xpub: &str) -> Result<pb::XPub, ()> {
+    let decoded = bs58::decode(xpub).with_check(None).into_vec().or(Err(()))?;
+    parse_xpub_bytes(&decoded)
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

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

@@ -24,9 +24,8 @@ use bitbox02::keystore;
 
 /// Derives an xpub from the keystore seed at the given keypath.
 pub fn get_xpub(keypath: &[u32]) -> Result<pb::XPub, ()> {
-    // Convert from C keystore to Rust by encoding the xpub in C and decoding it in Rust. Would be a
-    // bit better to encode/decoding using the raw 78 bytes, not the base58Check encoding.
-    bip32::parse_xpub(&keystore::encode_xpub_at_keypath(keypath)?)
+    // Convert from C keystore to Rust by encoding the xpub in C and decoding it in Rust.
+    bip32::parse_xpub_bytes(&keystore::encode_xpub_at_keypath(keypath)?)
 }
 
 /// Return the hash160 of the secp256k1 public key at the keypath.

src/rust/bitbox02/src/keystore.rs

@@ -14,6 +14,7 @@
 
 extern crate alloc;
 use alloc::string::String;
+use alloc::vec;
 use alloc::vec::Vec;
 
 use core::convert::TryInto;
@@ -198,19 +199,16 @@ pub fn secp256k1_pubkey_uncompressed(
     }
 }
 
-pub fn encode_xpub_at_keypath(keypath: &[u32]) -> Result<String, ()> {
-    let mut xpub = [0u8; bitbox02_sys::XPUB_ENCODED_LEN as _];
+pub fn encode_xpub_at_keypath(keypath: &[u32]) -> Result<Vec<u8>, ()> {
+    let mut xpub = vec![0u8; bitbox02_sys::BIP32_SERIALIZED_LEN as _];
     match unsafe {
         bitbox02_sys::keystore_encode_xpub_at_keypath(
             keypath.as_ptr(),
             keypath.len() as _,
             xpub.as_mut_ptr(),
-            xpub.len() as _,
         )
     } {
-        true => Ok(crate::util::str_from_null_terminated(&xpub[..])
-            .unwrap()
-            .into()),
+        true => Ok(xpub),
         false => Err(()),
     }
 }

test/unit-test/test_keystore.c

@@ -449,28 +449,6 @@ static void _test_keystore_get_bip39_mnemonic(void** state)
     assert_true(keystore_get_bip39_mnemonic(mnemonic, strlen(expected_mnemonic) + 1));
 }
 
-static void _test_keystore_encode_xpub(void** state)
-{
-    struct ext_key xpub = {0};
-    assert_int_equal(
-        bip32_key_from_seed(
-            (const unsigned char*)"seedseedseedseed",
-            BIP32_ENTROPY_LEN_128,
-            BIP32_VER_MAIN_PRIVATE,
-            BIP32_FLAG_SKIP_HASH,
-            &xpub),
-        WALLY_OK);
-    assert_int_equal(bip32_key_strip_private_key(&xpub), WALLY_OK);
-    char out[XPUB_ENCODED_LEN] = {0};
-    assert_false(keystore_encode_xpub(&xpub, out, 110));
-
-    assert_true(keystore_encode_xpub(&xpub, out, sizeof(out)));
-    assert_string_equal(
-        out,
-        "xpub661MyMwAqRbcFLG1NSwsGkQxYGaRj3qDsDB6g64CviEc82D3r7Dp4eMnWdarcVkpPbMgwwuLLPPwCXVQFWWomv"
-        "yj6QKEuDXWvNbCDF98tgM");
-}
-
 static void _test_keystore_create_and_store_seed(void** state)
 {
     const uint8_t seed_random[32] =
@@ -736,7 +714,6 @@ int main(void)
         cmocka_unit_test(_test_keystore_unlock),
         cmocka_unit_test(_test_keystore_lock),
         cmocka_unit_test(_test_keystore_get_bip39_mnemonic),
-        cmocka_unit_test(_test_keystore_encode_xpub),
         cmocka_unit_test(_test_keystore_create_and_store_seed),
         cmocka_unit_test(_test_keystore_get_ed25519_seed),
         cmocka_unit_test(_test_secp256k1_schnorr_sign),

test/unit-test/test_keystore_functional.c

@@ -23,6 +23,7 @@
 #include <memory/bitbox02_smarteeprom.h>
 #include <memory/smarteeprom.h>
 #include <mock_memory.h>
+#include <rust/rust.h>
 #include <securechip/securechip.h>
 #include <util.h>
 
@@ -107,6 +108,16 @@ static void _assert_equal_memory_hex(const uint8_t* buf, size_t buf_size, const
     assert_string_equal(buf_hex, expected_hex);
 }
 
+static bool _encode_xpub(const struct ext_key* xpub, char* out, size_t out_len)
+{
+    uint8_t bytes[BIP32_SERIALIZED_LEN] = {0};
+    if (bip32_key_serialize(xpub, BIP32_FLAG_KEY_PUBLIC, bytes, sizeof(bytes)) != WALLY_OK) {
+        return false;
+    }
+    return rust_base58_encode_check(
+        rust_util_bytes(bytes, sizeof(bytes)), rust_util_cstr_mut(out, out_len));
+}
+
 static void _check_pubs(const char* expected_xpub, const char* expected_pubkey_uncompressed_hex)
 {
     struct ext_key __attribute__((__cleanup__(keystore_zero_xkey))) xpub;
@@ -120,7 +131,7 @@ static void _check_pubs(const char* expected_xpub, const char* expected_pubkey_u
 
     assert_true(keystore_get_xpub(keypath, 3, &xpub));
     char xpub_serialized[120];
-    assert_true(keystore_encode_xpub(&xpub, xpub_serialized, sizeof(xpub_serialized)));
+    assert_true(_encode_xpub(&xpub, xpub_serialized, sizeof(xpub_serialized)));
     assert_string_equal(xpub_serialized, expected_xpub);
 
     uint8_t pubkey_uncompressed[EC_PUBLIC_KEY_UNCOMPRESSED_LEN];