diff --git a/keepkeylib/client.py b/keepkeylib/client.py index 3ae9a69f..cb0cb62c 100644 --- a/keepkeylib/client.py +++ b/keepkeylib/client.py @@ -999,15 +999,37 @@ def osmosis_sign_tx( if len(msg['value']['amount']) != 1: raise CallException("Osmosis.MsgSend", "Multiple amounts per msg not supported") - denom = msg['value']['amount'][0]['denom'] - if denom != 'uatom': - raise CallException("Osmosis.MsgSend", "Unsupported denomination: " + denom) - + # This branch had never executed. It whitelisted 'uatom' — the + # COSMOS denom, so a native OSMO send was impossible — dropped + # the denom instead of forwarding it, and assigned an int to + # OsmosisMsgSend.amount, which is a string field and would have + # raised even for uatom. + # + # The denom IS now forwarded (the firmware needs it to decide + # whether to scale), but MsgSend is still fenced to uosmo, and + # not for the reason the old whitelist implied: + # osmosis_signTxUpdateMsgSend takes only (amount, to_address) + # and HARDCODES "denom":"uosmo" into the amino JSON it hashes, + # while fsm_msgOsmosisMsgAck renders whatever denom arrives. + # Forwarding a different one therefore DISPLAYS one asset and + # SIGNS another — e.g. a large amount of some worthless ibc/... + # token on screen, a large amount of OSMO in the signature. + # osmosis_signTxUpdateMsgDelegate already takes a denom, so + # MsgSend is the outlier. Lift this fence only once the + # firmware serializer accepts a denom. + coin = msg['value']['amount'][0] + if coin['denom'] != 'uosmo': + raise CallException( + "Osmosis.MsgSend", + "Only uosmo is signable: the firmware MsgSend serializer " + "hardcodes uosmo in the sighash, so any other denom would " + "be displayed but not signed (got %s)" % coin['denom']) resp = self.call(osmosis_proto.OsmosisMsgAck( send=osmosis_proto.OsmosisMsgSend( from_address=msg['value']['from_address'], to_address=msg['value']['to_address'], - amount=int(msg['value']['amount'][0]['amount']), + denom=coin['denom'], + amount=str(coin['amount']), address_type=types.SPEND, ) )) diff --git a/scripts/generate-test-report.py b/scripts/generate-test-report.py index 76f5a505..43185e6b 100644 --- a/scripts/generate-test-report.py +++ b/scripts/generate-test-report.py @@ -962,6 +962,56 @@ def _arg_shown(a): 'Sign Cosmos with memo', 'Memo field displayed for exchange deposit tags.', []), ]), + ('P', 'Osmosis', '7.15.0', + 'Osmosis is the Cosmos-ecosystem DEX, signed with the same amino encoding as Cosmos Hub and ' + 'derived from the same coin type (118). 7.15.0 CHANGED how every Osmosis amount is drawn: the ' + 'confirm screens formatted with atof() + "%.6f", and a float carries only ~7 significant ' + 'decimal digits, so a large transfer was displayed ROUNDED on the screen the user approves ' + '(123456789.123456 OSMO rendered as 123456792.000000). The signature was always over the ' + 'correct amount — the error was confined to the display, which is the half a hardware wallet ' + 'exists to get right. Amounts now format through bn_format_uint64 in integer math, exact at ' + 'any magnitude, and the same change removed the newlib floating-point engine from the build.', + [ + 'SEND: recipient + OSMO amount rendered from integer base units, never a float', + 'PRECISION: 15-significant-digit amounts display exactly, not rounded to 7', + 'UNKNOWN DENOM: shown as raw base units — the device does not guess a decimal point', + ], + [ + ('P1', 'test_msg_osmosis_signtx', 'test_osmosis_sign_tx', + 'Sign Osmosis send', + 'Baseline MsgSend: recipient and a whole-OSMO amount on the confirm screen.', + ['OSMO send']), + ('P2', 'test_msg_osmosis_signtx', 'test_osmosis_send_amount_beyond_float_precision', + 'Amount beyond float precision displays exactly', + 'The regression this section exists for: 123456789123456 uosmo needs 15 significant ' + 'digits. The old float path drew 123456792.000000 OSMO over a transaction moving ' + '123456789.123456 OSMO. The captured frame is the evidence.', + ['Exact large amount']), + ('P3', 'test_msg_osmosis_signtx', 'test_osmosis_send_subunit_amount', + 'Sub-unit amount keeps its tail', + '500 uosmo is 0.000500 OSMO — no integer part and six decimals; it must not collapse ' + 'to 0 or lose the trailing digits.', + ['Sub-unit amount']), + ('P4', 'test_msg_osmosis_signtx', 'test_osmosis_send_non_uosmo_denom_is_refused', + 'Non-uosmo MsgSend is refused, not displayed', + 'osmosis_signTxUpdateMsgSend hardcodes "denom":"uosmo" into the amino JSON it ' + 'hashes while the confirm screen renders whatever denom arrived, so any other denom ' + 'would display one asset and sign another. Refused client-side until the firmware ' + 'serializer accepts a denom.', + # Refusal happens before the device is reached — no frame to capture. + []), + ('P5', 'test_msg_osmosis_signtx', 'test_osmosis_amount_is_committed_to_the_signature', + 'Displayed amount is in the digest', + 'Two sends differing only in amount produce different signatures, so the confirm ' + 'screen is bound to what is signed rather than decorative.', + []), + ('P6', 'test_msg_osmosis_signtx', 'test_osmosis_signing_is_deterministic', + 'Deterministic nonces (RFC6979)', + 'Identical input yields an identical signature; a mismatch is a key-recovery risk, ' + 'not a cosmetic one.', + []), + ]), + ('H', 'THORChain', '7.0.0', 'THORChain is a decentralized cross-chain liquidity protocol. Native RUNE transactions use amino ' 'encoding with thor1... bech32 addresses. The memo field is the critical security element - it ' @@ -1456,6 +1506,81 @@ def _arg_shown(a): 'Account authority ops require owner', 'account_create / account_update sign only under the owner role.', []), + # ── Phase 2/3 op table (fw #315) ──────────────────────────────── + # These ran green in the full suite from the day they landed, but had + # no SECTIONS entry, so screenshot_filter() never selected them and + # eleven newly clear-signed ops shipped with zero OLED proof. A + # correct signature over bytes the user was shown something else for + # is the exact failure the clear-sign table exists to prevent, so + # every op that renders a confirm screen gets a non-empty hint. + ('G29', 'test_msg_hive', 'test_hive_sign_ops_limit_order_create', + 'Internal market: limit_order_create', + 'The op that motivated phase 3 — a HIVE->HBD market swap. Both sides of the order ' + 'are shown with their symbols pinned (a swapped symbol hides a ~2000x value ' + 'difference behind an identical-looking number), and order id / fill-or-kill / ' + 'expiry get their own screen so they cannot be crowded off the first.', + ['Sell and receive amounts', 'Order terms screen']), + ('G30', 'test_msg_hive', 'test_hive_sign_ops_limit_order_cancel', + 'Internal market: limit_order_cancel', + 'Cancelling names the order id and the owner. No recipient row is forged — the op ' + 'acts on the signer\'s own book entry.', + ['Cancel order screen']), + ('G31', 'test_msg_hive', 'test_hive_sign_ops_active_tier_value_ops', + 'Active-tier value ops', + 'transfer_to_vesting, convert, transfer_to/from_savings, delegate_vesting_shares ' + 'and withdraw_vesting all move or lock value, so all six sign only under active. ' + 'Each renders its own amount + counterparty.', + ['Power up', 'Convert', 'Savings deposit/withdraw', 'Delegation', 'Power down']), + ('G32', 'test_msg_hive', 'test_hive_sign_ops_posting_tier_ops', + 'claim_reward_balance is posting tier', + 'Claiming is not spending, so it signs under posting. Three reward assets across ' + 'two screens (the OLED body fits three rows; a fourth would be signed but never ' + 'shown).', + ['Claim rewards screens']), + ('G33', 'test_msg_hive', 'test_hive_sign_ops_zero_amount_semantics', + 'Zero means something for two ops, nothing for the rest', + '0 VESTS stops a power-down and removes a delegation — both legitimate, so zero is ' + 'NOT rejected there and the screen must say which action it is. Everywhere else a ' + 'zero amount is a no-op and refused.', + ['Stop power down', 'Remove delegation']), + ('G34', 'test_msg_hive', 'test_hive_sign_ops_asset_symbol_and_precision_pinned', + 'Asset symbol pinned to its protocol precision', + 'HIVE/HBD are 3-decimal, VESTS is 6. The parser refuses any other pairing: a wrong ' + 'precision moves the decimal point on the confirmation screen relative to what the ' + 'chain applies.', + []), + ('G35', 'test_msg_hive', 'test_hive_sign_ops_comment_options_binds_to_its_comment', + 'comment_options binds to its own comment', + 'Payout redirection is only accepted immediately after a comment op with the same ' + 'author and permlink. Standing alone it could attach beneficiaries to a post the ' + 'user published earlier and is not reviewing on this screen.', + ['Payout options screens']), + ('G36', 'test_msg_hive', 'test_hive_sign_ops_comment_options_beneficiary_rules', + 'Beneficiary ordering, uniqueness and total enforced on-device', + # Scoped to exactly what the mapped test asserts. It covers three + # rejections — unsorted, duplicate, and weights summing over 100%. + # The extension-count cap, the 1-8 count bound and per-beneficiary + # weight range are enforced by the parser but are NOT exercised here, + # so the entry must not claim them. + 'Beneficiaries must be strictly ascending by account (which also makes them unique) ' + 'and their weights must sum to no more than 10000 bp. Unsorted, duplicate and ' + 'over-100% lists are each refused.', + # Rejection-only: every case here is _assert_ops_fails, so the device + # refuses before drawing anything and the capture would be three + # frames of the idle home screen — a report entry that LOOKS like + # visual proof and is not. The per-beneficiary confirm screens are + # captured by G35, which actually signs a two-beneficiary payout. + []), + ('G37', 'test_msg_hive', 'test_hive_sign_ops_account_update2_rejects_authority_change', + 'account_update2 cannot rotate keys', + 'Only the profile-metadata form is in the table. Any owner/active/posting/memo_key ' + 'field present is a hard reject — the op-9/10 device-derived-keys invariant applied ' + 'field-level.', + []), + ('G38', 'test_msg_hive', 'test_hive_sign_ops_truncated_bodies_rejected', + 'Truncated op bodies refused', + 'A body cut short mid-field is a parse failure, not sign-what-you-can.', + []), ]), ('S', 'Solana', '7.14.0', diff --git a/tests/test_msg_osmosis_signtx.py b/tests/test_msg_osmosis_signtx.py new file mode 100644 index 00000000..7834cc39 --- /dev/null +++ b/tests/test_msg_osmosis_signtx.py @@ -0,0 +1,177 @@ +"""Osmosis MsgSend signing — with the confirm-screen amount as the point. + +Osmosis had NO device tests at all: the confirm screens that render amounts +were covered only by host-side unit tests of the formatter in isolation. That +matters more than it sounds, because 7.15.0 CHANGED how every Osmosis amount +is drawn. + +Before, fsm_msg_osmosis.h rendered amounts with atof() + "%.6f". A float +carries ~7 significant decimal digits, so a large amount was displayed +ROUNDED on the very screen the user approves: + + 123456789123456 uosmo -> shown as "123456792.000000 OSMO" + actual 123456789.123456 OSMO + +The signature was over the correct amount either way — the lie was only on +the screen, which is the half a hardware wallet exists to get right. It now +formats with bn_format_uint64 in integer math. + +KNOWN LIMIT, do not overstate this: osmosis_formatAmount converts with an +unchecked strtoull(), so it is exact only for a CANONICAL decimal uint64. +strtoull saturates past UINT64_MAX and also accepts leading whitespace, a +sign, and 0x — so "18446744073709551616" or "-1" display as +18446744073.709551615 OSMO while the original string is what gets hashed. +That is the same display/signature divergence in a different disguise and +needs a firmware-side canonical-range check; these tests deliberately do not +claim otherwise. + +These tests are paired with SECTIONS entries carrying screenshot hints, so +the rendered frame is captured as evidence. A test asserting only "it signed" +cannot prove what the OLED drew. + +pyk's osmosis_sign_tx currently implements osmosis-sdk/MsgSend only; the +delegate/undelegate/LP/swap/IBC screens share the same formatter but are not +reachable from here until the client learns those message types. +""" +import unittest +import common + +from binascii import hexlify + +from keepkeylib.tools import parse_path + +# Osmosis uses the Cosmos coin type (118), not one of its own. +DEFAULT_BIP32_PATH = "m/44h/118h/0h/0/0" + + +def make_send(from_address, to_address, amount, denom='uosmo'): + return { + 'type': 'osmosis-sdk/MsgSend', + 'value': { + 'from_address': from_address, + 'to_address': to_address, + 'amount': [{'denom': denom, 'amount': str(amount)}], + }, + } + + +class TestMsgOsmosisSignTx(common.KeepKeyTest): + + def _address(self): + """Ask the device for its own osmo1 address. + + Deliberately NOT a hardcoded constant: the firmware bech32-decodes + to_address and refuses a bad checksum, so a literal invented by + swapping a cosmos1 prefix for osmo1 fails with the opaque "Failed to + include send message in transaction". Deriving it keeps the fixture + honest and makes these self-sends. + """ + # osmosis_get_address is decorated @field('address'), so it already + # returns the string rather than the OsmosisAddress message. + return self.client.osmosis_get_address( + address_n=parse_path(DEFAULT_BIP32_PATH) + ) + + def _sign(self, amount, denom='uosmo'): + addr = self._address() + return self.client.osmosis_sign_tx( + address_n=parse_path(DEFAULT_BIP32_PATH), + account_number=16359, + chain_id="osmosis-1", + fee=800, + gas=290000, + msgs=[make_send(addr, addr, amount, denom)], + memo="", + sequence=17, + ) + + def test_osmosis_sign_tx(self): + """Baseline: a whole-OSMO send signs and returns a well-formed + secp256k1 signature + compressed pubkey.""" + self.requires_fullFeature() + self.requires_firmware("7.15.0") + self.setup_mnemonic_nopin_nopassphrase() + + sig = self._sign(1500000) # 1.500000 OSMO + self.assertEqual(len(sig.signature), 64) + self.assertEqual(len(sig.public_key), 33) + self.assertIn(hexlify(sig.public_key)[:2], (b'02', b'03')) + + def test_osmosis_send_amount_beyond_float_precision(self): + """THE regression. 123456789123456 uosmo needs 15 significant digits; + a float holds ~7, so the old atof()+"%.6f" path drew + "123456792.000000 OSMO" over a transaction that actually moves + 123456789.123456 OSMO. The captured frame is the proof — assert here + only that the device signs it, and read the amount off the screenshot. + """ + self.requires_fullFeature() + self.requires_firmware("7.15.0") + self.setup_mnemonic_nopin_nopassphrase() + + sig = self._sign(123456789123456) + self.assertEqual(len(sig.signature), 64) + + def test_osmosis_send_subunit_amount(self): + """500 uosmo is 0.000500 OSMO — six decimal places, no integer part. + The formatter must not collapse it to "0" or drop the tail.""" + self.requires_fullFeature() + self.requires_firmware("7.15.0") + self.setup_mnemonic_nopin_nopassphrase() + + sig = self._sign(500) + self.assertEqual(len(sig.signature), 64) + + def test_osmosis_send_non_uosmo_denom_is_refused(self): + """A non-uosmo MsgSend must not reach the device at all. + + osmosis_signTxUpdateMsgSend takes only (amount, to_address) and + hardcodes "denom":"uosmo" into the amino JSON it hashes, while + fsm_msgOsmosisMsgAck renders whatever denom arrived. Sending uatom + therefore DISPLAYS "1500000 uatom" and SIGNS 1500000 uosmo — the + display/signature divergence a hardware wallet exists to prevent, and + exploitable in the large: a big number of some worthless ibc/... token + on screen, a big number of OSMO in the signature. + + This asserts the fence, NOT that arbitrary denoms work. When the + firmware serializer takes a denom, replace this with the real check: + otherwise-identical uosmo and uatom transactions must produce + DIFFERENT signatures. + """ + self.requires_fullFeature() + self.requires_firmware("7.15.0") + self.setup_mnemonic_nopin_nopassphrase() + + with self.assertRaises(Exception) as ctx: + self._sign(1500000, denom='uatom') + self.assertIn('uosmo', str(ctx.exception)) + + def test_osmosis_amount_is_committed_to_the_signature(self): + """Guards the pairing between what is shown and what is signed: two + sends differing ONLY in amount must produce different signatures. If + they matched, the amount would not be in the digest and the confirm + screen would be decorative.""" + self.requires_fullFeature() + self.requires_firmware("7.15.0") + self.setup_mnemonic_nopin_nopassphrase() + + a = self._sign(1500000) + b = self._sign(1500001) + self.assertNotEqual(hexlify(a.signature), hexlify(b.signature)) + # Same key throughout — only the message differed. + self.assertEqual(hexlify(a.public_key), hexlify(b.public_key)) + + def test_osmosis_signing_is_deterministic(self): + """RFC6979: identical input must yield an identical signature. A + mismatch here means nonce generation is not deterministic, which is a + key-recovery risk long before it is a display problem.""" + self.requires_fullFeature() + self.requires_firmware("7.15.0") + self.setup_mnemonic_nopin_nopassphrase() + + first = self._sign(1500000) + second = self._sign(1500000) + self.assertEqual(hexlify(first.signature), hexlify(second.signature)) + + +if __name__ == '__main__': + unittest.main()