Merge pull request #59 from BitHighlander/fix/alpha-audit-followups

fix(audit): require complete signing and screenshot evidence

Author: BitHighlander

.circleci/config.yml

@@ -11,7 +11,8 @@ jobs:
       - run:
           name: Clone python-keepkey (current branch)
           command: |
-            git clone --depth 1 -b "$CIRCLE_BRANCH" https://github.com/keepkey/python-keepkey.git .pykk
+            git clone --depth 1 -b "$CIRCLE_BRANCH" \
+              "https://github.com/${CIRCLE_PROJECT_USERNAME}/${CIRCLE_PROJECT_REPONAME}.git" .pykk
             cd .pykk && git submodule update --init --recursive
 
       # ────────────────────────────────────────────────────────────────
@@ -27,8 +28,12 @@ jobs:
             # Move python-keepkey out of the way
             mv .pykk ../
 
-            # Clone firmware repository (expects $FIRMWARE_REPO env var)
-            git clone --depth 1 -b master "$FIRMWARE_REPO" .
+            # Test the same exact companion implementation as the GitHub
+            # Actions integration lane. Bump deliberately with that workflow.
+            git init .
+            git remote add origin https://github.com/BitHighlander/keepkey-firmware.git
+            git fetch --depth 1 origin 54b169a7036b29db22944d962fb666b50aef9083
+            git checkout --detach FETCH_HEAD
 
             # Initialise firmware submodules
             git submodule update --init --recursive

.github/workflows/ci.yml

@@ -130,7 +130,7 @@ jobs:
           # integration-btc job) and the Ironwood known-answer vectors. So this
           # job validates 7.16.0; it does not validate the RC18 dependency
           # graph. Bump deliberately, and re-read that claim when you do.
-          ref: 6d5e1917b7cc80a5b84a6ec79c82cffef3aab4c9
+          ref: 54b169a7036b29db22944d962fb666b50aef9083
           path: keepkey-firmware
 
       # NOT `submodules: recursive`. trezor-firmware carries a micropython

keepkeylib/client.py

@@ -523,18 +523,26 @@ def _capture_oled(self):
             print("[SCREENSHOT] ERROR: %s" % e, file=sys.stderr)
             traceback.print_exc(file=sys.stderr)
 
+    def capture_oled(self):
+        """Capture a settled confirmation screen in a manual protocol flow.
+
+        Tests that use call_raw() need per-step control and never dispatch
+        callback_ButtonRequest(). Keeping the settle delay in this public
+        helper makes their evidence equivalent to the automatic callback path.
+        """
+        if SCREENSHOT:
+            time.sleep(SCREENSHOT_SETTLE_SECONDS)
+        self._capture_oled()
+
     def callback_ButtonRequest(self, msg):
         if self.verbose:
             log("ButtonRequest code: " + get_buttonrequest_value(msg.code))
 
         # The firmware emits ButtonRequest immediately before drawing the
         # confirmation. Allow the emulator's render transition to settle so
         # regression evidence cannot capture a partially drawn OLED.
-        if SCREENSHOT:
-            time.sleep(SCREENSHOT_SETTLE_SECONDS)
-
         # Capture OLED screenshot BEFORE pressing button (confirmation screen)
-        self._capture_oled()
+        self.capture_oled()
 
         if self.auto_button:
             if self.verbose:

keepkeylib/eth/ethereum_tokens.py

@@ -29,20 +29,30 @@ def add_tokens(self, network):
             fullpath = os.path.join(dirname, filename)
 
             if not os.path.isfile(fullpath):
-                return
+                continue
 
             with open(fullpath, 'r') as f:
                 token = json.load(f)
 
                 self.tokens.append(ETHToken(token, network))
 
     def build(self):
+        source = HERE + '/ethereum-lists/src/tokens'
+        if not os.path.isdir(source):
+            raise RuntimeError(
+                'vetted ethereum-lists token source is missing; initialize '
+                'submodules recursively before generating firmware tables')
+
         with open(HERE + '/ethereum_networks.json', 'r') as f:
             networks = json.load(f)
 
             for network in networks:
                 self.add_tokens(network)
 
+        if not self.tokens:
+            raise RuntimeError(
+                'vetted ethereum-lists token source produced zero candidates')
+
     def serialize_c(self, outf):
         # Flash budget: this table is the largest read-only symbol in the ARM
         # image. See token_policy for why it is capped rather than complete.

scripts/generate-test-report.py

@@ -526,10 +526,9 @@ def _arg_shown(a):
           '0x transformERC20 raw disclosure',
           'A 1480-byte transformERC20 payload exceeds one 1024-byte chunk. The device must NOT '
           'clear-sign it as a token swap, because the bytes past the initial chunk are hashed '
-          'without being decoded. With AdvancedMode on it falls to the raw path, where the byte '
-          'count shown must be the FULL length (1480), not the chunk length (1024) - a short '
-          'count would under-report what is being signed.',
-          ['Raw contract data screen showing the full byte count']),
+          'without being decoded. With AdvancedMode on it falls to the raw path, where the final '
+          'screen commits to all 1480 bytes with a Keccak-256 the user can compare to the host.',
+          ['Complete contract-data Keccak-256 commitment']),
          ('J2', 'test_msg_ethereum_erc20_0x_signtx', 'test_sign_0x_swap_ERC20_to_ETH',
           '0x sellToUniswap names both assets',
           'Clear-signing is only honest when BOTH token words resolve to known assets. This '
@@ -538,18 +537,24 @@ def _arg_shown(a):
           ['Swap screen naming both assets and amounts']),
          ('J3', 'test_msg_ethereum_erc20_0x_signtx', 'test_sign_longdata_swap',
           'Long 0x calldata stays disclosed',
-          'Calldata spanning multiple chunks must not silently lose its tail from the display '
-          'while remaining inside the signature.',
-          ['Contract data screen']),
+          'Calldata spanning multiple chunks must not silently lose its tail from the user-visible '
+          'commitment while remaining inside the signature.',
+          ['Complete contract-data Keccak-256 commitment']),
+         ('J4', 'test_msg_ethereum_signing_guards',
+          'test_streamed_calldata_tail_changes_user_commitment',
+          'A streamed tail changes the approval screens',
+          'Signs two equal-length payloads with identical initial 1024-byte chunks and a one-bit '
+          'difference in the final EthereumTxAck byte. Their ordered OLED frame sequences must '
+          'differ, proving the complete-calldata Keccak-256 -- not merely the visible prefix or '
+          'declared length -- reaches the user before either signature is emitted.',
+          ['Complete contract-data Keccak-256 commitment']),
          ('J8', 'test_msg_ethereum_signing_guards',
           'test_contract_handler_streamed_calldata_signs_full_data',
           'Streamed calldata is fully covered',
-          'Calldata delivered across several chunks must be hashed in full and disclosed in full. '
-          'This is the positive control for the chunk-completeness gate. NOTE: every test in '
-          'test_msg_ethereum_signing_guards currently SKIPS in CI under requires_firmware, so no '
-          'screen can be captured for it yet - the screenshot list stays empty until the gate '
-          'opens, rather than declaring an expectation nothing can satisfy.',
-          []),
+          'Calldata delivered across several chunks must be hashed in full, and the final OLED '
+          'commitment must cover that same complete byte string. This is the positive control for '
+          'the chunk-completeness gate.',
+          ['Complete contract-data Keccak-256 commitment']),
          ('J9', 'test_msg_ethereum_signing_guards', 'test_eip1559_requires_chain_id',
           'Omitted chain_id is refused before any screen',
           'Without a chain_id the device cannot name the network, and a signature would be '
@@ -1133,21 +1138,25 @@ def _arg_shown(a):
           'Failure on the wire.',
           []),
          ('E17', 'test_msg_ethereum_erc20_uniswap_liquidity', 'test_sign_uni_approve_liquidity_ETH',
-          'Uniswap V2 add-liquidity approve (pending)',
-          'PENDING, disclosed: known emulator limitation — an approve to an unknown (non-registry) '
-          'token contract cannot complete against the kkemu emulator (matches the sibling '
-          'add/remove-liquidity skips below); the device-firmware path is not in question, only '
-          'CI emulator coverage. Real-device testing is unaffected.',
-          []),
+          'Uniswap V2 LP-token approval',
+          'Approves the Uniswap V2 FOX/WETH LP token for the canonical router. The exact pool '
+          'identity and full-LP allowance are shown before the generic fee review, and the fixed '
+          'signature proves the reviewed transaction bytes are the bytes signed.',
+          ['Full LP allowance', 'LP token and pool address', 'Fee and final approval']),
          ('E18', 'test_msg_ethereum_erc20_uniswap_liquidity', 'test_sign_uni_add_liquidity_ETH',
-          'Uniswap V2 add liquidity ETH+token (pending)',
-          'PENDING, disclosed: same emulator limitation as E17 — a daily-driver LP-deposit flow '
-          'with no PDF proof on this build; tracked for real-device verification.',
-          []),
+          'Uniswap V2 add liquidity ETH+token',
+          'Clear-signs both desired/minimum FOX and ETH amounts, the signed recipient, and the '
+          'deadline before the final fee review. The fixed signature binds those confirmations '
+          'to the complete addLiquidityETH calldata.',
+          ['FOX desired amount', 'FOX minimum', 'Recipient', 'ETH desired amount',
+           'ETH minimum', 'Deadline', 'Fee and final approval']),
          ('E19', 'test_msg_ethereum_erc20_uniswap_liquidity', 'test_sign_uni_remove_liquidity_ETH',
-          'Uniswap V2 remove liquidity ETH+token (pending)',
-          'PENDING, disclosed: same emulator limitation as E17.',
-          []),
+          'Uniswap V2 remove liquidity ETH+token',
+          'Clear-signs the LP burn amount, minimum FOX and ETH outputs, the non-self signed '
+          'recipient, and deadline before the final fee review. This is the regression for the '
+          'recipient-confirmation path that previously cancelled after the user approved it.',
+          ['LP burn amount', 'FOX minimum', 'Recipient', 'ETH minimum', 'Deadline',
+           'Fee and final approval']),
          ('E20', 'test_msg_ethereum_thorchain_deposit', 'test_deposit_legacy_selector',
           'THORChain router deposit() (legacy selector)',
           'Cross-chain swap via the THORChain router contract — a daily-driver EVM<->THORChain '
@@ -2930,7 +2939,7 @@ def _arg_shown(a):
        'f2cee375...912090f and messageHash c52c0ee5...4b371e, both published, both matched on '
        'hardware and in the emulator.',
        ['Domain name', 'Domain version', 'chainId', 'verifyingContract (42 chars, in full)',
-        'Cow / wallet', 'Bob / wallet', 'contents']),
+        'From name: Cow', 'From wallet', 'To name: Bob', 'To wallet', 'contents']),
       ('TD2', 'test_msg_eip712_streaming', 'test_array_of_structs_walks',
        'An array of structs walks and signs',
        'Arrays hash WITHOUT a typeHash prefix -- enc(array) is the keccak of the concatenated '

tests/test_msg_eip712_streaming.py

@@ -118,6 +118,10 @@ def _walk(self, doc, max_steps=400):
         resp = self.client.call_raw(msg)
         for _ in range(max_steps):
             if isinstance(resp, proto.ButtonRequest):
+                # This is a manual, device-driven call_raw() loop, so no
+                # callback_ButtonRequest() will capture the field being
+                # approved. Retain it while that exact field is still active.
+                self.client.capture_oled()
                 self.client.debug.press_yes()
                 resp = self.client.call_raw(proto.ButtonAck())
             elif isinstance(resp, eth.EthereumTypedDataStructRequest):
@@ -140,6 +144,8 @@ def setUp(self):
         self.requires_structured_eip712()
         self.setup_mnemonic_nopin_nopassphrase()
         self.client.apply_policy('AdvancedMode', 1)
+        # The report entries describe typed-data fields, not the policy prompt.
+        self.client.reset_screenshots()
 
     def test_spec_example_matches_the_published_hashes(self):
         """The device's own hashes equal the EIP-712 reference implementation's.

tests/test_msg_ethereum_erc20_uniswap_liquidity.py

@@ -27,24 +27,6 @@
 
 class TestMsgEthereumUniswaptxERC20(common.KeepKeyTest):
 
-    def setUp(self):
-        super(TestMsgEthereumUniswaptxERC20, self).setUp()
-        # Every test in this file approves or spends against the ETH/FOX pool,
-        # whose contract is NOT in the token table. Approving an unknown token
-        # contract does not complete on the emulator: the device never returns,
-        # so these tests HANG instead of failing, and CI kills the whole run on
-        # its no-output timeout -- taking every later test with it.
-        #
-        # This is a firmware-side limitation, not a gap in the tests. It is
-        # gated here rather than deleted so the coverage returns automatically
-        # once the firmware completes this path. Known-token approves
-        # (test_msg_ethereum_erc20_approve) run here and pass; on real hardware
-        # this path is exercised by the app.
-        if self.client.features.firmware_variant[0:8] == "Emulator":
-            self.skipTest(
-                "Uniswap liquidity against an unknown token contract does not "
-                "complete on the emulator")
-    
     def test_sign_uni_approve_liquidity_ETH(self):
         self.requires_fullFeature()
         self.requires_firmware("7.1.0")
@@ -102,7 +84,9 @@ def test_sign_uni_add_liquidity_ETH(self):
 
     def test_sign_uni_remove_liquidity_ETH(self):
         self.requires_fullFeature()
-        self.requires_firmware("7.1.0")
+        # Sending the withdrawn assets to a third-party recipient was refused
+        # by RC18. The reviewed external-recipient flow lands on the 7.16 line.
+        self.requires_firmware("7.16.0")
         self.setup_mnemonic_nopin_nopassphrase()
 
         # remove liquidity from the ETH/FOX pool

tests/test_msg_ethereum_signing_guards.py

@@ -204,6 +204,10 @@ def test_legacy_with_max_fee_rejected(self):
     def _sign_streamed(self, selector):
         """Sign the streaming-path tx with `selector`, recording its screens."""
         data = selector + self.STREAMED_TAIL
+        return self._sign_streamed_data(data)
+
+    def _sign_streamed_data(self, data):
+        """Sign exact streamed calldata and retain every approval framebuffer."""
         with _ScreenRecorder(self.client) as rec:
             sig_v, sig_r, sig_s = self.client.ethereum_sign_tx(
                 data=data, **self.STREAM_TX)
@@ -270,6 +274,35 @@ def test_streamed_handler_calldata_is_not_clear_signed(self):
         # Any clear-sign summary would be one or more EXTRA confirm screens.
         self.assertEqual(len(observed.frames), len(baseline.frames))
 
+    def test_streamed_calldata_tail_changes_user_commitment(self):
+        """Bytes arriving after the initial chunk must change what is shown.
+
+        Both transactions have identical first 1024-byte protobuf chunks and
+        identical lengths; only the final byte in EthereumTxAck differs.  The
+        pre-fix firmware displayed the same prefix/count screens for both and
+        then hashed the distinct tails invisibly.  The complete-calldata
+        Keccak-256 confirmation makes the approval sequences distinguishable.
+        """
+        self.requires_firmware("7.16.0")
+        self.requires_fullFeature()
+        self.setup_mnemonic_nopin_nopassphrase()
+        self.client.apply_policy("AdvancedMode", 1)
+
+        base_data = self.NO_HANDLER_SELECTOR + self.STREAMED_TAIL
+        changed_data = base_data[:-1] + binascii.unhexlify(
+            "%02x" % (bytearray(base_data)[-1] ^ 0x01))
+        self.assertEqual(base_data[:1024], changed_data[:1024])
+
+        baseline, _, baseline_sig = self._sign_streamed_data(base_data)
+        self._assert_signed_full_calldata(base_data, baseline_sig)
+        changed, _, changed_sig = self._sign_streamed_data(changed_data)
+        self._assert_signed_full_calldata(changed_data, changed_sig)
+
+        self.assertEqual(len(baseline.frames), len(changed.frames))
+        self.assertNotEqual(
+            baseline.layouts, changed.layouts,
+            "a streamed tail changed the signature but not the user's screens")
+
 
 if __name__ == "__main__":
     unittest.main()

tests/test_msg_recoverydevice_cipher.py

@@ -111,11 +111,18 @@ def test_nopin_nopassphrase(self):
         ret = self.client.call_raw(proto.ButtonAck())
 
         mnemonic_words = mnemonic.split(' ')
+        captured_cipher = False
 
         for index, word in enumerate(mnemonic_words):
             for character in word:
                 self.assertIsInstance(ret, proto.CharacterRequest)
                 cipher = self.client.debug.read_recovery_cipher()
+                if not captured_cipher:
+                    # CharacterRequest is driven manually and never reaches
+                    # callback_ButtonRequest(); capture only after DebugLink
+                    # proves this is the active randomized cipher grid.
+                    self.client.capture_oled()
+                    captured_cipher = True
 
                 encoded_character = cipher[ord(character) - 97]
                 ret = self.client.call_raw(proto.CharacterAck(character=encoded_character))

tests/test_msg_resetdevice.py

@@ -87,6 +87,10 @@ def test_reset_device(self):
         mnemonic = []
         while isinstance(resp, proto.ButtonRequest):
             mnemonic.append(self.client.debug.read_reset_word())
+            if len(mnemonic) == 1:
+                # Manual call_raw() flow: bind the report evidence to a word
+                # the DebugLink confirms is currently on the device.
+                self.client.capture_oled()
             self.client.debug.press_yes()
             resp = self.client.call_raw(proto.ButtonAck())
 
@@ -132,6 +136,7 @@ def test_reset_device_dice(self):
         # Device announces the on-device dice entry screen
         self.assertIsInstance(ret, proto.ButtonRequest)
         self.assertEqual(ret.code, proto_types.ButtonRequest_DiceRoll)
+        self.client.capture_oled()
 
         # Ack without blocking on the reply: the device only leaves the dice
         # screen once the rolls are complete, and input is ignored until the
@@ -169,6 +174,7 @@ def test_reset_device_dice(self):
         dice_digest = self.client.debug.read_dice_digest()
         self.assertEqual(dice_digest,
                          hashlib.sha256(expected.encode('ascii')).digest())
+        self.client.capture_oled()
 
         self.client.debug.press_yes()
         ret = self.client.call_raw(proto.ButtonAck())