What the developer wrote
Authored by Peter D. Gray
slight rework
What changed, and why it matters
This commit fixes a bug in the COLDCARD hardware wallet's handling of Bitcoin transaction files (PSBTs). Previously, the wallet only checked the most recent 50 inputs for duplicates, so a malicious transaction with more than 50 inputs could spend the same coin twice without being detected. The fix now checks up to 100 inputs and uses a more reliable data structure. This could have allowed an attacker to trick a user into signing a transaction that moves more money than it appears to.
Treat this as a security fix. Verify the new 100-entry limit is sufficient for all supported PSBT sizes, consider whether an unbounded set is feasible on the embedded device, and review related PSBT validation paths for similar windowing or resource-limit bugs. No CVE or vendor advisory was supplied.
Security signals we found
Fixes incomplete duplicate-input detection (sliding 50-entry window)
Changes prevout key ordering from (n, hash) to (hash, n), which is more conventional and less collision-prone
Raises FatalPSBTIssue on duplicate inputs
Test coverage expanded to 110 inputs to verify detection beyond old 50-entry limit
Removes test helper restriction that prevented duplicating the first input, enabling more realistic duplicate tests
Evidence from the diff
The patch changes duplicate-input detection in shared/psbt.py from an OrderedDict limited to the last 50 prevouts to a set that stores up to 100 prevouts. The original code used a sliding window of 50 entries, meaning a duplicate input appearing more than 50 positions after its original would not be detected. The new code stores prevouts as (hash, n) tuples in a set and raises FatalPSBTIssue if a duplicate is found. Tests are updated to exercise 2 and 110 inputs, and the test helper is reworked so duplicates are actual copies of earlier inputs rather than just reusing key derivation paths.
Changed components
shared/psbt.py: psbtObject class, input validation during signingtesting/test_sign.py: test_duplicate_inputstesting/txn.py: fake_txn helperInspect captured patch +25 / −35
diff --git a/shared/psbt.py b/shared/psbt.py
index 31bc40d..7c2fb58 100644
--- a/shared/psbt.py
+++ b/shared/psbt.py
@@ -5,7 +5,6 @@
import stash, gc, history, sys, ngu, ckcc, chains
from ustruct import unpack_from, unpack, pack
from ubinascii import hexlify as b2a_hex
-from ucollections import OrderedDict
from utils import xfp2str, B2A, keypath_to_str
from utils import seconds2human_readable, datetime_from_timestamp, datetime_to_str
from chains import NLOCK_IS_TIME
@@ -1762,20 +1761,16 @@ class psbtObject(psbtProxy):
foreign = []
total_in = 0
- prevouts_max_len = 50
- prevouts = OrderedDict()
+ prevouts = set()
for i, txi in self.input_iter():
# check for duplicate inputs
- if len(prevouts) >= prevouts_max_len:
- first = next(iter(prevouts)) # O(1) in mpy
- del prevouts[first]
+ k = (txi.prevout.hash, txi.prevout.n)
+ if k in prevouts:
+ raise FatalPSBTIssue("Duplicate inputs")
- k = txi.prevout.n, txi.prevout.hash
- if k in prevouts: # O(1)
- raise FatalPSBTIssue("Duplicate inputs!")
-
- prevouts[k] = True
+ if len(prevouts) < 100:
+ prevouts.add(k)
inp = self.inputs[i]
if inp.fully_signed:
diff --git a/testing/test_sign.py b/testing/test_sign.py
index d98540b..b031761 100644
--- a/testing/test_sign.py
+++ b/testing/test_sign.py
@@ -3647,16 +3647,14 @@ def test_txn_nVersion_zero(segwit, fake_txn, start_sign, cap_story, goto_home):
assert "txn version" in story
@pytest.mark.parametrize("segwit_in", [True, False])
-@pytest.mark.parametrize("num_ins", [2, 50, 51])
+@pytest.mark.parametrize("num_ins", [2, 110])
def test_duplicate_inputs(segwit_in, num_ins, fake_txn, start_sign, end_sign, cap_story):
psbt = fake_txn(num_ins, 2, segwit_in=segwit_in, dupe_ins=[num_ins-1])
start_sign(psbt)
title, story = cap_story()
- if num_ins <= 50:
- # only works if duplicate is no longer than 50 inputs from original
- assert "Duplicate inputs!" in story
+ if num_ins <= 100:
+ assert "Duplicate input" in story
else:
- # does not work
assert title == "OK TO SEND?"
# EOF
diff --git a/testing/txn.py b/testing/txn.py
index 17a7551..4c85e26 100644
--- a/testing/txn.py
+++ b/testing/txn.py
@@ -27,10 +27,6 @@ def fake_txn(dev, pytestconfig):
psbt_v2=None, input_amount=1E8, unknown_out_script=None, lock_time=0,
sequences=None, sighashes=None, dupe_ins=[]):
- # dupe_ins cannot contain zero, as that will be the duplicated input
- if dupe_ins:
- assert 0 not in dupe_ins
-
psbt = BasicPSBT()
if psbt_v2 is None:
@@ -62,17 +58,12 @@ def fake_txn(dev, pytestconfig):
# - each input is 1BTC
# addr where the fake money will be stored.
- if i in dupe_ins:
- # always duplicate zeroth input
- subkey = mk.subkey_for_path(subpath % 0)
- else:
- subkey = mk.subkey_for_path(subpath % i)
+ subkey = mk.subkey_for_path(subpath % i)
sec = subkey.sec()
assert len(sec) == 33, "expect compressed"
assert subpath[0:2] == '0/'
- idx = 0 if i in dupe_ins else i
- psbt.inputs[i].bip32_paths[sec] = xfp + struct.pack('<II', 0, idx)
+ psbt.inputs[i].bip32_paths[sec] = xfp + struct.pack('<II', 0, i)
# UTXO that provides the funding for to-be-signed txn
supply = CTransaction()
@@ -102,7 +93,7 @@ def fake_txn(dev, pytestconfig):
if segwit_in:
# just utxo for segwit
- psbt.inputs[i].witness_utxo = supply.vout[0 if i in dupe_ins else -1].serialize()
+ psbt.inputs[i].witness_utxo = supply.vout[-1].serialize()
else:
# whole tx for pre-segwit
psbt.inputs[i].utxo = supply.serialize_with_witness()
@@ -131,14 +122,20 @@ def fake_txn(dev, pytestconfig):
seq = sequences[0]
spendable = CTxIn(COutPoint(supply.sha256, 0), nSequence=seq)
- txn.vin.append(spendable)
+ if i not in dupe_ins:
+ txn.vin.append(spendable)
- if psbt_v2:
- psbt.inputs[i].previous_txid = supply.hash
- psbt.inputs[i].prevout_idx = 0
- psbt.inputs[i].sequence = seq
- # psbt.inputs[i].req_time_locktime = None
- # psbt.inputs[i].req_height_locktime = None
+ if psbt_v2:
+ psbt.inputs[i].previous_txid = supply.hash
+ psbt.inputs[i].prevout_idx = 0
+ psbt.inputs[i].sequence = seq
+ # psbt.inputs[i].req_time_locktime = None
+ # psbt.inputs[i].req_height_locktime = None
+ else:
+ assert i != 0, 'cant dup first input'
+ txn.vin.append(txn.vin[-1])
+ from copy import deepcopy
+ psbt.inputs[i] = deepcopy(psbt.inputs[i-1])
for i in range(num_outs):
# random P2PKH
Why this scored 79/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.