Improve pubkey memory management

src/common/wallet.c

@@ -2,6 +2,7 @@
 #include <string.h>
 #include <limits.h>
 
+#include "../common/base58.h"
 #include "../common/bip32.h"
 #include "../common/buffer.h"
 #include "../common/script.h"
@@ -358,20 +359,41 @@ int parse_policy_map_key_info(buffer_t *buffer, policy_map_key_info_t *out, int
 
  // consume the rest of the buffer into the pubkey, except possibly the final "/**"
  unsigned int ext_pubkey_len = 0;
+ char ext_pubkey_str[MAX_SERIALIZED_PUBKEY_LENGTH];
  uint8_t c;
  while (ext_pubkey_len < MAX_SERIALIZED_PUBKEY_LENGTH && buffer_peek(buffer, &c) &&
  is_alphanumeric(c)) {
- out->ext_pubkey[ext_pubkey_len] = c;
+ ext_pubkey_str[ext_pubkey_len] = c;
  ++ext_pubkey_len;
  buffer_seek_cur(buffer, 1);
  }
- out->ext_pubkey[ext_pubkey_len] = '\0';
+ ext_pubkey_str[ext_pubkey_len] = '\0';
 
  if (ext_pubkey_len < 111 || ext_pubkey_len > 112) {
  // loose sanity check; pubkeys in bitcoin can be 111 or 112 characters long
  return WITH_ERROR(-1, "Invalid extended pubkey length");
  }
 
+ serialized_extended_pubkey_check_t ext_pubkey_check;
+ if (base58_decode(ext_pubkey_str,
+ ext_pubkey_len,
+ (uint8_t *) &ext_pubkey_check,
+ sizeof(ext_pubkey_check)) < 0) {
+ return WITH_ERROR(-1, "Error decoding serialized extended pubkey");
+ }
+
+ // verify checksum
+ uint8_t checksum[4];
+ crypto_get_checksum((uint8_t *) &ext_pubkey_check.serialized_extended_pubkey,
+ sizeof(ext_pubkey_check.serialized_extended_pubkey),
+ checksum);
+
+ if (memcmp(&ext_pubkey_check.checksum, checksum, sizeof(checksum)) != 0) {
+ return WITH_ERROR(-1, "Wrong extended pubkey checksum");
+ }
+
+ out->ext_pubkey = ext_pubkey_check.serialized_extended_pubkey;
+
  // either the string terminates now, or it has a final "/**" suffix for the wildcard.
  if (!buffer_can_read(buffer, 1)) {
  // no wildcard; this is an error in V1

src/common/wallet.h

@@ -6,6 +6,7 @@
 #include "common/bip32.h"
 #include "common/buffer.h"
 #include "../constants.h"
+#include "../crypto.h"
 
 #ifndef SKIP_FOR_CMOCKA
 #include "os.h"
@@ -80,7 +81,7 @@ typedef struct {
  uint8_t master_key_derivation_len;
  uint8_t has_key_origin;
  uint8_t has_wildcard; // true iff the keys ends with the wildcard (/ followed by **)
- char ext_pubkey[MAX_SERIALIZED_PUBKEY_LENGTH + 1];
+ serialized_extended_pubkey_t ext_pubkey;
 } policy_map_key_info_t;
 
 typedef struct {

src/crypto.c

@@ -296,14 +296,10 @@ bool crypto_derive_symmetric_key(const char *label, size_t label_len, uint8_t ke
  return true;
 }
 
-// TODO: Split serialization from key derivation?
-// It might be difficult to have a clean API without wasting memory, as the checksum
-// needs to be concatenated to the data before base58 serialization.
-int get_serialized_extended_pubkey_at_path(const uint32_t bip32_path[],
- uint8_t bip32_path_len,
- uint32_t bip32_pubkey_version,
- char out_xpub[static MAX_SERIALIZED_PUBKEY_LENGTH + 1],
- serialized_extended_pubkey_t *out_pubkey) {
+int get_extended_pubkey_at_path(const uint32_t bip32_path[],
+ uint8_t bip32_path_len,
+ uint32_t bip32_pubkey_version,
+ serialized_extended_pubkey_t *out_pubkey) {
  // find parent key's fingerprint and child number
  uint32_t parent_fingerprint = 0;
  uint32_t child_number = 0;
@@ -312,43 +308,30 @@ int get_serialized_extended_pubkey_at_path(const uint32_t bip32_path[],
  // for the response, in order to save memory
 
  uint8_t parent_pubkey[33];
- crypto_get_compressed_pubkey_at_path(bip32_path, bip32_path_len - 1, parent_pubkey, NULL);
+ if (!crypto_get_compressed_pubkey_at_path(bip32_path,
+ bip32_path_len - 1,
+ parent_pubkey,
+ NULL)) {
+ return -1;
+ }
 
  parent_fingerprint = crypto_get_key_fingerprint(parent_pubkey);
  child_number = bip32_path[bip32_path_len - 1];
  }
 
- struct {
- serialized_extended_pubkey_t ext_pubkey;
- uint8_t checksum[4];
- } ext_pubkey_check; // extended pubkey and checksum
-
- serialized_extended_pubkey_t *ext_pubkey = &ext_pubkey_check.ext_pubkey;
-
- write_u32_be(ext_pubkey->version, 0, bip32_pubkey_version);
- ext_pubkey->depth = bip32_path_len;
- write_u32_be(ext_pubkey->parent_fingerprint, 0, parent_fingerprint);
- write_u32_be(ext_pubkey->child_number, 0, child_number);
-
- crypto_get_compressed_pubkey_at_path(bip32_path,
- bip32_path_len,
- ext_pubkey->compressed_pubkey,
- ext_pubkey->chain_code);
- crypto_get_checksum((uint8_t *) ext_pubkey, 78, ext_pubkey_check.checksum);
+ write_u32_be(out_pubkey->version, 0, bip32_pubkey_version);
+ out_pubkey->depth = bip32_path_len;
+ write_u32_be(out_pubkey->parent_fingerprint, 0, parent_fingerprint);
+ write_u32_be(out_pubkey->child_number, 0, child_number);
 
- if (out_pubkey != NULL) {
- memcpy(out_pubkey, &ext_pubkey_check.ext_pubkey, sizeof(ext_pubkey_check.ext_pubkey));
+ if (!crypto_get_compressed_pubkey_at_path(bip32_path,
+ bip32_path_len,
+ out_pubkey->compressed_pubkey,
+ out_pubkey->chain_code)) {
+ return -1;
  }
 
- int serialized_pubkey_len = base58_encode((uint8_t *) &ext_pubkey_check,
- 78 + 4,
- out_xpub,
- MAX_SERIALIZED_PUBKEY_LENGTH);
-
- if (serialized_pubkey_len > 0) {
- out_xpub[serialized_pubkey_len] = '\0';
- }
- return serialized_pubkey_len;
+ return 0;
 }
 
 int base58_encode_address(const uint8_t in[20], uint32_t version, char *out, size_t out_len) {

src/crypto.h

@@ -260,19 +260,15 @@ uint32_t crypto_get_master_key_fingerprint();
  * Number of steps in the BIP32 derivation.
  * @param[in] bip32_pubkey_version
  * Version prefix to use for the pubkey.
- * @param[out] out_xpub
- * Pointer to the output buffer, which must be long enough to contain the result
- * (including the terminating null character).
  * @param[out] out_pubkey
- * If not NULL, pointer to a serialized_extended_pubkey_t.
+ * A pointer to a serialized_extended_pubkey_t.
  *
- * @return the length of the output pubkey (not including the null character), or -1 on error.
+ * @return 0 on success, or -1 on error.
  */
-int get_serialized_extended_pubkey_at_path(const uint32_t bip32_path[],
- uint8_t bip32_path_len,
- uint32_t bip32_pubkey_version,
- char out_xpub[static MAX_SERIALIZED_PUBKEY_LENGTH + 1],
- serialized_extended_pubkey_t *out_pubkey);
+int get_extended_pubkey_at_path(const uint32_t bip32_path[],
+ uint8_t bip32_path_len,
+ uint32_t bip32_pubkey_version,
+ serialized_extended_pubkey_t *out_pubkey);
 
 /**
  * Derives the level-1 symmetric key at the given label using SLIP-0021.

src/handler/get_extended_pubkey.c

@@ -20,6 +20,7 @@
 #include "boilerplate/io.h"
 #include "boilerplate/dispatcher.h"
 #include "boilerplate/sw.h"
+#include "../common/base58.h"
 #include "../common/bip32.h"
 #include "../commands.h"
 #include "../constants.h"
@@ -141,27 +142,41 @@ void handler_get_extended_pubkey(dispatcher_context_t *dc, uint8_t protocol_vers
  return;
  }
 
- char serialized_pubkey_str[MAX_SERIALIZED_PUBKEY_LENGTH + 1];
+ serialized_extended_pubkey_check_t pubkey_check;
+ if (0 > get_extended_pubkey_at_path(bip32_path,
+ bip32_path_len,
+ BIP32_PUBKEY_VERSION,
+ &pubkey_check.serialized_extended_pubkey)) {
+ PRINTF("Failed getting bip32 pubkey\n");
+ SEND_SW(dc, SW_BAD_STATE);
+ return;
+ }
+
+ crypto_get_checksum((uint8_t *) &pubkey_check.serialized_extended_pubkey,
+ sizeof(pubkey_check.serialized_extended_pubkey),
+ pubkey_check.checksum);
 
- int serialized_pubkey_len = get_serialized_extended_pubkey_at_path(bip32_path,
- bip32_path_len,
- BIP32_PUBKEY_VERSION,
- serialized_pubkey_str,
- NULL);
- if (serialized_pubkey_len == -1) {
+ char pubkey_str[MAX_SERIALIZED_PUBKEY_LENGTH + 1];
+ int pubkey_str_len = base58_encode((uint8_t *) &pubkey_check,
+ sizeof(pubkey_check),
+ pubkey_str,
+ sizeof(pubkey_str));
+ if (pubkey_str_len != 111 && pubkey_str_len != 112) {
+ PRINTF("Failed encoding base58 pubkey\n");
  SEND_SW(dc, SW_BAD_STATE);
  return;
  }
+ pubkey_str[pubkey_str_len] = 0;
 
  char path_str[MAX_SERIALIZED_BIP32_PATH_LENGTH + 1] = "(Master key)";
  if (bip32_path_len > 0) {
  bip32_path_format(bip32_path, bip32_path_len, path_str, sizeof(path_str));
  }
 
- if (display && !ui_display_pubkey(dc, path_str, !is_safe, serialized_pubkey_str)) {
+ if (display && !ui_display_pubkey(dc, path_str, !is_safe, pubkey_str)) {
  SEND_SW(dc, SW_DENY);
  return;
  }
 
- SEND_RESPONSE(dc, serialized_pubkey_str, strlen(serialized_pubkey_str), SW_OK);
+ SEND_RESPONSE(dc, pubkey_str, pubkey_str_len, SW_OK);
 }

src/handler/lib/policy.c

@@ -411,20 +411,7 @@ __attribute__((noinline, warn_unused_result)) static int get_extended_pubkey(
  return -1;
  }
  }
-
- // decode pubkey
- serialized_extended_pubkey_check_t decoded_pubkey_check;
- if (base58_decode(key_info.ext_pubkey,
- strlen(key_info.ext_pubkey),
- (uint8_t *) &decoded_pubkey_check,
- sizeof(decoded_pubkey_check)) == -1) {
- return -1;
- }
- // TODO: validate checksum
-
- memcpy(out,
- &decoded_pubkey_check.serialized_extended_pubkey,
- sizeof(decoded_pubkey_check.serialized_extended_pubkey));
+ *out = key_info.ext_pubkey;
 
  return key_info.has_wildcard ? 1 : 0;
 }
@@ -1376,19 +1363,16 @@ bool is_wallet_policy_standard(dispatcher_context_t *dispatcher_context,
  }
 
  // generate pubkey and check if it matches
- char pubkey_derived[MAX_SERIALIZED_PUBKEY_LENGTH + 1];
- int serialized_pubkey_len =
- get_serialized_extended_pubkey_at_path(key_info.master_key_derivation,
- key_info.master_key_derivation_len,
- BIP32_PUBKEY_VERSION,
- pubkey_derived,
- NULL);
- if (serialized_pubkey_len == -1) {
+ serialized_extended_pubkey_t derived_pubkey;
+ if (0 > get_extended_pubkey_at_path(key_info.master_key_derivation,
+ key_info.master_key_derivation_len,
+ BIP32_PUBKEY_VERSION,
+ &derived_pubkey)) {
  PRINTF("Failed to derive pubkey\n");
  return false;
  }
 
- if (strncmp(key_info.ext_pubkey, pubkey_derived, MAX_SERIALIZED_PUBKEY_LENGTH) != 0) {
+ if (memcmp(&key_info.ext_pubkey, &derived_pubkey, sizeof(derived_pubkey)) != 0) {
  return false;
  }
 
@@ -1661,13 +1645,13 @@ int get_key_placeholder_by_index(const policy_node_t *policy,
  return -1;
 }
 
-// Utility function to extract the i-th xpub from the keys information vector
-static int get_xpub_from_merkle_tree(dispatcher_context_t *dispatcher_context,
- int wallet_version,
- const uint8_t keys_merkle_root[static 32],
- uint32_t n_keys,
- uint32_t index,
- char out[static MAX_SERIALIZED_PUBKEY_LENGTH + 1]) {
+// Utility function to extract and decode the i-th xpub from the keys information vector
+static int get_pubkey_from_merkle_tree(dispatcher_context_t *dispatcher_context,
+  int wallet_version,
+  const uint8_t keys_merkle_root[static 32],
+  uint32_t n_keys,
+  uint32_t index,
+  serialized_extended_pubkey_t *out) {
  char key_info_str[MAX_POLICY_KEY_INFO_LEN];
  int key_info_len = call_get_merkle_leaf_element(dispatcher_context,
  keys_merkle_root,
@@ -1686,7 +1670,7 @@ static int get_xpub_from_merkle_tree(dispatcher_context_t *dispatcher_context,
  if (parse_policy_map_key_info(&key_info_buffer, &key_info, wallet_version) == -1) {
  return WITH_ERROR(-1, "Failed to parse key information");
  }
- strncpy(out, key_info.ext_pubkey, MAX_SERIALIZED_PUBKEY_LENGTH + 1);
+ *out = key_info.ext_pubkey;
  return 0;
 }
 
@@ -1750,28 +1734,33 @@ int is_policy_sane(dispatcher_context_t *dispatcher_context,
 
  // check that all the xpubs are different
  for (unsigned int i = 0; i < n_keys - 1; i++) { // no point in running this for the last key
- char xpub_i[MAX_SERIALIZED_PUBKEY_LENGTH + 1];
- if (0 > get_xpub_from_merkle_tree(dispatcher_context,
- wallet_version,
- keys_merkle_root,
- n_keys,
- i,
- xpub_i)) {
+ serialized_extended_pubkey_t pubkey_i;
+ if (0 > get_pubkey_from_merkle_tree(dispatcher_context,
+  wallet_version,
+  keys_merkle_root,
+  n_keys,
+  i,
+  &pubkey_i)) {
  return -1;
  }
 
  for (unsigned int j = i + 1; j < n_keys; j++) {
- char xpub_j[MAX_SERIALIZED_PUBKEY_LENGTH + 1];
- if (0 > get_xpub_from_merkle_tree(dispatcher_context,
- wallet_version,
- keys_merkle_root,
- n_keys,
- j,
- xpub_j)) {
+ serialized_extended_pubkey_t pubkey_j;
+ if (0 > get_pubkey_from_merkle_tree(dispatcher_context,
+  wallet_version,
+  keys_merkle_root,
+  n_keys,
+  j,
+  &pubkey_j)) {
  return -1;
  }
 
- if (strncmp(xpub_i, xpub_j, sizeof(xpub_i)) == 0) {
+ // We reject if any two xpubs have the same pubkey
+ // Conservatively, we only compare the compressed pubkey, rather than the whole xpub:
+ // there is no good reason for allowing two different xpubs with the same pubkey.
+ if (memcmp(pubkey_i.compressed_pubkey,
+ pubkey_j.compressed_pubkey,
+ sizeof(pubkey_i.compressed_pubkey)) == 0) {
  // duplicated pubkey
  return WITH_ERROR(-1, "Repeated pubkey in wallet policy");
  }

src/handler/register_wallet.c

@@ -170,20 +170,19 @@ void handler_register_wallet(dispatcher_context_t *dc, uint8_t protocol_version)
  if (key_info.has_key_origin &&
  read_u32_be(key_info.master_key_fingerprint, 0) == master_key_fingerprint) {
  // we verify that we can actually generate the same pubkey
- char pubkey_derived[MAX_SERIALIZED_PUBKEY_LENGTH + 1];
+ serialized_extended_pubkey_t pubkey_derived;
  int serialized_pubkey_len =
- get_serialized_extended_pubkey_at_path(key_info.master_key_derivation,
- key_info.master_key_derivation_len,
- BIP32_PUBKEY_VERSION,
- pubkey_derived,
- NULL);
+ get_extended_pubkey_at_path(key_info.master_key_derivation,
+ key_info.master_key_derivation_len,
+ BIP32_PUBKEY_VERSION,
+ &pubkey_derived);
  if (serialized_pubkey_len == -1) {
  SEND_SW(dc, SW_BAD_STATE);
  ui_post_processing_confirm_wallet_registration(dc, false);
  return;
  }
 
- if (strncmp(key_info.ext_pubkey, pubkey_derived, MAX_SERIALIZED_PUBKEY_LENGTH) == 0) {
+ if (memcmp(&key_info.ext_pubkey, &pubkey_derived, sizeof(pubkey_derived)) == 0) {
  is_key_internal = true;
  ++n_internal_keys;
  }