From 2fe498b1a01f78c09d9e1b210c575dd9ec269867 Mon Sep 17 00:00:00 2001 From: The MMGen Project Date: Mon, 9 Nov 2026 09:33:34 +0000 Subject: [PATCH] XMR transaction data integrity checking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Perform a series of pre-signing checks to ensure that the change address in the transaction belongs to the user’s wallet, that the amount shown is going to the destination shown, and others, preventing an attacker from stealing funds in the event the user’s online installation is compromised. These checks were always performed by the wallet for non-privacy coins but were regrettably overlooked for Monero. Thus this patch represents an urgent security fix for Monero users. It bears mentioning that no system, however secure, can do everything for the user. For example, when sending funds to an outside address, the offline installation has no way of knowing whether the destination and amount are what the user intended. A good rule of thumb is to always check the offline signing summary to ensure that the transaction type is correct and, for transfers, that the address and amount are correct. --- mmgen/exception.py | 1 + mmgen/proto/xmr/tx/base.py | 2 +- mmgen/xmrwallet/ops/sign.py | 114 ++++++++++++++++++++++++++++++++++++ 3 files changed, 116 insertions(+), 1 deletion(-) diff --git a/mmgen/exception.py b/mmgen/exception.py index 3a941d2b..bb1b8fe5 100644 --- a/mmgen/exception.py +++ b/mmgen/exception.py @@ -84,6 +84,7 @@ class HexadecimalStringError(Exception): mmcode = 3 class SeedLengthError(Exception): mmcode = 3 class PrivateKeyError(Exception): mmcode = 3 class TxFileVersionError(Exception): mmcode = 3 +class TxIntegrityCheckError(Exception): mmcode = 3 class MMGenCalledProcessError(Exception): mmcode = 3 class TestSuiteFatalException(Exception): mmcode = 3 diff --git a/mmgen/proto/xmr/tx/base.py b/mmgen/proto/xmr/tx/base.py index 9c68f869..1fcf829f 100644 --- a/mmgen/proto/xmr/tx/base.py +++ b/mmgen/proto/xmr/tx/base.py @@ -27,7 +27,7 @@ class Base: return None def check_serialized_integrity(self): - pass + 'This check is performed by check_integrity() in mmgen/xmrwallet/ops/sign.py' def check_sigs(self): return bool(getattr(self.compat_tx.data, 'signed_txset', None)) diff --git a/mmgen/xmrwallet/ops/sign.py b/mmgen/xmrwallet/ops/sign.py index 5f91470a..1a9d87b1 100644 --- a/mmgen/xmrwallet/ops/sign.py +++ b/mmgen/xmrwallet/ops/sign.py @@ -11,6 +11,9 @@ xmrwallet.ops.sign: Monero wallet ops for the MMGen Suite """ from pathlib import Path + +from ...util import ymsg, die + from ..rpc import MoneroWalletRPC from .wallet import OpWallet @@ -21,12 +24,123 @@ class OpSign(OpWallet): return_data = True main_starts_daemon = True + def get_subaddress(self, d, acct_idx, addr_idx): + from ...proto.xmr.subaddress import get_subaddress + return get_subaddress( + proto = self.proto, + master_addr = d.addr, + master_svk = bytes.fromhex(d.viewkey), + acct_idx = acct_idx, + addr_idx = addr_idx) + + def check_integrity(self, d, h, t, desc_res): + """ + Since Monero hard fork #12, the transaction version for new transactions is always 2, + and version 2 transactions must have exactly two outputs. Transfers have a change and + destination output, while sweeps have a dummy and destination output. + + Thus the following actions will ensure that funds are not being stolen by an attacker: + + Actions performed by the code: + - for all transactions, check that change (for transfers) or destination (for sweeps) + address referenced in metadata belongs to wallet (performed via get_subaddress()) + - for all transactions, check that destination address, amount and fee in metadata + match those in transaction data + - check that sweeps have one dummy output and transfers have none + - check that sweeps have no change address + - check that transfer transaction data has a change address and that it is the account + base address + - check that all inputs are RingCT + - check that input amount is equal to sum of send amount, change amount and fee + + Actions performed by the user: + - For transfers, observe output of the offline signing operation and check correctness + of destination address and amount. The offline installation cannot know how much the + user intended to send and to what address. + """ + + def do_err(msg): + die('TxIntegrityCheckError', msg) + + def check_subaddr(subaddr, subaddr_chk): + if subaddr != subaddr_chk: + do_err( + f'subaddr in metadata ({subaddr}) does not match subaddr ' + f'generated by wallet ({subaddr_chk})') + + def check_dummies(count): + if (dcount := res['dummy_outputs']) != count: + do_err(f'{dcount}: incorrect number of dummy outputs (should be {count})') + + if len(desc_res['desc']) != 1: + do_err('transaction has multiple descriptions') + + res = desc_res['desc'][0] + + match t.op: + case 'transfer': + wallet_data = h.get_wallet_data(print=False) + chg_addr = wallet_data.accts_data['subaddress_accounts'][t.source.account]['base_address'] + check_subaddr( + chg_addr, + self.get_subaddress(d, t.source.account, 0)) + check_dummies(0) + if not (chg_res := res['change_address']): + do_err('no change address for transfer TX!') + if chg_res != chg_addr: + do_err(f'{chg_res}: incorrect change address for transfer TX (should be {chg_addr})') + case 'sweep' | 'sweep_all': + check_subaddr( + t.dest_address, + self.get_subaddress( + (d if t.dest.wallet == h.d.idx else + self.gen_keyaddr_list(str(t.dest.wallet)).data[0]), + t.dest.account, + t.dest.account_address)) + check_dummies(1) + if res['change_address']: + do_err('sweep TX must not have a change address!') + + mismatch_fs = '{x} in metadata ({}) does not match {x} in transaction data ({})' + + if (fee_res := self.proto.coin_amt(res['fee'], from_unit='atomic')) != t.fee: + do_err(mismatch_fs.format(t.fee, fee_res.hl(color=False), x='fee')) + + idata = res['sources'] + + if len(idata) != 1: + ymsg(f'Warning: transaction has {len(idata)} inputs!') + + for e in idata: + if not e['rct']: + do_err('transaction source is not RingCT') + + odata = res['recipients'] + + if len(odata) != 1: + do_err(f'{len(odata)}: incorrect number of recipients for TX (should be 1)') + + if (addr_res := odata[0]['address']) != t.dest_address: + do_err(mismatch_fs.format(t.dest_address, addr_res, x='destination address')) + + if (amt_res := self.proto.coin_amt(odata[0]['amount'], from_unit='atomic')) != t.amount: + do_err(mismatch_fs.format(t.amount, amt_res.hl(color=False), x='send amount')) + + # check the wallet daemon itself, just in case: + if (osum := odata[0]['amount'] + res['change_amount'] + res['fee']) != res['amount_in']: + do_err(f'sum of outputs ({osum}) does not match sum of inputs ({res["amount_in"]})') + async def process_wallet(self, d, fn, last): h = MoneroWalletRPC(self, d) if self.need_daemon_start or len(self.kal.data) != 1: h.open_wallet(refresh=False) self.head_msg(d.idx, h.fn) tx = self.get_tx_cls('Unsigned')(self.cfg, Path(self.uargs.spec)) + self.check_integrity( + d, h, tx.data, + self.c.call( + 'describe_transfer', + unsigned_txset = tx.data.unsigned_txset)) res = self.c.call( 'sign_transfer', unsigned_txset = tx.data.unsigned_txset,