multisig: explicitly check cosigner xpubs are valid base58 xpub strings

Blockstream · Feb 2, 2024 · ca6ea18 · ca6ea18
1 parent b692387
commit ca6ea18
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 9 deletions.
diff --git a/main/signer.c b/main/signer.c
@@ -42,19 +42,21 @@ bool validate_signers(const signer_t* signers, const size_t num_signers, const b
             }
         }
 
-        // See if signer that matches this wallet (by fingerprint)
+        // Check all given xpubs can be parsed
+        const uint32_t flags = BIP32_FLAG_KEY_PUBLIC | BIP32_FLAG_SKIP_HASH;
+        struct ext_key hdkey_provided;
+        if (!wallet_derive_from_xpub(signer->xpub, NULL, 0, flags, &hdkey_provided)) {
+            JADE_LOGE("Cannot deserialise xpub for derivation path (signer %d)", i);
+            return false;
+        }
+
+        // See if signer matches this wallet (by fingerprint)
         if (!sodium_memcmp(wallet_fingerprint, signer->fingerprint, wallet_fingerprint_len)) {
             // This signer has our fingerprint - check xpub provided
             // NOTE: because some 3rd-party apps provide xpubs which are slightly incorrect in their ancilliary
             // metadata fields, we can't strictly compare xpub strings without affecting compatabililty.
             // Instead we deserialise the provided xpub string and compare pubkey and chaincode only.
             // NOTE: a mismatch here is not a hard fail as could be a fingerprint clash with another signer
-            const uint32_t flags = BIP32_FLAG_KEY_PUBLIC | BIP32_FLAG_SKIP_HASH;
-            struct ext_key hdkey_provided;
-            if (!wallet_derive_from_xpub(signer->xpub, NULL, 0, flags, &hdkey_provided)) {
-                JADE_LOGE("Cannot deserialise xpub for derivation path (signer %d)", i);
-                return false;
-            }
             struct ext_key hdkey_calculated;
             if (!wallet_get_hdkey(signer->derivation, signer->derivation_len, flags, &hdkey_calculated)) {
                 JADE_LOGE("Cannot derive key for derivation path (signer %d)", i);

diff --git a/test_jade.py b/test_jade.py
@@ -842,6 +842,10 @@ def test_bad_params(jade):
     bad_multi_cosigners3[1]['derivation'] = [1, 2, 3, 4]
     bad_multi_cosigners4 = copy.deepcopy(MULTI_COSIGNERS)
     bad_multi_cosigners4[1]['path'] = [2147483648]
+    bad_multi_cosigners5 = copy.deepcopy(MULTI_COSIGNERS)
+    bad_multi_cosigners5[1]['xpub'] = bad_multi_cosigners2[0]['xpub']
+    bad_multi_cosigners6 = copy.deepcopy(MULTI_COSIGNERS)
+    bad_multi_cosigners6[0]['xpub'] = bad_multi_cosigners6[0]['xpub'].replace('D', 'E', 1)
 
     DESCRIPTOR = 'wsh(pkh(@0/<0;1>/*))'
     DESCR_SIGNER = "[e3ebcc79/48'/1'/0'/2']tpubDDvj9CrVJ9kWXSL2kjtA8v53rZvTmL3\
@@ -1022,16 +1026,24 @@ def test_bad_params(jade):
                       'variant': 'sh(multi(k))', 'threshold': 2,
                       'signers': bad_multi_cosigners4}}), 'Failed to validate co-signers'),
                   (('badmulti19', 'register_multisig',
+                    {'network': 'testnet', 'multisig_name': 'test', 'descriptor': {
+                      'variant': 'sh(multi(k))', 'threshold': 2,
+                      'signers': bad_multi_cosigners5}}), 'Failed to validate co-signers'),
+                  (('badmulti20', 'register_multisig',
+                    {'network': 'testnet', 'multisig_name': 'test', 'descriptor': {
+                      'variant': 'sh(multi(k))', 'threshold': 2,
+                      'signers': bad_multi_cosigners6}}), 'Failed to validate co-signers'),
+                  (('badmulti21', 'register_multisig',
                     {'network': 'testnet', 'multisig_name': 'test', 'descriptor': {
                       'variant': 'sh(wsh(multi(k)))', 'threshold': 1, 'signers': MULTI_COSIGNERS,
                       'master_blinding_key': 1234}}),
                    'Invalid blinding key'),
-                  (('badmulti20', 'register_multisig',
+                  (('badmulti22', 'register_multisig',
                     {'network': 'testnet', 'multisig_name': 'test', 'descriptor': {
                       'variant': 'wsh(multi(k))', 'threshold': 1, 'signers': MULTI_COSIGNERS,
                       'master_blinding_key': 'abcdef'}}),
                    'Invalid blinding key'),
-                  (('badmulti21', 'register_multisig',
+                  (('badmulti23', 'register_multisig',
                     {'network': 'testnet', 'multisig_name': 'test', 'descriptor': {
                       'variant': 'sh(wsh(multi(k)))', 'threshold': 1, 'signers': MULTI_COSIGNERS,
                       'master_blinding_key': EXPECTED_MASTER_BLINDING_KEY[:-1]}}),