test(ethereum): bind the signing-guard tests to the signed pre-image

Both tests asserted only that r and s were 32 bytes. The regressions they
describe -- an RLP list header desynced by a priority-fee field that Stage 1
counts and Stage 2 skips, and a clear-sign handler hashing the confirmed
prefix instead of the full streamed calldata -- both still produce a
perfectly well-formed 32-byte r/s, so those assertions could not fail.

Reconstruct the intended digest and recover the signer against
ethereum_get_address: eth_sighash_eip1559 with an explicit zero priority fee
for the 1559 case, eth_sighash_legacy over the COMPLETE calldata for the
streamed case. Verified offline that a prefix-only digest does not recover to
the same address, so the recovery genuinely discriminates.

Adds test_streamed_handler_calldata_is_not_clear_signed for the second half
of the old docstring's claim, which nothing tested: the Sablier summary must
not be drawn for calldata the handler never saw. Compared against a
no-handler baseline of identical shape rather than a hardcoded screen count,
because the raw-data screen paginates with the calldata.

Author: pastaghost

tests/test_msg_ethereum_signing_guards.py

@@ -7,19 +7,73 @@
 #
 # These exercise the guards added in the firmware ethereum signing path.
 
+import time
 import unittest
 import common
 import binascii
 
 import keepkeylib.messages_ethereum_pb2 as eth_proto
 from keepkeylib.client import CallException
 from keepkeylib.tools import int_to_big_endian
+from keepkeylib.signed_metadata import (
+    eth_sighash_eip1559, eth_sighash_legacy, keccak256,
+)
 
 # Sablier proxy address — the withdrawFromSalary clear-sign handler target.
 SABLIER_PROXY = binascii.unhexlify("bd6a40bb904aea5a49c59050b5395f7484a4203d")
 RECIPIENT = binascii.unhexlify("1d1c328764a41bda0492b66baa30c4a339ff85ef")
 
 
+def recover_eth_signer(sig_r, sig_s, recovery_id, digest):
+    """Recover the 20-byte signer from (r, s, recovery_id) over `digest`.
+
+    Same approach as test_msg_thorchain_signtx.py. Recovering the signer --
+    rather than asserting r/s are 32 bytes -- is what makes these tests able to
+    fail: the regressions they describe (a desynced RLP list header, a prefix
+    hashed instead of the full calldata) still produce a perfectly well-formed
+    32-byte r/s, so a length assertion passes while the device signs a
+    pre-image that is not the transaction under test.
+    """
+    from ecdsa import VerifyingKey, SECP256k1, util
+    keys = VerifyingKey.from_public_key_recovery_with_digest(
+        sig_r + sig_s, digest, SECP256k1, hashfunc=None,
+        sigdecode=util.sigdecode_string,
+    )
+    return keccak256(keys[recovery_id].to_string())[-20:]
+
+
+class _ScreenRecorder(object):
+    """Record the framebuffer of every confirm screen an operation draws.
+
+    Mirrors ScreenRecorder in test_msg_ethereum_clearsign_additive.py.
+    """
+
+    SETTLE = 0.3
+
+    def __init__(self, client):
+        self.client = client
+        self.frames = []
+
+    def __enter__(self):
+        original = self.client.callback_ButtonRequest
+
+        def record(msg):
+            time.sleep(self.SETTLE)
+            self.frames.append((msg.code, bytes(self.client.debug.read_layout())))
+            return original(msg)
+
+        self.client.callback_ButtonRequest = record
+        return self
+
+    def __exit__(self, *exc):
+        del self.client.callback_ButtonRequest
+        return False
+
+    @property
+    def layouts(self):
+        return [layout for _, layout in self.frames]
+
+
 class TestMsgEthereumSigningGuards(common.KeepKeyTest):
     # ---- EIP-1559 type / fee / chain_id pre-image consistency ----
 
@@ -52,8 +106,9 @@ def test_eip1559_no_priority_fee_signs(self):
         self.requires_firmware("7.15.0")
         self.requires_fullFeature()
         self.setup_mnemonic_nopin_nopassphrase()
+        address_n = [0, 0]
         sig_v, sig_r, sig_s = self.client.ethereum_sign_tx(
-            n=[0, 0],
+            n=address_n,
             nonce=0,
             gas_limit=21000,
             max_fee_per_gas=20,  # no max_priority_fee_per_gas
@@ -64,6 +119,20 @@ def test_eip1559_no_priority_fee_signs(self):
         self.assertIn(sig_v, (0, 1))  # EIP-1559 recovery-id parity
         self.assertEqual(len(sig_r), 32)
         self.assertEqual(len(sig_s), 32)
+        # The regression this test names -- Stage 1 counting the priority-fee
+        # field while Stage 2 skips hashing it -- desyncs the RLP list header
+        # and yields a signature over a DIFFERENT pre-image. That signature is
+        # still 32+32 bytes, so only reconstructing the intended digest and
+        # recovering the signer can detect it. The absent field must encode as
+        # the empty integer, i.e. exactly max_priority_fee_per_gas = 0.
+        digest = eth_sighash_eip1559(
+            chain_id=1, nonce=0, max_priority_fee_per_gas=0,
+            max_fee_per_gas=20, gas_limit=21000, to=RECIPIENT,
+            value=10, data=b'',
+        )
+        signer = recover_eth_signer(sig_r, sig_s, sig_v, digest)
+        # NB: KeepKeyTest's assertEqual override takes no msg argument.
+        self.assertEqual(signer, self.client.ethereum_get_address(address_n))
 
     def test_type2_without_max_fee_rejected(self):
         """Typed prefix (0x02) is chosen from msg.type but the fee fields from
@@ -107,40 +176,99 @@ def test_legacy_with_max_fee_rejected(self):
 
     # ---- Contract clear-sign handler gate ----
 
+    # withdrawFromSalary selector + 2 words, then padded past 1024 bytes so
+    # data_total != data_initial_chunk.size (forces the streaming path).
+    STREAMED_TAIL = (
+        binascii.unhexlify(
+            "0000000000000000000000000000000000000000000000000000000000001210"
+            "0000000000000000000000000000000000000000000000000000000000000001"
+        ) + b"\x00" * 1100
+    )
+    HANDLER_SELECTOR = binascii.unhexlify("fea7c53f")   # withdrawFromSalary
+    # A selector the device has no clear-sign handler for. Same length, same
+    # streaming path, same `to` -- so the only thing that can change the screen
+    # sequence is whether the handler fired.
+    NO_HANDLER_SELECTOR = binascii.unhexlify("deadbeef")
+
+    STREAM_TX = dict(
+        n=[2147483692, 2147483708, 2147483648, 0, 0],
+        nonce=0xAB,
+        gas_price=0x24C988AC00,
+        gas_limit=0x26249,
+        value=0,
+        to=SABLIER_PROXY,
+        address_type=0,
+        chain_id=1,
+    )
+
+    def _sign_streamed(self, selector):
+        """Sign the streaming-path tx with `selector`, recording its screens."""
+        data = selector + self.STREAMED_TAIL
+        with _ScreenRecorder(self.client) as rec:
+            sig_v, sig_r, sig_s = self.client.ethereum_sign_tx(
+                data=data, **self.STREAM_TX)
+        return rec, data, (sig_v, sig_r, sig_s)
+
+    def _assert_signed_full_calldata(self, data, sig):
+        """Recover the signer against a digest over the COMPLETE calldata."""
+        sig_v, sig_r, sig_s = sig
+        self.assertEqual(len(sig_r), 32)
+        self.assertEqual(len(sig_s), 32)
+        self.assertIn(sig_v, [37, 38])  # EIP-155, chain_id = 1
+        digest = eth_sighash_legacy(
+            self.STREAM_TX['nonce'], self.STREAM_TX['gas_price'],
+            self.STREAM_TX['gas_limit'], SABLIER_PROXY,
+            self.STREAM_TX['value'], data, 1,
+        )
+        signer = recover_eth_signer(sig_r, sig_s, sig_v - 37, digest)
+        # NB: KeepKeyTest's assertEqual override takes no msg argument.
+        self.assertEqual(
+            signer, self.client.ethereum_get_address(self.STREAM_TX['n']))
+
     def test_contract_handler_streamed_calldata_signs_full_data(self):
-        """A handler selector (sablier withdrawFromSalary) whose calldata is
-        larger than the initial chunk must NOT be clear-signed from the prefix.
-        The device falls back to generic raw-data confirmation and signs the
-        full streamed calldata.
-
-        Asserts here that signing completes over the full (streamed) calldata;
-        the screen-level assertion (no 'Sablier' clear-sign summary appears for
-        streamed calldata) is verified on-device / on the emulator via
-        DebugLink layout."""
+        """A handler selector whose calldata is larger than the initial chunk
+        must sign the FULL streamed calldata, not the confirmed prefix.
+
+        Recovering the signer against a digest built over the complete `data`
+        is what makes this test able to fail: if the device hashed only the
+        first chunk it would still return a well-formed 32-byte r/s, and the
+        length assertions this test used to make would pass.
+        """
         self.requires_firmware("7.15.0")
         self.requires_fullFeature()
         self.setup_mnemonic_nopin_nopassphrase()
         self.client.apply_policy("AdvancedMode", 1)
-        # withdrawFromSalary selector + 2 words, then padded past 1024 bytes so
-        # data_total != data_initial_chunk.size (forces the streaming path).
-        data = binascii.unhexlify(
-            "fea7c53f"
-            + "0000000000000000000000000000000000000000000000000000000000001210"
-            + "0000000000000000000000000000000000000000000000000000000000000001"
-        ) + b"\x00" * 1100
-        sig_v, sig_r, sig_s = self.client.ethereum_sign_tx(
-            n=[2147483692, 2147483708, 2147483648, 0, 0],
-            nonce=0xAB,
-            gas_price=0x24C988AC00,
-            gas_limit=0x26249,
-            value=0,
-            to=SABLIER_PROXY,
-            address_type=0,
-            chain_id=1,
-            data=data,
-        )
-        self.assertEqual(len(sig_r), 32)
-        self.assertEqual(len(sig_s), 32)
+
+        _, data, sig = self._sign_streamed(self.HANDLER_SELECTOR)
+        self._assert_signed_full_calldata(data, sig)
+
+    def test_streamed_handler_calldata_is_not_clear_signed(self):
+        """The Sablier summary must NOT be drawn for streamed calldata.
+
+        The handler may only clear-sign what it actually verified, and it
+        cannot verify calldata it has not seen. So the streamed run must fall
+        back to the ordinary raw-data review -- the same screens a selector
+        with no handler at all draws.
+
+        Compared against a no-handler baseline of identical shape (same `to`,
+        same calldata length, same streaming path) rather than a hardcoded
+        screen count, because the raw-data screen paginates with the calldata.
+        A clear-signed run would add summary frames the baseline does not have.
+        """
+        self.requires_firmware("7.15.0")
+        self.requires_fullFeature()
+        self.setup_mnemonic_nopin_nopassphrase()
+        self.client.apply_policy("AdvancedMode", 1)
+
+        baseline, base_data, base_sig = self._sign_streamed(
+            self.NO_HANDLER_SELECTOR)
+        self._assert_signed_full_calldata(base_data, base_sig)
+
+        observed, data, sig = self._sign_streamed(self.HANDLER_SELECTOR)
+        self._assert_signed_full_calldata(data, sig)
+
+        # Any clear-sign summary would be one or more EXTRA confirm screens.
+        self.assertEqual(len(observed.frames), len(baseline.frames))
 
 
 if __name__ == "__main__":