XMR transaction data integrity checking

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

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

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

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,