Skip to content

Commit

Permalink
Merge #581: trezor: Remove support for external inputs
Browse files Browse the repository at this point in the history 
7b0eced trezor: Disallow external inputs for future firmware versions (Andrew Chow)
1f3f188 tests: Change test_big_tx to use our inputs (Andrew Chow)
a51ba9d bitbox01: Limit each signing attempt to 15 sigs (Andrew Chow)

Pull request description:

  Trezor will be closing the loophole that we use to be able to sign external inputs. So we will need to tell users that they will not be able to sign such transactions if they are using those firmware versions.

Top commit has no ACKs.

Tree-SHA512: ac4db2f1c3adadeebd4f6ec914ea59ef307f95ec0e4ca6a970a5a0555beccdce00e721656e0f2bb3778616b49bf8deda493cd98257464f959b7c5e544e75790c
  • Loading branch information
@achow101
achow101 committed Mar 15, 2022
2 parents 3fe369d + 7b0eced commit 63a4afd
Show file tree
Hide file tree
Showing 5 changed files with 90 additions and 68 deletions.
5 changes: 4 additions & 1 deletion docs/devices/index.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
75 changes: 39 additions & 36 deletions hwilib/devices/digitalbitbox.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
53 changes: 34 additions & 19 deletions hwilib/devices/trezor.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -439,25 +447,33 @@ 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

# Find key to sign with
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
Expand All @@ -466,37 +482,36 @@ 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:
passes = our_keys

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'
Expand Down
19 changes: 10 additions & 9 deletions test/test_device.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
6 changes: 3 additions & 3 deletions test/test_trezor.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
]

Expand Down

0 comments on commit 63a4afd

Please sign in to comment.