Merge pull request #241 from LedgerHQ/check_pubkey_version

Check pubkey version

src/crypto.h

@@ -252,7 +252,7 @@ uint32_t crypto_get_key_fingerprint(const uint8_t pub_key[static 33]);
 uint32_t crypto_get_master_key_fingerprint();
 
 /**
- * Computes the base58check-encoded extended pubkey at a given path.
+ * Computes extended pubkey at a given path, serialized as per BIP32.
  *
  * @param[in]  bip32_path
  *   Pointer to 32-bit array of BIP-32 derivation steps.

src/handler/register_wallet.c

@@ -159,6 +159,12 @@ void handler_register_wallet(dispatcher_context_t *dc, uint8_t protocol_version)
             return;
         }
 
+        if (read_u32_be(key_info.ext_pubkey.version, 0) != BIP32_PUBKEY_VERSION) {
+            PRINTF("Invalid pubkey version. Wrong network?\n");
+            SEND_SW(dc, SW_INCORRECT_DATA);
+            return;
+        }
+
         // We refuse to register wallets without key origin information, or whose keys don't end
         // with the wildcard ('/**'). The key origin information is necessary when signing to
         // identify which one is our key. Using addresses without a wildcard could potentially be
@@ -189,10 +195,6 @@ void handler_register_wallet(dispatcher_context_t *dc, uint8_t protocol_version)
             }
         }
 
-        // TODO: it would be sensible to validate the pubkey (at least syntactically + validate
-        // checksum)
-        //       Currently we are showing to the user whichever string is passed by the host.
-
         if (!ui_display_policy_map_cosigner_pubkey(dc,
                                                    (char *) next_pubkey_info,
                                                    cosigner_index,  // 1-indexed for the UI

tests/test_register_wallet.py

@@ -92,6 +92,24 @@ def test_register_wallet_reject_header(client: Client):
         client.register_wallet(wallet)
 
 
+@has_automation("automations/register_wallet_accept.json")
+def test_register_wallet_invalid_pubkey_version(client: Client):
+    # This is the same wallet policy as the test_register_wallet_accept_wit test,
+    # but the external pubkey has the wrong BIP32 version (mainnet xpub instead of testnet tpub).
+    # An older version of the app ignored the version for external pubkeys, while now it rejects it
+    # if the version is wrong, as a sanity check.
+    with pytest.raises(IncorrectDataError):
+        client.register_wallet(MultisigWallet(
+            name="Cold storage",
+            address_type=AddressType.WIT,
+            threshold=2,
+            keys_info=[
+                "[76223a6e/48'/1'/0'/2']xpub6DjjtjxALtJSP9dKRKuhejeTpZc711gUGZyS9nCM5GAtrNTDuMBZD2FcndJoHst6LYNbJktm4NmJyKqspLi5uRmtnDMAdcPAf2jiSj9gFTX",
+                "[f5acc2fd/48'/1'/0'/2']tpubDFAqEGNyad35aBCKUAXbQGDjdVhNueno5ZZVEn3sQbW5ci457gLR7HyTmHBg93oourBssgUxuWz1jX5uhc1qaqFo9VsybY1J5FuedLfm4dK",
+            ],
+        ))
+
+
 @has_automation("automations/register_wallet_accept.json")
 def test_register_wallet_invalid_names(client: Client):
     too_long_name = "This wallet name is much too long since it requires 65 characters"