From d2cf21fc2bcf79f07b7e41178cd3e4ca9e3d9f68 Mon Sep 17 00:00:00 2001 From: SomberNight Date: Thu, 11 May 2023 09:55:19 +0000 Subject: [PATCH] qml wizard: enforce homogeneous master keys in multisig - {xpub, Ypub, Zpub} categories cannot be mixed - old mpk must not be used in multisig --- electrum/bip32.py | 3 +- .../gui/qml/components/NewWalletWizard.qml | 4 ++ .../qml/components/wizard/WCHaveMasterKey.qml | 6 ++- .../gui/qml/components/wizard/WCHaveSeed.qml | 5 ++- electrum/gui/qml/qebitcoin.py | 32 +++++++++------ electrum/gui/qml/qewizard.py | 12 ++++-- electrum/wizard.py | 39 ++++++++++++++++--- 7 files changed, 77 insertions(+), 24 deletions(-) diff --git a/electrum/bip32.py b/electrum/bip32.py index 248abef21..85cc27fdb 100644 --- a/electrum/bip32.py +++ b/electrum/bip32.py @@ -289,7 +289,8 @@ class BIP32Node(NamedTuple): return hash_160(self.eckey.get_public_key_bytes(compressed=True))[0:4] -def xpub_type(x): +def xpub_type(x: str): + assert x is not None return BIP32Node.from_xkey(x).xtype diff --git a/electrum/gui/qml/components/NewWalletWizard.qml b/electrum/gui/qml/components/NewWalletWizard.qml index efb8e5867..e7f5e53d3 100644 --- a/electrum/gui/qml/components/NewWalletWizard.qml +++ b/electrum/gui/qml/components/NewWalletWizard.qml @@ -33,6 +33,10 @@ Wizard { walletwizard.path = wiz.path walletwizard.walletCreated() } + function onCreateError(error) { + var dialog = app.messageDialog.createObject(app, { text: error }) + dialog.open() + } } } diff --git a/electrum/gui/qml/components/wizard/WCHaveMasterKey.qml b/electrum/gui/qml/components/wizard/WCHaveMasterKey.qml index cf6452432..cea7bfb46 100644 --- a/electrum/gui/qml/components/wizard/WCHaveMasterKey.qml +++ b/electrum/gui/qml/components/wizard/WCHaveMasterKey.qml @@ -42,10 +42,14 @@ WizardComponent { if (cosigner) { applyMasterKey(key) - if (wiz.hasDuplicateKeys(wizard_data)) { + if (wiz.hasDuplicateMasterKeys(wizard_data)) { validationtext.text = qsTr('Error: duplicate master public key') return false } + if (wiz.hasHeterogeneousMasterKeys(wizard_data)) { + validationtext.text = qsTr('Error: master public key types do not match') + return false + } } return valid = true diff --git a/electrum/gui/qml/components/wizard/WCHaveSeed.qml b/electrum/gui/qml/components/wizard/WCHaveSeed.qml index 35069f37c..29229641e 100644 --- a/electrum/gui/qml/components/wizard/WCHaveSeed.qml +++ b/electrum/gui/qml/components/wizard/WCHaveSeed.qml @@ -65,9 +65,12 @@ WizardComponent { return } else { apply() - if (wiz.hasDuplicateKeys(wizard_data)) { + if (wiz.hasDuplicateMasterKeys(wizard_data)) { validationtext.text = qsTr('Error: duplicate master public key') return + } else if (wiz.hasHeterogeneousMasterKeys(wizard_data)) { + validationtext.text = qsTr('Error: master public key types do not match') + return } else { valid = true } diff --git a/electrum/gui/qml/qebitcoin.py b/electrum/gui/qml/qebitcoin.py index c03eecb17..95897d39f 100644 --- a/electrum/gui/qml/qebitcoin.py +++ b/electrum/gui/qml/qebitcoin.py @@ -113,22 +113,30 @@ class QEBitcoin(QObject): return False k = keystore.from_master_key(key) - if isinstance(k, keystore.Xpub): # has xpub # TODO are these checks useful? - t1 = xpub_type(k.xpub) - if wallet_type == 'standard': - if t1 not in ['standard', 'p2wpkh', 'p2wpkh-p2sh']: - self.validationMessage = '%s: %s' % (_('Wrong key type'), t1) - return False - return True - elif wallet_type == 'multisig': - if t1 not in ['standard', 'p2wsh', 'p2wsh-p2sh']: + if wallet_type == 'standard': + if isinstance(k, keystore.Xpub): # has bip32 xpub + t1 = xpub_type(k.xpub) + if t1 not in ['standard', 'p2wpkh', 'p2wpkh-p2sh']: # disallow Ypub/Zpub self.validationMessage = '%s: %s' % (_('Wrong key type'), t1) return False - return True + elif isinstance(k, keystore.Old_KeyStore): + pass else: - self.validationMessage = '%s: %s' % (_('Unsupported wallet type'), wallet_type) - self.logger.error(f'Unsupported wallet type: {wallet_type}') + self._logger.error(f"unexpected keystore type: {type(keystore)}") + return False + elif wallet_type == 'multisig': + if not isinstance(k, keystore.Xpub): # old mpk? + self.validationMessage = '%s: %s' % (_('Wrong key type'), "not bip32") return False + t1 = xpub_type(k.xpub) + if t1 not in ['standard', 'p2wsh', 'p2wsh-p2sh']: # disallow ypub/zpub + self.validationMessage = '%s: %s' % (_('Wrong key type'), t1) + return False + else: + self.validationMessage = '%s: %s' % (_('Unsupported wallet type'), wallet_type) + self._logger.error(f'Unsupported wallet type: {wallet_type}') + return False + # looks okay return True @pyqtSlot(str, result=bool) diff --git a/electrum/gui/qml/qewizard.py b/electrum/gui/qml/qewizard.py index 5b26fa488..43d0108ce 100644 --- a/electrum/gui/qml/qewizard.py +++ b/electrum/gui/qml/qewizard.py @@ -83,10 +83,16 @@ class QENewWalletWizard(NewWalletWizard, QEAbstractWizard): return self._daemon.singlePasswordEnabled @pyqtSlot('QJSValue', result=bool) - def hasDuplicateKeys(self, js_data): - self._logger.info('Checking for duplicate keys') + def hasDuplicateMasterKeys(self, js_data): + self._logger.info('Checking for duplicate masterkeys') data = js_data.toVariant() - return self.has_duplicate_keys(data) + return self.has_duplicate_masterkeys(data) + + @pyqtSlot('QJSValue', result=bool) + def hasHeterogeneousMasterKeys(self, js_data): + self._logger.info('Checking for heterogeneous masterkeys') + data = js_data.toVariant() + return self.has_heterogeneous_masterkeys(data) @pyqtSlot('QJSValue', bool, str) def createStorage(self, js_data, single_password_enabled, single_password): diff --git a/electrum/wizard.py b/electrum/wizard.py index 072aa4534..65ac64244 100644 --- a/electrum/wizard.py +++ b/electrum/wizard.py @@ -305,18 +305,38 @@ class NewWalletWizard(AbstractWizard): return True - def has_duplicate_keys(self, wizard_data): + def has_duplicate_masterkeys(self, wizard_data) -> bool: + """Multisig wallets need distinct master keys. If True, need to prevent wallet-creation.""" xpubs = [] xpubs.append(self.keystore_from_data(wizard_data).get_master_public_key()) for cosigner in wizard_data['multisig_cosigner_data']: data = wizard_data['multisig_cosigner_data'][cosigner] xpubs.append(self.keystore_from_data(data).get_master_public_key()) - - while len(xpubs): - xpub = xpubs.pop() - if xpub in xpubs: + assert xpubs + return len(xpubs) != len(set(xpubs)) + + def has_heterogeneous_masterkeys(self, wizard_data) -> bool: + """Multisig wallets need homogeneous master keys. + All master keys need to be bip32, and e.g. Ypub cannot be mixed with Zpub. + If True, need to prevent wallet-creation. + """ + xpubs = [] + xpubs.append(self.keystore_from_data(wizard_data).get_master_public_key()) + for cosigner in wizard_data['multisig_cosigner_data']: + data = wizard_data['multisig_cosigner_data'][cosigner] + xpubs.append(self.keystore_from_data(data).get_master_public_key()) + assert xpubs + try: + k_xpub_type = xpub_type(xpubs[0]) + except Exception: + return True # maybe old_mpk? + for xpub in xpubs: + try: + my_xpub_type = xpub_type(xpub) + except Exception: + return True # maybe old_mpk? + if my_xpub_type != k_xpub_type: return True - return False def keystore_from_data(self, data): @@ -422,10 +442,17 @@ class NewWalletWizard(AbstractWizard): db.put('x3/', data['x3/']) db.put('use_trustedcoin', True) elif data['wallet_type'] == 'multisig': + if not isinstance(k, keystore.Xpub): + raise Exception(f"unexpected keystore(main) type={type(k)} in multisig. not bip32.") + k_xpub_type = xpub_type(k.xpub) db.put('wallet_type', '%dof%d' % (data['multisig_signatures'],data['multisig_participants'])) db.put('x1/', k.dump()) for cosigner in data['multisig_cosigner_data']: cosigner_keystore = self.keystore_from_data(data['multisig_cosigner_data'][cosigner]) + if not isinstance(cosigner_keystore, keystore.Xpub): + raise Exception(f"unexpected keystore(cosigner) type={type(cosigner_keystore)} in multisig. not bip32.") + if k_xpub_type != xpub_type(cosigner_keystore.xpub): + raise Exception("multisig wallet needs to have homogeneous xpub types") if data['encrypt'] and cosigner_keystore.may_have_password(): cosigner_keystore.update_password(None, data['password']) db.put(f'x{cosigner}/', cosigner_keystore.dump())