SFT-8098: complete change verification before transaction review
What changed, and why it matters
This commit moves a safety check earlier in the process of approving a Bitcoin transaction on the Passport hardware wallet. Previously, the wallet verified that 'change' outputs (money going back to your own wallet) truly belonged to you only after you had already reviewed the transaction and approved signing. Now that ownership proof happens before the transaction details are shown to the user. This prevents a scenario where a malicious or buggy PSBT (the file describing the transaction) hides a change address that actually belongs to an attacker, because the wallet will refuse to show the review screen if it cannot prove the change is yours. The change is defensive: it closes a window where a user might approve a transaction whose change output has not yet been validated.
Treat this as a security-hardening fix and include it in the next firmware release. Users signing PSBTs—especially multisig ones where change output details are hidden during review—benefit from the earlier validation. No immediate end-user action is required beyond applying the update. Reviewers should confirm that `double_check_psbt_change_task` covers all change output types (P2WPKH, P2TR, multisig) and that no other code path can reach `show_transaction_details` without passing this check.
Security signals we found
Change-address verification moved from post-approval signing stage to pre-review stage
PSBT change output ownership now proved before user is shown transaction details
Sensitive key derivation context closed before transaction review is displayed
Unit tests assert mismatched change keys abort flow before review/signing
Comment updated to document that change public-key validation happens before review
Evidence from the diff
The patch relocates the double_check_psbt_change_task invocation from sign_transaction() to check_multisig_import() in sign_psbt_common_flow.py. The task derives the expected public keys for change outputs and confirms they match the PSBT’s claimed subpaths/tap_subpaths. By running it before show_transaction_details, the UI no longer presents a transaction review screen until all change outputs have been cryptographically tied to the device’s own keys. The comment in psbt.py is updated to reflect that change-key derivation/validation now occurs before review rather than during signing. Unit tests are expanded to assert that the verification runs exactly once, that the sensitive-value context is closed before review, and that mismatched change keys abort the flow with an error instead of reaching the review/signing stages.
Changed components
ports/stm32/boards/Passport/modules/flows/sign_psbt_common_flow.pyports/stm32/boards/Passport/modules/psbt.pyports/stm32/boards/Passport/modules/tests/unit/psbt_multisig_approval.pyCHANGELOG.mdInspect captured patch +150 / −46
### CHANGELOG.md
@@ -5,6 +5,7 @@ SPDX-License-Identifier: GPL-3.0-or-later
-->
## Head
+- Complete change-address verification before transaction review
- Identify network fees from unverifiable PSBT inputs as unverified
- Validate the complete local xpub when importing multisig wallets
- Require confirmation before using PSBT-proposed multisig wallets with temporary seeds,
### ports/stm32/boards/Passport/modules/flows/sign_psbt_common_flow.py
@@ -42,6 +42,8 @@ async def validate_psbt(self):
async def check_multisig_import(self):
from flows import ImportMultisigWalletFlow
+ from pages import ErrorPage
+ from tasks import double_check_psbt_change_task
# Based on the import mode and whether this already exists, the validation step
# will have set this flag.
@@ -51,6 +53,16 @@ async def check_multisig_import(self):
self.set_result(None)
return
+ # Review hides change destinations, so prove ownership before showing it.
+ gc.collect()
+ (error_msg, error) = await spinner_task('Validating transaction',
+ double_check_psbt_change_task, args=[self.psbt])
+ gc.collect()
+ if error is not None:
+ await ErrorPage(error_msg).show()
+ self.set_result(None)
+ return
+
self.goto(self.show_transaction_details)
async def show_transaction_details(self):
@@ -149,7 +161,6 @@ async def show_warnings(self):
self.goto(self.sign_transaction)
async def sign_transaction(self):
- from tasks import double_check_psbt_change_task
from utils import spinner_task
from pages import ErrorPage, QuestionPage
@@ -166,16 +177,6 @@ async def sign_transaction(self):
else:
self.back()
else:
- # TODO: Why do this here instead of in validate?
- (error_msg, error) = await spinner_task('Signing Transaction',
- double_check_psbt_change_task, args=[self.psbt])
-
- gc.collect()
- if error is not None:
- await ErrorPage(error_msg).show()
- self.set_result(None)
- return
-
(error_msg, error) = await spinner_task('Signing Transaction',
sign_psbt_task, args=[self.psbt])
gc.collect()
### ports/stm32/boards/Passport/modules/psbt.py
@@ -392,7 +392,7 @@ def validate(self, out_idx, txo, my_xfp, active_multisig):
# careful and fully validate all the details.
# - no output info is needed, in general, so
# any output info provided better be right, or fail as "fraud"
- # - full key derivation and validation is done during signing, and critical.
+ # - the signing flow derives and validates change public keys before review.
# - we raise fraud alarms, since these are not innocent errors
#
### ports/stm32/boards/Passport/modules/tests/unit/psbt_multisig_approval.py
@@ -1,14 +1,31 @@
# SPDX-FileCopyrightText: © 2026 Foundation Devices, Inc. <hello@foundation.xyz>
# SPDX-License-Identifier: GPL-3.0-or-later
#
-# Test approval of multisig wallets proposed by PSBTs.
+# Test wallet approval and change verification before transaction review.
import common
import flows
+import pages
+import stash
+import utils
import uasyncio as asyncio
-from flows import SignPsbtCommonFlow
+from flows import Flow, SignPsbtCommonFlow
+from flows import sign_psbt_common_flow
from public_constants import MUSIG_ASK, MUSIG_SKIP
-from utils import get_multisig_policy
+from ubinascii import unhexlify
+from utils import get_multisig_policy, str_to_keypath
+
+
+MY_XFP = 0x12345678
+OTHER_KEY = unhexlify('02c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5')
+PATHS = ("m/84'/0'/0'/1/7", "m/86'/0'/0'/1/7", "m/48'/0'/0'/2'/1/7")
+# Public keys derived from the deterministic test seed bytes(range(32)).
+OWNED_KEYS = (
+ unhexlify('02477e5978ac99be533333b444e635fdee1001992c01dec8c41327cdb9a7d7b2a1'),
+ unhexlify('03bdb8d2c655d9d1fb6ff6533b43660033e5c23b2ca4b290223d813f08407d6bdb'),
+ unhexlify('03505e71bc8f2aa762c561194bf3aa6c315fb07a0cf63fd631e43a6a2c17674105'),
+)
+events = []
class FakeImportMultisigWalletFlow:
@@ -20,13 +37,86 @@ def __init__(self, wallet):
FakeImportMultisigWalletFlow.calls += 1
async def run(self):
+ events.append('import')
return self.result
class FakePsbt:
def __init__(self, needs_approval):
self.multisig_import_needs_approval = needs_approval
self.active_multisig = 'proposed-wallet'
+ self.my_xfp = MY_XFP
+ self.outputs = []
+
+
+class FakeOutput:
+ is_change = True
+
+ def __init__(self, path, key):
+ self.subpaths = {}
+ self.tap_subpaths = {}
+ if path == PATHS[1]:
+ self.tap_subpaths[key[1:]] = (str_to_keypath(MY_XFP, path), [])
+ else:
+ if path == PATHS[2]:
+ # The other cosigner must not be mistaken for our ownership proof.
+ self.subpaths[b'\x03' + OTHER_KEY[1:]] = str_to_keypath(0xabcdef01, path)
+ self.subpaths[key] = str_to_keypath(MY_XFP, path)
+
+
+class FakeNode:
+ def __init__(self, key):
+ self.key = key
+
+ def public_key(self):
+ return self.key
+
+
+class FakeSensitiveValues:
+ def __enter__(self):
+ events.append('verify')
+ return self
+
+ def __exit__(self, *_args):
+ events.append('clear')
+
+ def derive_path(self, path):
+ assert path in PATHS
+ events.append(path)
+ return FakeNode(OWNED_KEYS[PATHS.index(path)])
+
+
+class FakeErrorPage:
+ def __init__(self, text):
+ assert "BIP32 path doesn't match" in text
+
+ async def show(self):
+ events.append('error')
+
+
+class FakeQuestionPage:
+ def __init__(self, **_kwargs):
+ pass
+
+ async def show(self):
+ events.append('confirm')
+ return True
+
+
+async def fake_spinner_task(_text, task, args=()):
+ results = []
+
+ async def on_done(*result):
+ results.append(result)
+
+ await task(on_done, *args)
+ assert len(results) == 1
+ return results[0]
+
+
+async def fake_sign_psbt_task(on_done, _psbt):
+ events.append('sign')
+ await on_done(None, None)
class FakeSettings:
@@ -43,52 +133,63 @@ def get(self, key, default=None):
return default
-class FakeSignFlow:
+class FakeSignFlow(SignPsbtCommonFlow):
def __init__(self, needs_approval):
+ Flow.__init__(self, initial_state=self.check_multisig_import)
self.psbt = FakePsbt(needs_approval)
- self.show_transaction_details = 'transaction-details'
- self.completed = False
- self.result = 'unset'
- self.next_state = None
-
- def set_result(self, result):
- self.completed = True
- self.result = result
- def goto(self, state):
- self.next_state = state
+ async def show_transaction_details(self):
+ events.append('review')
+ self.goto(self.sign_transaction)
async def run_tests():
- original_import_flow = flows.ImportMultisigWalletFlow
original_settings = common.settings
+ replacements = (
+ (flows, 'ImportMultisigWalletFlow', FakeImportMultisigWalletFlow),
+ (pages, 'ErrorPage', FakeErrorPage),
+ (pages, 'QuestionPage', FakeQuestionPage),
+ (stash, 'SensitiveValues', FakeSensitiveValues),
+ (utils, 'spinner_task', fake_spinner_task),
+ (sign_psbt_common_flow, 'spinner_task', fake_spinner_task),
+ (sign_psbt_common_flow, 'sign_psbt_task', fake_sign_psbt_task),
+ )
+ originals = [(module, name, getattr(module, name)) for module, name, _ in replacements]
try:
- flows.ImportMultisigWalletFlow = FakeImportMultisigWalletFlow
+ for module, name, replacement in replacements:
+ setattr(module, name, replacement)
FakeImportMultisigWalletFlow.result = False
FakeImportMultisigWalletFlow.calls = 0
+ events.clear()
flow = FakeSignFlow(needs_approval=True)
- await SignPsbtCommonFlow.check_multisig_import(flow)
+ assert await flow.run() is None
assert FakeImportMultisigWalletFlow.calls == 1
- assert flow.completed
- assert flow.result is None
- assert flow.next_state is None
+ assert events == ['import']
FakeImportMultisigWalletFlow.result = True
- FakeImportMultisigWalletFlow.calls = 0
- flow = FakeSignFlow(needs_approval=True)
- await SignPsbtCommonFlow.check_multisig_import(flow)
- assert FakeImportMultisigWalletFlow.calls == 1
- assert not flow.completed
- assert flow.next_state == flow.show_transaction_details
-
- FakeImportMultisigWalletFlow.calls = 0
- flow = FakeSignFlow(needs_approval=False)
- await SignPsbtCommonFlow.check_multisig_import(flow)
- assert FakeImportMultisigWalletFlow.calls == 0
- assert not flow.completed
- assert flow.next_state == flow.show_transaction_details
+ for needs_approval in (True, False):
+ events.clear()
+ flow = FakeSignFlow(needs_approval)
+ assert await flow.run() is flow.psbt
+ expected = ['import'] if needs_approval else []
+ assert events == expected + ['verify', 'clear', 'review', 'confirm', 'sign']
+
+ # Exercise the real ownership task and flow, with only the key store and UI replaced.
+ # Its sensitive-value context must close before review, and run just once.
+ for path, owned_key in zip(PATHS, OWNED_KEYS):
+ for key in (owned_key, OTHER_KEY):
+ events.clear()
+ flow = FakeSignFlow(needs_approval=False)
+ flow.psbt.outputs = [FakeOutput(path, key)]
+ result = await flow.run()
+ if key == owned_key:
+ assert result is flow.psbt
+ assert events == ['verify', path, 'clear', 'review', 'confirm', 'sign']
+ else:
+ assert result is None
+ assert events == ['verify', path, 'clear', 'error']
common.settings = FakeSettings()
assert get_multisig_policy() == MUSIG_ASK
@@ -98,7 +199,8 @@ async def run_tests():
return_value.write(b'OK')
finally:
- flows.ImportMultisigWalletFlow = original_import_flow
+ for module, name, original in originals:
+ setattr(module, name, original)
common.settings = original_settings
Why this scored 59/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.