From ea9d183a4863759ae1627fdf0afc49e4aabdcb3c Mon Sep 17 00:00:00 2001 From: scgbckbone Date: Mon, 5 May 2025 18:51:52 +0200 Subject: [PATCH] bugfix: PUSHDATA2 in scripts cause yikes bugfix: missing warning summary in the top of the story for unknown scripts --- releases/Next-ChangeLog.md | 2 + shared/auth.py | 3 - shared/chains.py | 29 ++++++---- shared/psbt.py | 75 +++++++++++++++++++----- shared/serializations.py | 38 ++++++++---- testing/api.py | 5 +- testing/devtest/unit_script.py | 5 +- testing/helpers.py | 5 ++ testing/test_sign.py | 102 ++++++++++++++++++++++++--------- testing/txn.py | 19 ++++-- 10 files changed, 207 insertions(+), 76 deletions(-) diff --git a/releases/Next-ChangeLog.md b/releases/Next-ChangeLog.md index c3895456..8abd0aef 100644 --- a/releases/Next-ChangeLog.md +++ b/releases/Next-ChangeLog.md @@ -12,6 +12,8 @@ This lists the new changes that have not yet been published in a normal release. ## 5.4.? - 2025-05- - Bugfix: Mk4 with both NFC & Virtual Disk OFF cannot exit `Export Wallet` menu. Stuck in export loop - needs reboot. +- Bugfix: PUSHDATA2 in script caused yikes +- Bugfix: Warning for unknown script was not shown at the top of the signing story # Q Specific Changes diff --git a/shared/auth.py b/shared/auth.py index 1702711c..70c24c8c 100644 --- a/shared/auth.py +++ b/shared/auth.py @@ -306,10 +306,7 @@ class ApproveTransaction(UserAuthorizedAction): return to_ret + "\n" # Handle future things better: allow them to happen at least. - self.psbt.warnings.append( - ('Output?', 'Sending to a script that is not well understood.')) dest = B2A(o.scriptPubKey) - return '%s\n - to script -\n%s\n' % (val, dest) async def interact(self): diff --git a/shared/chains.py b/shared/chains.py index 34c5e2a9..b3f6cc6a 100644 --- a/shared/chains.py +++ b/shared/chains.py @@ -250,24 +250,29 @@ class ChainsBase: @classmethod def op_return(cls, script): - """Returns decoded string op return data if script is op return otherwise None""" + # returns decoded string op return data if script is op return otherwise None gen = disassemble(script) script_type = next(gen) - if OP_RETURN in script_type: - try: - data = next(gen)[0] - if data is None: raise RuntimeError - except (RuntimeError, StopIteration): - return "null-data", "" + if OP_RETURN not in script_type: + return + + try: + data = next(gen)[0] + if data is None: raise RuntimeError + except (RuntimeError, StopIteration): + return "null-data", "" + + data_ascii = None + if len(data) > 200: + # completely arbitrary limit, prevents huge stories + data_hex = b2a_hex(data[:100]).decode() + "\n ⋯\n" + b2a_hex(data[-100:]).decode() + else: data_hex = b2a_hex(data).decode() - data_ascii = None if min(data) >= 32 and max(data) < 127: # printable try: data_ascii = data.decode("ascii") - except: - pass - return data_hex, data_ascii - return None + except: pass + return data_hex, data_ascii @classmethod def possible_address_fmt(cls, addr): diff --git a/shared/psbt.py b/shared/psbt.py index cfd35647..8e46ba01 100644 --- a/shared/psbt.py +++ b/shared/psbt.py @@ -18,7 +18,7 @@ from serializations import CTxIn, CTxInWitness, CTxOut, ser_string, ser_uint256, from serializations import ser_sig_der, uint256_from_str, ser_push_data from serializations import SIGHASH_ALL, SIGHASH_SINGLE, SIGHASH_NONE, SIGHASH_ANYONECANPAY from serializations import ALL_SIGHASH_FLAGS -from opcodes import OP_CHECKMULTISIG +from opcodes import OP_CHECKMULTISIG, OP_RETURN from glob import settings from public_constants import ( @@ -400,13 +400,20 @@ class psbtOutputProxy(psbtProxy): # num_ours = self.parse_subpaths(my_xfp, parent.warnings) - if num_ours == 0: + # - must match expected address for this output, coming from unsigned txn + af, addr_or_pubkey, is_segwit = txo.get_address() + + if (num_ours == 0) or (af in ["p2tr", "op_return", None]): + # num_ours == 0 # - not considered fraud because other signers looking at PSBT may have them # - user will see them as normal outputs, which they are from our PoV. - return - - # - must match expected address for this output, coming from unsigned txn - addr_type, addr_or_pubkey, is_segwit = txo.get_address() + # OP_RETURN + # - nothing we can do with anchor outputs + # UNKNOWN + # - scripts that we do not understand + # P2TR + # - unsupported, will be properly rendered as address (no change check) + return af if len(self.subpaths) == 1: # p2pk, p2pkh, p2wpkh cases @@ -415,7 +422,7 @@ class psbtOutputProxy(psbtProxy): # p2wsh/p2sh cases need full set of pubkeys, and therefore redeem script expect_pubkey = None - if addr_type == 'p2pk': + if af == 'p2pk': # output is public key (not a hash, much less common) assert len(addr_or_pubkey) == 33 @@ -423,12 +430,12 @@ class psbtOutputProxy(psbtProxy): raise FraudulentChangeOutput(out_idx, "P2PK change output is fraudulent") self.is_change = True - return + return af # Figure out what the hashed addr should be pkh = addr_or_pubkey - if addr_type == 'p2sh': + if af == 'p2sh': # P2SH or Multisig output # Can be both, or either one depending on address type @@ -473,13 +480,13 @@ class psbtOutputProxy(psbtProxy): # - might be a p2sh output for another wallet that isn't us # - not fraud, just an output with more details than we need. self.is_change = False - return + return af if MultisigWallet.disable_checks: # Without validation, we have to assume all outputs # will be taken from us, and are not really change. self.is_change = False - return + return af # redeem script must be exactly what we expect # - pubkeys will be reconstructed from derived paths here @@ -502,7 +509,7 @@ class psbtOutputProxy(psbtProxy): raise FraudulentChangeOutput(out_idx, "P2WSH witness script has wrong hash") self.is_change = True - return + return af if witness_script: # p2sh-p2wsh case (because it had witness script) @@ -519,19 +526,20 @@ class psbtOutputProxy(psbtProxy): # old BIP-16 style; looks like payment addr expect_pkh = hash160(redeem_script) - elif addr_type == 'p2pkh': + elif af == 'p2pkh': # input is hash160 of a single public key assert len(addr_or_pubkey) == 20 expect_pkh = hash160(expect_pubkey) else: # we don't know how to "solve" this type of input - return + return af if pkh != expect_pkh: raise FraudulentChangeOutput(out_idx, "Change output is fraudulent") # We will check pubkey value at the last second, during signing. self.is_change = True + return af # Track details of each input of PSBT @@ -736,6 +744,16 @@ class psbtInputProxy(psbtProxy): which_key = None addr_type, addr_or_pubkey, addr_is_segwit = utxo.get_address() + if addr_type == "op_return": + self.required_key = None + return + if addr_type == "p2tr": + raise FatalPSBTIssue("Install EDGE firmware to spend taproot.") + if addr_type is None: + # If this is reached, we do not understand the output well + # enough to allow the user to authorize the spend, so fail hard. + raise FatalPSBTIssue('Unhandled scriptPubKey: ' + b2a_hex(addr_or_pubkey).decode()) + if addr_is_segwit and not self.is_segwit: self.is_segwit = True @@ -1473,17 +1491,28 @@ class psbtObject(psbtProxy): # - mark change outputs, so perhaps we don't show them to users total_out = 0 total_change = 0 + num_op_return = 0 + num_op_return_size = 0 + num_unknown_scripts = 0 self.num_change_outputs = 0 for idx, txo in self.output_iter(): output = self.outputs[idx] # perform output validation - output.validate(idx, txo, self.my_xfp, self.active_multisig, self) + af = output.validate(idx, txo, self.my_xfp, self.active_multisig, self) total_out += txo.nValue if output.is_change: self.num_change_outputs += 1 total_change += txo.nValue + if af == "op_return": + num_op_return += 1 + if len(txo.scriptPubKey) > 83: + num_op_return_size += 1 + + elif af is None: + num_unknown_scripts += 1 + if self.total_value_out is None: self.total_value_out = total_out else: @@ -1517,6 +1546,22 @@ class psbtObject(psbtProxy): self.warnings.append(('Big Fee', 'Network fee is more than ' '5%% of total value (%.1f%%).' % per_fee)) + if (num_op_return > 1) or num_op_return_size: + mm = "" + if num_op_return > 1: + mm += " Multiple OP_RETURN outputs: %d" % num_op_return + if num_op_return_size: + mm += " OP_RETURN size > 80 bytes." + self.warnings.append( + ("OP_RETURN", + "TX may not be relayed by some nodes.%s" % mm)) + + if num_unknown_scripts: + self.warnings.append( + ('Output?', + 'Sending to %d not well understood script(s).' % num_unknown_scripts) + ) + self.consolidation_tx = (self.num_change_outputs == self.num_outputs) # Enforce policy related to change outputs diff --git a/shared/serializations.py b/shared/serializations.py index 19cbbcaa..ef5a87ae 100755 --- a/shared/serializations.py +++ b/shared/serializations.py @@ -210,11 +210,13 @@ def disassemble(script): #print('dis %d: number=%d' % (offset, (c - OP_1 + 1))) yield (c - OP_1 + 1, None) elif c == OP_PUSHDATA1: - cnt = script[offset]; offset += 1 + cnt = script[offset] + offset += 1 yield (script[offset:offset+cnt], None) offset += cnt elif c == OP_PUSHDATA2: - cnt = struct.unpack_from("H", script, offset) + # up to 65535 bytes + cnt, = struct.unpack_from("H", script, offset) offset += 2 yield (script[offset:offset+cnt], None) offset += cnt @@ -227,7 +229,8 @@ def disassemble(script): # OP_0 included here #print('dis %d: opcode=%d' % (offset, c)) yield (None, c) - except: + except Exception as e: + import sys;sys.print_exception(e) raise ValueError("bad script") @@ -351,15 +354,13 @@ class CTxOut(object): # Detect type of output from scriptPubKey, and return 3-tuple: # (addr_type_code, addr, is_segwit) # 'addr' is byte string, either 20 or 32 long + if self.is_p2tr(): + return 'p2tr', self.scriptPubKey[2:2+32], True - if len(self.scriptPubKey) == 22 and \ - self.scriptPubKey[0] == 0 and self.scriptPubKey[1] == 20: - # aka. P2WPKH + if self.is_p2wpkh(): return 'p2pkh', self.scriptPubKey[2:2+20], True - if len(self.scriptPubKey) == 34 and \ - self.scriptPubKey[0] == 0 and self.scriptPubKey[1] == 32: - # aka. P2WSH + if self.is_p2wsh(): return 'p2sh', self.scriptPubKey[2:2+32], True if self.is_p2pkh(): @@ -372,9 +373,22 @@ class CTxOut(object): # rare, pay to full pubkey return 'p2pk', self.scriptPubKey[2:2+33], False - # If this is reached, we do not understand the output well - # enough to allow the user to authorize the spend, so fail hard. - raise ValueError('scriptPubKey template fail: ' + b2a_hex(self.scriptPubKey).decode()) + if self.scriptPubKey[0] == OP_RETURN: + return 'op_return', self.scriptPubKey, False + + return None, self.scriptPubKey, None + + def is_p2tr(self): + return len(self.scriptPubKey) == 34 and \ + (OP_1 <= self.scriptPubKey[0] <= OP_16) and self.scriptPubKey[1] == 0x20 + + def is_p2wpkh(self): + return len(self.scriptPubKey) == 22 and \ + self.scriptPubKey[0] == 0 and self.scriptPubKey[1] == 0x14 + + def is_p2wsh(self): + return len(self.scriptPubKey) == 34 and \ + self.scriptPubKey[0] == 0 and self.scriptPubKey[1] == 0x20 def is_p2sh(self): return len(self.scriptPubKey) == 23 and self.scriptPubKey[0] == 0xa9 \ diff --git a/testing/api.py b/testing/api.py index b8bfe1e8..942e168c 100644 --- a/testing/api.py +++ b/testing/api.py @@ -68,8 +68,9 @@ class Bitcoind: # Wait for cookie file to be created cookie_path = os.path.join(self.datadir, "regtest", ".cookie") for i in range(20): - if not os.path.exists(cookie_path): - time.sleep(0.5) + if os.path.exists(cookie_path): + break + time.sleep(0.5) else: RuntimeError("'.cookie' not found. Is bitcoind running?") # Read .cookie file to get user and pass diff --git a/testing/devtest/unit_script.py b/testing/devtest/unit_script.py index ac712bab..a5af2512 100644 --- a/testing/devtest/unit_script.py +++ b/testing/devtest/unit_script.py @@ -2,7 +2,7 @@ # from uio import BytesIO from serializations import ser_push_data, ser_string_vector, deser_string_vector -from serializations import ser_compact_size, deser_compact_size +from serializations import ser_compact_size, deser_compact_size, disassemble test_data = [ # data, result @@ -17,9 +17,10 @@ test_data = [ (65535*b"a", b'M\xff\xff' + (65535 * b"a")), ] -c = 0 for i, (data, result) in enumerate(test_data): assert ser_push_data(data) == result, i + d, _ = list(disassemble(result))[0] + assert d == data try: # PUSHDATA 4 not implemented diff --git a/testing/helpers.py b/testing/helpers.py index d998b028..9ef8babf 100644 --- a/testing/helpers.py +++ b/testing/helpers.py @@ -53,6 +53,11 @@ def fake_dest_addr(style='p2pkh'): if style == "p2tr": return bytes([81, 32]) + prandom(32) + if style == "unknown": + # OP_CHECKLOCKTIMEVERIFY OP_DROP OP_DUP OP_HASH160 OP_EQUALVERIFY OP_CHECKSIG + hex_str = "049f7b2a5cb17576a914371c20fb2e9899338ce5e99908e64fd30b78931388ac" + return bytes.fromhex(hex_str) + # missing: if style == 'p2pk' => pay to pubkey, considered obsolete raise ValueError('not supported: ' + style) diff --git a/testing/test_sign.py b/testing/test_sign.py index 6efd9675..2c374228 100644 --- a/testing/test_sign.py +++ b/testing/test_sign.py @@ -1765,6 +1765,10 @@ def test_bitcoind_missing_foreign_utxo(bitcoind, bitcoind_d_sim_watch, microsd_p @pytest.mark.bitcoind @pytest.mark.parametrize("op_return_data", [ + 81 * b"a", + 255 * b"b", # biggest possible with PUSHDATA1 + 256 * b"c", # PUSHDATA2 + 4000 * b"d", # PUSHDATA2 b"Coldcard is the best signing device", # to test with both pushdata opcodes b"Coldcard, the best signing deviceaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", # len 80 max b"\x80" * 80, @@ -1777,25 +1781,44 @@ def test_op_return_signing(op_return_data, dev, fake_txn, bitcoind_d_sim_watch, start_sign, end_sign, cap_story): cc = bitcoind_d_sim_watch dest_address = cc.getnewaddress() - bitcoind.supply_wallet.generatetoaddress(101, dest_address) + bitcoind.supply_wallet.sendtoaddress(dest_address, 2) + bitcoind.supply_wallet.generatetoaddress(1, bitcoind.supply_wallet.getnewaddress()) psbt = cc.walletcreatefundedpsbt([], [{dest_address: 1.0}, {"data": op_return_data.hex()}], 0, {"fee_rate": 20})["psbt"] - start_sign(base64.b64decode(psbt)) + start_sign(base64.b64decode(psbt), finalize=True) time.sleep(.1) title, story = cap_story() assert title == "OK TO SEND?" # in older implementations, one would see a warning for OP_RETURN --> not now - assert "warning" not in story + if len(op_return_data) > 80: + assert "warning" in story + else: + assert "warning" not in story + assert "OP_RETURN" in story try: + assert len(op_return_data) <= 200 expect = op_return_data.decode("ascii") except: expect = binascii.hexlify(op_return_data).decode() + if len(op_return_data) > 200: + expect = expect[:200] + "\n ⋯\n" + expect[-200:] + assert expect in story - signed = end_sign(accept=True) - tx = cc.finalizepsbt(base64.b64encode(signed).decode())["hex"] - assert cc.testmempoolaccept([tx])[0]["allowed"] is True - tx_id = cc.sendrawtransaction(tx) - assert isinstance(tx_id, str) and len(tx_id) == 64 + tx = end_sign(accept=True, finalize=True).hex() + + # tx is final at this point and consensus valid + # tx = cc.finalizepsbt(base64.b64encode(signed).decode())["hex"] + res = cc.testmempoolaccept([tx])[0] + + if len(op_return_data) > 80: + # policy + assert res["allowed"] is False + assert res["reject-reason"] == "scriptpubkey" + else: + assert res["allowed"] is True + tx_id = cc.sendrawtransaction(tx) + assert isinstance(tx_id, str) and len(tx_id) == 64 + @pytest.mark.parametrize("unknowns", [ # tuples (unknown_global, unknown_ins, unknown_outs) @@ -2165,6 +2188,26 @@ def test_send2taproot_addresss(fake_txn , start_sign, end_sign, cap_story, use_t signed = end_sign(accept=True, finalize=False) assert signed +def test_sign_taproot_input(fake_txn, start_sign, end_sign, cap_story): + psbt = fake_txn(1, 2, taproot_in=True) + start_sign(psbt) + title, story = cap_story() + assert title == "Failure" + assert "Install EDGE firmware" in story + +def test_send2unknown_script(fake_txn , start_sign, end_sign, cap_story, use_testnet): + use_testnet() + psbt = fake_txn(2, 2, segwit_in=True, change_outputs=[0], outstyles=["p2tr", "unknown"]) + start_sign(psbt) + title, story = cap_story() + assert title == "OK TO SEND?" + # we do not understand change in taproot (taproot not supported) + assert "Consolidating" not in story + assert "Change back" not in story + assert "to script" in story + signed = end_sign(accept=True, finalize=False) + assert signed + @pytest.mark.parametrize("num_tx", [1, 2, 10]) @pytest.mark.parametrize("ui_path", [True, False]) @@ -2982,20 +3025,19 @@ def test_txout_explorer(chain, data, fake_txn, start_sign, settings_set, txout_e start_sign(psbt) txout_explorer(data, chain) - -def test_txout_explorer_op_return(fake_txn, start_sign, cap_story, is_q1, - need_keypress, press_cancel): - d = [ - (1, b"Coinkite"), - (0, b"Mk1 Mk2 Mk3 Mk4 Q"), - (100, b"binarywatch.org"), - (100, b"a" * 75), - ] - psbt = fake_txn(1, 20, segwit_in=False, op_return=d) - start_sign(psbt) +@pytest.mark.parametrize("finalize", [True, False]) +@pytest.mark.parametrize("data", [ + [(1, b"Coinkite"), (0, b"Mk1 Mk2 Mk3 Mk4 Q"), (100, b"binarywatch.org"), (100, b"a" * 75)], + [(0, b"a" * 300), (10, b"x" * 1000)] +]) +def test_txout_explorer_op_return(finalize, data, fake_txn, start_sign, cap_story, is_q1, + need_keypress, press_cancel, press_select, end_sign): + psbt = fake_txn(1, 20, segwit_in=False, op_return=data) + start_sign(psbt, finalize=finalize) time.sleep(.1) title, story = cap_story() assert title == 'OK TO SEND?' + assert "warning" in story assert "Press (2) to explore txn" in story need_keypress("2") time.sleep(.1) @@ -3007,17 +3049,25 @@ def test_txout_explorer_op_return(fake_txn, start_sign, cap_story, is_q1, time.sleep(.1) _, story = cap_story() ss = story.split("\n\n") - for i, (sa, sb, (amount, data)) in enumerate(zip(ss[:-1:2], ss[1::2], d), start=20): + for i, (sa, sb, (amount, d)) in enumerate(zip(ss[:-1:2], ss[1::2], data), start=20): assert f"Output {i}:" == sa - val, name, dd = sb.split("\n") + try: + val, name, dd = sb.split("\n") + except: + dd = None + val, name, dd0, _, dd1 = sb.split("\n") assert "OP_RETURN" in name assert f'{amount / 100000000:.8f} XTN' == val - hex_str, ascii_str = dd.split(" ", 1) - assert f"(ascii: {data.decode()})" == ascii_str - assert data.hex() == hex_str + if dd: + hex_str, ascii_str = dd.split(" ", 1) + assert f"(ascii: {d.decode()})" == ascii_str + assert d.hex() == hex_str + else: + assert d.hex()[:200] == dd0 + assert d.hex()[-200:] == dd1 - press_cancel() - press_cancel() + press_cancel() # exit txn out explorer + end_sign(finalize=finalize) def test_low_R_grinding(dev, goto_home, microsd_path, press_select, offer_ms_import, diff --git a/testing/txn.py b/testing/txn.py index d1f2bd64..fc34e2ac 100644 --- a/testing/txn.py +++ b/testing/txn.py @@ -6,7 +6,7 @@ import pytest, struct from ckcc_protocol.protocol import MAX_TXN_LEN from psbt import BasicPSBT, BasicPSBTInput, BasicPSBTOutput from io import BytesIO -from helpers import fake_dest_addr, make_change_addr, hash160 +from helpers import fake_dest_addr, make_change_addr, hash160, taptweak from base58 import decode_base58 from bip32 import BIP32Node from constants import ADDR_STYLES, simulator_fixed_tprv @@ -23,8 +23,8 @@ def fake_txn(dev, pytestconfig): def doit(num_ins, num_outs, master_xpub=None, subpath="0/%d", fee=10000, invals=None, outvals=None, segwit_in=False, wrapped=False, outstyles=['p2pkh'], psbt_hacker=None, change_outputs=[], - capture_scripts=None, add_xpub=None, op_return=None, - psbt_v2=None, input_amount=1E8): + capture_scripts=None, add_xpub=None, op_return=None, taproot_in=False, + psbt_v2=None, input_amount=1E8, unknown_out_script=None): psbt = BasicPSBT() @@ -79,6 +79,10 @@ def fake_txn(dev, pytestconfig): # p2sh-p2wpkh psbt.inputs[i].redeem_script = scr scr = bytes([0xa9, 0x14]) + hash160(scr) + bytes([0x87]) + elif taproot_in: + psbt.inputs[i].taproot_bip32_paths[sec[1:]] = b"\x00" + xfp + struct.pack('