bugfix: PUSHDATA2 in scripts cause yikes

bugfix: missing warning summary in the top of the story for unknown scripts
This commit is contained in:
scgbckbone 2025-05-05 18:51:52 +02:00 committed by doc-hex
parent 32cfd53569
commit ea9d183a48
10 changed files with 207 additions and 76 deletions

View File

@ -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

View File

@ -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):

View File

@ -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):

View File

@ -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

View File

@ -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 \

View File

@ -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

View File

@ -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

View File

@ -53,6 +53,11 @@ def fake_dest_addr(style='p2pkh'):
if style == "p2tr":
return bytes([81, 32]) + prandom(32)
if style == "unknown":
# <same date> OP_CHECKLOCKTIMEVERIFY OP_DROP OP_DUP OP_HASH160 <pubKeyHash> 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)

View File

@ -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,

View File

@ -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('<II', 0, i)
tweaked_xonly = taptweak(sec[1:])
scr = bytes([81, 32]) + tweaked_xonly
else:
# p2pkh
scr = bytes([0x76, 0xa9, 0x14]) + subkey.hash160() + bytes([0x88, 0xac])
@ -153,9 +157,16 @@ def fake_txn(dev, pytestconfig):
amount, data = op_ret
op_return_size = len(data)
if op_return_size < 76:
# OP_RETURN PUSHDATA
script = bytes([106, op_return_size]) + data
else:
elif op_return_size < 256:
# OP_RETURN PUSHDATA1
script = bytes([106, 76, op_return_size]) + data
elif op_return_size < 65536:
# OP_RETURN PUSHDATA2
script = bytes([106, 77]) + struct.pack(b'<H', op_return_size) + data
else:
assert False, "too big OP_RETURN"
op_ret_o = BasicPSBTOutput(idx=len(psbt.outputs))
if psbt_v2: