From a51ba9d65abfebc895092748118776845dbdeaf7 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Thu, 24 Feb 2022 13:51:10 -0500 Subject: [PATCH 1/3] bitbox01: Limit each signing attempt to 15 sigs The simulator is having trouble with anything larger, so the device probably will too. We change the signing to do 15 signing bundles at a time rather than all them at the same time. --- hwilib/devices/digitalbitbox.py | 75 +++++++++++++++++---------------- 1 file changed, 39 insertions(+), 36 deletions(-) diff --git a/hwilib/devices/digitalbitbox.py b/hwilib/devices/digitalbitbox.py index 15c8862cd..7264fb92a 100644 --- a/hwilib/devices/digitalbitbox.py +++ b/hwilib/devices/digitalbitbox.py @@ -494,42 +494,45 @@ def sign_tx(self, tx: PSBT) -> PSBT: if len(sighash_tuples) == 0: return tx - # Sign the sighashes - to_send = '{"sign":{"data":[' - for tup in sighash_tuples: - to_send += '{"hash":"' - to_send += tup[0] - to_send += '","keypath":"' - to_send += tup[1] - to_send += '"},' - if to_send[-1] == ',': - to_send = to_send[:-1] - to_send += ']}}' - logging.debug(to_send) - - reply = send_encrypt(to_send, self.password, self.device) - logging.debug(reply) - if 'error' in reply: - raise DBBError(reply) - print("Touch the device for 3 seconds to sign. Touch briefly to cancel", file=sys.stderr) - reply = send_encrypt(to_send, self.password, self.device) - logging.debug(reply) - if 'error' in reply: - raise DBBError(reply) - - # Extract sigs - sigs = [] - for item in reply['sign']: - sigs.append(binascii.unhexlify(item['sig'])) - - # Make sigs der - der_sigs = [] - for sig in sigs: - der_sigs.append(ser_sig_der(sig[0:32], sig[32:64])) - - # add sigs to tx - for tup, sig in zip(sighash_tuples, der_sigs): - tx.inputs[tup[2]].partial_sigs[tup[3]] = sig + for i in range(0, len(sighash_tuples), 15): + tups = sighash_tuples[i:i + 15] + + # Sign the sighashes + to_send = '{"sign":{"data":[' + for tup in tups: + to_send += '{"hash":"' + to_send += tup[0] + to_send += '","keypath":"' + to_send += tup[1] + to_send += '"},' + if to_send[-1] == ',': + to_send = to_send[:-1] + to_send += ']}}' + logging.debug(to_send) + + reply = send_encrypt(to_send, self.password, self.device) + logging.debug(reply) + if 'error' in reply: + raise DBBError(reply) + print("Touch the device for 3 seconds to sign. Touch briefly to cancel", file=sys.stderr) + reply = send_encrypt(to_send, self.password, self.device) + logging.debug(reply) + if 'error' in reply: + raise DBBError(reply) + + # Extract sigs + sigs = [] + for item in reply['sign']: + sigs.append(binascii.unhexlify(item['sig'])) + + # Make sigs der + der_sigs = [] + for sig in sigs: + der_sigs.append(ser_sig_der(sig[0:32], sig[32:64])) + + # add sigs to tx + for tup, sig in zip(tups, der_sigs): + tx.inputs[tup[2]].partial_sigs[tup[3]] = sig return tx From 1f3f1880e7e97d1e229333fb94619ed3b5abb5f0 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Wed, 23 Feb 2022 15:53:03 -0500 Subject: [PATCH 2/3] tests: Change test_big_tx to use our inputs Use inputs belonging to the device instead of external inputs. Apparently this fixes the coldcard problem with this test too. --- test/test_device.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/test/test_device.py b/test/test_device.py index 028c61fdb..a86076bce 100644 --- a/test/test_device.py +++ b/test/test_device.py @@ -572,26 +572,27 @@ def test_signtx(self): # Make a huge transaction which might cause some problems with different interfaces def test_big_tx(self): - # make a huge transaction that is unrelated to the hardware wallet + # make a huge transaction + keypool_desc = self.do_command(self.dev_args + ["getkeypool", "--account", "10", "--addr-type", "legacy", "0", "100"]) + import_result = self.wrpc.importdescriptors(keypool_desc) + self.assertTrue(import_result[0]['success']) outputs = [] num_inputs = 60 for i in range(0, num_inputs): - outputs.append({self.wpk_rpc.getnewaddress('', 'legacy'): 0.001}) + outputs.append({self.wrpc.getnewaddress('', 'legacy'): 0.001}) + outputs.append({self.wrpc.getnewaddress("", "legacy"): 10}) psbt = self.wpk_rpc.walletcreatefundedpsbt([], outputs, 0, {}, True)['psbt'] psbt = self.wpk_rpc.walletprocesspsbt(psbt)['psbt'] tx = self.wpk_rpc.finalizepsbt(psbt)['hex'] - txid = self.wpk_rpc.sendrawtransaction(tx) - inputs = [] - for i in range(0, num_inputs): - inputs.append({'txid': txid, 'vout': i}) - psbt = self.wpk_rpc.walletcreatefundedpsbt(inputs, [{self.wpk_rpc.getnewaddress('', 'legacy'): 0.001 * num_inputs}], 0, {'subtractFeeFromOutputs': [0]}, True)['psbt'] + self.wpk_rpc.sendrawtransaction(tx) + self.wpk_rpc.generatetoaddress(10, self.wpk_rpc.getnewaddress()) + inputs = self.wrpc.listunspent() + psbt = self.wrpc.walletcreatefundedpsbt(inputs, [{self.wpk_rpc.getnewaddress('', 'legacy'): 0.001 * num_inputs}])['psbt'] # For cli, this should throw an exception try: result = self.do_command(self.dev_args + ['signtx', psbt]) if self.interface == 'cli': self.fail('Big tx did not cause CLI to error') - if self.emulator.type == 'coldcard': - self.assertEqual(result['code'], -7) else: self.assertNotIn('code', result) self.assertNotIn('error', result) From 7b0eced86f2778fbf42ca638500a3ca847a80261 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Wed, 23 Feb 2022 15:28:02 -0500 Subject: [PATCH 3/3] trezor: Disallow external inputs for future firmware versions Trezor is going to close the loophole that we use to get external input support, so we need to remove trying to sign with that loophole for future firmware versions. --- docs/devices/index.rst | 5 +++- hwilib/devices/trezor.py | 53 ++++++++++++++++++++++++++-------------- test/test_trezor.py | 6 ++--- 3 files changed, 41 insertions(+), 23 deletions(-) diff --git a/docs/devices/index.rst b/docs/devices/index.rst index 17e6751d0..764e4bf91 100644 --- a/docs/devices/index.rst +++ b/docs/devices/index.rst @@ -51,13 +51,16 @@ The table below lists what devices and features are supported for each device. +------------------------------------+---------------+---------------+------------+----------------+----------+----------+---------+----------+------------------+ | Arbitrary witnessScript Inputs | ✓ | ✓ | ― | ― | ✓ | ― | ― | ― | ✓ | +------------------------------------+---------------+---------------+------------+----------------+----------+----------+---------+----------+------------------+ -| Non-wallet inputs | ✓ | ✓ | ✓ | ✓ | ✓ | ― | ✓ | ✓ | ✓ | +| Non-wallet inputs | ✓ | ✓ | ✗\ :sup:`1` | ✗\ :sup:`2` | ✓ | ― | ✓ | ✓ | ✓ | +------------------------------------+---------------+---------------+------------+----------------+----------+----------+---------+----------+------------------+ | Mixed Segwit and Non-Segwit Inputs | ― | ― | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ | +------------------------------------+---------------+---------------+------------+----------------+----------+----------+---------+----------+------------------+ | Display on device screen | ✓ | ✓ | ✓ | ✓ | ― | ✓ | ✓ | ✓ | ✓ | +------------------------------------+---------------+---------------+------------+----------------+----------+----------+---------+----------+------------------+ +* 1 - Support removed for devices with firmware 1.10.6 and greater. +* 2 - Support removed for devices with firmware 2.4.4 and greater. + \* There are some caveats. See the `sign_tx` for these devices. Support Policy diff --git a/hwilib/devices/trezor.py b/hwilib/devices/trezor.py index 70415df28..4d57878b0 100644 --- a/hwilib/devices/trezor.py +++ b/hwilib/devices/trezor.py @@ -327,6 +327,15 @@ def _check_unlocked(self) -> None: if self.client.features.pin_protection and not self.client.features.unlocked: raise DeviceNotReadyError('{} is locked. Unlock by using \'promptpin\' and then \'sendpin\'.'.format(self.type)) + def _supports_external(self) -> bool: + if self.client.features.model == "1" and self.client.version <= (1, 10, 5): + return True + if self.client.features.model == "T" and self.client.version <= (2, 4, 3): + return True + if self.client.features.model == "K1-14AM": + return True + return False + @trezor_exception def get_pubkey_at_path(self, path: str) -> ExtendedKey: self._check_unlocked() @@ -364,7 +373,6 @@ def sign_tx(self, tx: PSBT) -> PSBT: # Prepare inputs inputs = [] to_ignore = [] # Note down which inputs whose signatures we're going to ignore - has_tr = False for input_num, psbt_in in builtins.enumerate(tx.inputs): assert psbt_in.prev_txid is not None assert psbt_in.prev_out is not None @@ -439,14 +447,20 @@ def ignore_input() -> None: txinputtype.script_type = messages.InputScriptType.SPENDMULTISIG else: # Cannot sign bare multisig, ignore it + if not self._supports_external(): + raise BadArgumentError("Cannot sign bare multisig") ignore_input() continue elif not is_ms and not is_wit and not is_p2pkh(scriptcode): # Cannot sign unknown spk, ignore it + if not self._supports_external(): + raise BadArgumentError("Cannot sign unknown scripts") ignore_input() continue elif not is_ms and is_wit and p2wsh: # Cannot sign unknown witness script, ignore it + if not self._supports_external(): + raise BadArgumentError("Cannot sign unknown witness versions") ignore_input() continue @@ -454,10 +468,12 @@ def ignore_input() -> None: found = False # Whether we have found a key to sign with found_in_sigs = False # Whether we have found one of our keys in the signatures our_keys = 0 + path_last_ours = None # The path of the last key that is ours. We will use this if we need to ignore this input because it is already signed. if txinputtype.script_type in ECDSA_SCRIPT_TYPES: for key in psbt_in.hd_keypaths.keys(): keypath = psbt_in.hd_keypaths[key] if keypath.fingerprint == master_fp: + path_last_ours = keypath.path if key in psbt_in.partial_sigs: # This key already has a signature found_in_sigs = True continue @@ -466,17 +482,15 @@ def ignore_input() -> None: found = True our_keys += 1 elif txinputtype.script_type in SCHNORR_SCRIPT_TYPES: - if len(psbt_in.tap_key_sig) > 0: - found_in_sigs = True - else: - for key, (leaf_hashes, origin) in psbt_in.tap_bip32_paths.items(): - # TODO: Support script path signing - if key == psbt_in.tap_internal_key and origin.fingerprint == master_fp: - has_tr = True - txinputtype.address_n = origin.path - found = True - our_keys += 1 - break + found_in_sigs = len(psbt_in.tap_key_sig) > 0 + for key, (leaf_hashes, origin) in psbt_in.tap_bip32_paths.items(): + # TODO: Support script path signing + if key == psbt_in.tap_internal_key and origin.fingerprint == master_fp: + path_last_ours = origin.path + txinputtype.address_n = origin.path + found = True + our_keys += 1 + break # Determine if we need to do more passes to sign everything if our_keys > passes: @@ -484,19 +498,20 @@ def ignore_input() -> None: if not found and not found_in_sigs: # None of our keys were in hd_keypaths or in partial_sigs # This input is not one of ours + if not self._supports_external(): + raise BadArgumentError("Cannot sign external inputs") ignore_input() continue - elif not found and found_in_sigs: # All of our keys are in partial_sigs, ignore whatever signature is produced for this input - ignore_input() - continue + elif not found and found_in_sigs: + # All of our keys are in partial_sigs, pick the first key that is ours, sign with it, + # and ignore whatever signature is produced for this input + assert path_last_ours is not None + txinputtype.address_n = path_last_ours + to_ignore.append(input_num) # append to inputs inputs.append(txinputtype) - # Cannot sign transactions that have external inputs (to_ignore is not empty) and would sign taproot inputs - if has_tr and len(to_ignore) > 0: - raise BadArgumentError("Trezor cannot sign taproot inputs when the transaction also has external inputs") - # address version byte if self.chain != Chain.MAIN: p2pkh_version = b'\x6f' diff --git a/test/test_trezor.py b/test/test_trezor.py index 3fdfac76d..fcce33379 100755 --- a/test/test_trezor.py +++ b/test/test_trezor.py @@ -415,10 +415,10 @@ def trezor_test_suite(emulator, bitcoind, interface, model): dev_emulator = TrezorEmulator(emulator, model) signtx_cases = [ - (["legacy"], ["legacy"], True, True), - (["segwit"], ["segwit"], True, True), + (["legacy"], ["legacy"], False, True), + (["segwit"], ["segwit"], False, True), (["tap"], [], False, True), - (["legacy", "segwit"], ["legacy", "segwit"], True, True), + (["legacy", "segwit"], ["legacy", "segwit"], False, True), (["legacy", "segwit", "tap"], ["legacy", "segwit"], False, True), ]